sql: create new slice when removing backreference items #97631

chengxiong-ruan · 2023-02-24T16:43:49Z

Previously we reused the underlying slots of the DependedOnBy slice when removing items from it. This is problematic because we normally have to iterate the same slice to figure out what to drop. Iterating on a slice which is rewritten can be problematic unless we always know how to smartly move the cursor backward. This commit changes it to just create a new slice to avoid the hazzard.

Release note: None.

cockroach-teamcity · 2023-02-24T16:43:59Z

This change is

fqazi

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan)

pkg/sql/alter_table.go line 1615 at r1 (raw file):

			return nil, err
		}
		if depDesc.Dropped() {

Is there another bug we are fixing here, we should maybe have a commit message and release not if it was public.

pkg/sql/drop_table.go line 650 at r1 (raw file):

	refs []descpb.TableDescriptor_Reference, id descpb.ID,
) []descpb.TableDescriptor_Reference {
	var updatedRefs []descpb.TableDescriptor_Reference

Pre-allocate based on the size of the old refs, incase nothing gets removed. That way we don't keep on growing.

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan and @fqazi)

pkg/sql/drop_table.go line 650 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Pre-allocate based on the size of the old refs, incase nothing gets removed. That way we don't keep on growing.

This can be N^2 now in the worst case, right? Some other options are to collect the ids to remove or to clone the slice before iterating. This is yet another reason why the legacy schema changer is bad. I imagine the recursion makes this all tricky. What is the loop we were messing up?

chengxiong-ruan · 2023-02-24T17:37:21Z

pkg/sql/drop_table.go line 650 at r1 (raw file):

Previously, ajwerner wrote…

This can be N^2 now in the worst case, right? Some other options are to collect the ids to remove or to clone the slice before iterating. This is yet another reason why the legacy schema changer is bad. I imagine the recursion makes this all tricky. What is the loop we were messing up?

Yeah, N^2 can be bad if the dep graph is large.
The loops under concern here is DROP INDEX CASCADE and DROP COLUMN CASCADE
But yeah, I think collecting ids to drop is a good idea here.

chengxiong-ruan

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @fqazi)

pkg/sql/alter_table.go line 1615 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Is there another bug we are fixing here, we should maybe have a commit message and release not if it was public.

Yeah, another bug was accidentally fixed here...I added both bugs to commit message and added release note.

pkg/sql/drop_table.go line 650 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Yeah, N^2 can be bad if the dep graph is large.
The loops under concern here is DROP INDEX CASCADE and DROP COLUMN CASCADE
But yeah, I think collecting ids to drop is a good idea here.

Done

ajwerner

I like this much more

fqazi

This looks much nicer

Reviewed 1 of 3 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @chengxiong-ruan)

ajwerner

The version gate feels like it should have been a separate PR.

Reviewed 1 of 3 files at r2, 1 of 4 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @chengxiong-ruan and @fqazi)

pkg/sql/schemachanger/scdecomp/decomp.go line 123 at r4 (raw file):

			panic(
				scerrors.NotImplementedErrorf(
					nil,

nit: can you name the nil with a comment?

chengxiong-ruan · 2023-02-28T04:37:40Z

TFTRs!
bors r+

chengxiong-ruan · 2023-02-28T04:38:07Z

bors r-

craig · 2023-02-28T04:38:09Z

Canceled.

chengxiong-ruan · 2023-02-28T04:41:28Z

The version gate feels like it should have been a separate PR.
Actually it's needed in this PR... the mixed-22.2-23.1 logictest nicely failed the tests I added because it can't do drop column which cascade drop the function.

chengxiong-ruan · 2023-02-28T04:42:11Z

The version gate feels like it should have been a separate PR.

Sorry.. terrible formatting....
Actually it's needed in this PR... the mixed-22.2-23.1 logictest nicely failed the tests I added because it can't do drop column which cascade drop the function.

Fixes: cockroachdb#97546 Fixes: cockroachdb#96368 We are reusing the underlying slots of the `DependedOnBy` slice when removing items from it. This is problematic because we normally have to iterate the same slice to figure out what to drop. Iterating on a slice which is rewritten can be problematic unless we always know how to smartly move the cursor backward. This commit remove such kind of hazard by collecting descriptor IDs to drop before actually dropping everything. Releaste note (sql change): This commit fixes two bugs prevent users from doing `DROP COLUMN CASCADE` and `DROP INDEX CASCADE` when dependecies on the Table from Views and Functions get complex. They all happened When a table is referenced multiple times in more than one Views and Functions. Release note: None.

chengxiong-ruan · 2023-02-28T04:45:02Z

pkg/sql/schemachanger/scdecomp/decomp.go line 123 at r4 (raw file):

Previously, ajwerner wrote…

nit: can you name the nil with a comment?

done.

chengxiong-ruan · 2023-02-28T14:30:13Z

TFTRs!
bors r+

craig · 2023-02-28T14:55:49Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-02-28T14:55:58Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 2e05878 to blathers/backport-release-22.2-97631: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

In cockroachdb#97631 we refactor the method to drorp index and column cascade dependents. But we didn't return a correct droppedViews, instead, we returned a nil. This wasn't caught by the `event_log` logic test because we skip `local-legacy-schema-changer` test config in v23.1. Luckily,this was caught when backporting to v22.2 because there was a fallback. Epic: None Release note: None

96382: jobs: add VIEWJOB role option r=HonoreDB a=jayshrivastava #### jobs: allow CONTROLJOB users to view all jobs This is a preliminary change before adding the `VIEWJOB` role option. The purpose of the `VIEWJOB` role option will be to allow TSEs to view jobs in admin clusters without modifying them. This requires access to view all jobs, including ones owned by admins. Adding `VIEWJOB` will conflict with the present `CONTROLJOB` implementation. It will be stronger than `CONTROLJOB` because it lets one view admin jobs when `CONTROLJOB` doesn't, yet it will be weaker because it only allows viewing jobs and not pause/cancel/resume-ing them. This change is introduced to make `CONTROLJOB` at least as strong as `VIEWJOB`: it now allows for all jobs to be viewed. In other words, `VIEWJOB` lets you do a subset of things `CONTROLJOB` lets you do. Also, the reason that `CONTROLJOB` prevents access to admin-owned jobs is not well defined. This is tracked in #96432. Release note (general change): Previously, users with the `CONTROLJOB` role option could not view owned by admins (ex. via `SHOW JOBS`). Now, they can. Epic: none --- #### jobs: add VIEWJOB role option Release note (general change): Users can now be granted the `VIEWJOB` role option to be able to view all jobs (ex. via `SHOW JOBS`). This role can be revoked using `NOVIEWJOB`. Epic: none 97746: sql,schemachanger: ADD PRIMARY KEY NOT VALID returns an error r=Xiang-Gu a=Xiang-Gu Fixes #96729 Release note (sql change): When we add an unvalidated PK constraint to a table, assuming its PK was on the implicit "rowid" column, will return an error. This is compatible to what PG will do. 97802: sql: return droppedView instead of nil r=chengxiong-ruan a=chengxiong-ruan In #97631 we refactor the method to drorp index and column cascade dependents. But we didn't return a correct droppedViews for event logging, instead, we returned a nil. This wasn't caught by the `event_log` logic test because we `local-legacy-schema-changer` test config. Though this was caught when backporting to v22.2 because there was a fallback. Epic: None Release note: None Co-authored-by: Jayant Shrivastava <[email protected]> Co-authored-by: Xiang Gu <[email protected]> Co-authored-by: Chengxiong Ruan <[email protected]>

chengxiong-ruan requested a review from a team February 24, 2023 16:43

chengxiong-ruan added the backport-22.2.x label Feb 24, 2023

chengxiong-ruan force-pushed the create-new-slice-when-removing-dependencies branch from 21cf090 to fe645a5 Compare February 24, 2023 17:13

fqazi requested changes Feb 24, 2023

View reviewed changes

ajwerner reviewed Feb 24, 2023

View reviewed changes

chengxiong-ruan force-pushed the create-new-slice-when-removing-dependencies branch from fe645a5 to b6680ab Compare February 24, 2023 18:59

chengxiong-ruan commented Feb 24, 2023

View reviewed changes

ajwerner reviewed Feb 24, 2023

View reviewed changes

chengxiong-ruan force-pushed the create-new-slice-when-removing-dependencies branch from b6680ab to b2d689e Compare February 27, 2023 17:37

fqazi approved these changes Feb 27, 2023

View reviewed changes

chengxiong-ruan force-pushed the create-new-slice-when-removing-dependencies branch from b2d689e to bf5d0f3 Compare February 28, 2023 02:31

chengxiong-ruan mentioned this pull request Feb 28, 2023

sql/schemachanger: version gate dropping constraints / functions #97769

Merged

chengxiong-ruan force-pushed the create-new-slice-when-removing-dependencies branch from bf5d0f3 to 181008f Compare February 28, 2023 03:44

ajwerner approved these changes Feb 28, 2023

View reviewed changes

chengxiong-ruan force-pushed the create-new-slice-when-removing-dependencies branch from 181008f to 2e05878 Compare February 28, 2023 04:44

craig bot merged commit 46b1afa into cockroachdb:master Feb 28, 2023

chengxiong-ruan mentioned this pull request Feb 28, 2023

release-22.2: sql: create new slice when removing backreference items #97787

Merged

chengxiong-ruan mentioned this pull request Feb 28, 2023

sql: return droppedView instead of nil #97802

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: create new slice when removing backreference items #97631

sql: create new slice when removing backreference items #97631

chengxiong-ruan commented Feb 24, 2023 •

edited

Loading

cockroach-teamcity commented Feb 24, 2023

fqazi left a comment

ajwerner left a comment

chengxiong-ruan commented Feb 24, 2023

chengxiong-ruan left a comment

ajwerner left a comment

fqazi left a comment

ajwerner left a comment

chengxiong-ruan commented Feb 28, 2023

chengxiong-ruan commented Feb 28, 2023

craig bot commented Feb 28, 2023

chengxiong-ruan commented Feb 28, 2023

chengxiong-ruan commented Feb 28, 2023

chengxiong-ruan commented Feb 28, 2023

chengxiong-ruan commented Feb 28, 2023

craig bot commented Feb 28, 2023

blathers-crl bot commented Feb 28, 2023

sql: create new slice when removing backreference items #97631

sql: create new slice when removing backreference items #97631

Conversation

chengxiong-ruan commented Feb 24, 2023 • edited Loading

cockroach-teamcity commented Feb 24, 2023

fqazi left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

chengxiong-ruan commented Feb 24, 2023

chengxiong-ruan left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

chengxiong-ruan commented Feb 28, 2023

chengxiong-ruan commented Feb 28, 2023

craig bot commented Feb 28, 2023

chengxiong-ruan commented Feb 28, 2023

chengxiong-ruan commented Feb 28, 2023

chengxiong-ruan commented Feb 28, 2023

chengxiong-ruan commented Feb 28, 2023

craig bot commented Feb 28, 2023

blathers-crl bot commented Feb 28, 2023

chengxiong-ruan commented Feb 24, 2023 •

edited

Loading