schemachanger: Implement DROP CONSTRAINT in declarative schema changer
#96115
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
Xiang-Gu
force-pushed
the
implement-drop-constraint
branch
from
af9ed1f to
5a1f065
Compare
Xiang-Gu
force-pushed
the
implement-drop-constraint
branch
4 times, most recently
from
b07ed00 to
7f6f995
Compare
|
This is ready for a look! |
Xiang-Gu
changed the title
DROP CONSTRAINT in declarative schema changerDROP CONSTRAINT in declarative schema changer
Xiang-Gu
force-pushed
the
implement-drop-constraint
branch
from
7f6f995 to
300bcde
Compare
postamar
reviewed
Jan 31, 2023
There was a problem hiding this comment.
I have only minor comments, besides: this needs some more test cases involving awkward scenarios such as dropping check constraints with type/sequence refs, partial unique-without-indexes, etc. They don't necessarily have to be end-to-end. Logic test cases are fine, provided they exist (do they?) also the data-driven tests in scplan might be useful to check that the planner does the right thing in those annoying multi-statement cases. I don't see the cross-descriptor constraint is absent before * rules show up anywhere in this PR.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_constraint.go
Show resolved
Hide resolved
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_constraint.go
Show resolved
Hide resolved
ajwerner
reviewed
Jan 31, 2023
Comment on lines
-21
to
-23
| 9,AlterTableDropConstraint/alter_table_drop_1_check_constraint | ||
| 9,AlterTableDropConstraint/alter_table_drop_2_check_constraints | ||
| 9,AlterTableDropConstraint/alter_table_drop_3_check_constraints |
There was a problem hiding this comment.
do you have a sense of where these two come from? Might be worth looking at the traces before and after to get a sense. I don't think this blocks this PR, but we should understand it.
There was a problem hiding this comment.
I honestly don't. I'd love to understand it more afterward (maybe in the stability period?). I remember seeing similar results for other constraint-related statements in the new schema changer.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_constraint.go
Outdated
Show resolved
Hide resolved
pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_object.go
Outdated
Show resolved
Hide resolved
pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_object.go
Outdated
Show resolved
Hide resolved
Xiang-Gu
force-pushed
the
implement-drop-constraint
branch
2 times, most recently
from
be934f0 to
ce2d209
Compare
This commit changed sc operations emmitted on the adding/dropping path of FK constraint. The major change is to recognize that FK constraints are "cross-descriptor" constraints and thus we need to make accompanying changes in the referenced tables whenever we make changes to the referencing table.
Xiang-Gu
force-pushed
the
implement-drop-constraint
branch
2 times, most recently
from
3a8f6f5 to
33ad697
Compare
This commit implements `DROP CONSTRAINT`, turn it on by default, and added tests.
Xiang-Gu
force-pushed
the
implement-drop-constraint
branch
from
33ad697 to
2428a43
Compare
|
CI failed due to a flaky test, borsing bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Feb 1, 2023
95622: backupccl,storage: add logs around manifest handling and ExportRequest pagination r=stevendanna a=adityamaru backupccl: add logging to backup manifest handling Release note: None storage: log the ExportRequest pagination reason Release note: None Epic: None 95865: cmd/roachtest: adapt disk-stall detection roachtest r=nicktrav,erikgrinaker a=jbowens Move the existing disk-stall/* roachtests under disk-stall/fuse/* (for the FUSE filesystem approach to stalling) and skip them for now. Currently, they're not capable of stalling the disk longer 50us (see #95886), which makes them unreliable at exercising stalls. Add two new roachtests, disk-stall/dmsetup and disk-stall/cgroup that use dmsetup and cgroup bandwidth restrctions respectively to reliably induce a write stall for an indefinite duration. Informs #94373. Epic: None Release note: None 95999: multitenant: add multitenant/shared-process/basic roachtest r=stevendanna a=msbutler This patch introduces a simple roachtest that runs in a shared-process tenant. This test imports a 500 tpcc workload (about 30 GB of replicated data), and runs the workload for 10 minutes. The test is run on a 4 node, 4vcpu cluster with local ssds. A future patch could complicate the test by running schema changes or other bulk operations. Fixes #95990 Release note: None 96115: schemachanger: Implement `DROP CONSTRAINT` in declarative schema changer r=Xiang-Gu a=Xiang-Gu This PR implements `ALTER TABLE t DROP CONSTRAINT cons_name` in declarative schema changer. Supported constraints include Checks, FK, and UniqueWithoutIndex. Dropping PK or Unique constraints will fall back to legacy schema changer, which in turn spits out an "not supported yet" error. Epic: None 96202: opt: inverted-index accelerate filters of the form j->0 @> '{"b": "c"} r=Shivs11 a=Shivs11 Previously, the optimizer did not plan inverted index scans for filters having an integer as the index for the fetch value in a filter alongside the "contains" or the "contained by" operator. To address this, we now build JSON arrays from fetch value expressions with integer indexes. From these JSON arrays, inverted spans are built for constraining scans over inverted indexes. With these changes chains of both integer and string fetch value operators are now supported alongside the "contains" and the "contained by" operators. (e.g., j->0 `@>` '{"b": "c"}' and j->0 <@ '{"b": "c"}'). Epic: [CRDB-3301](https://cockroachlabs.atlassian.net/browse/CRDB-3301) Fixes: #94667 Release note (performance improvement): The optimizer now plans inverted index scans for queries that filter by JSON fetch value operators (->) with integer indices alongside the "contains" or the "contained by" operators, e.g, json_col->0 `@>` '{"b": "c"}' or json_col->0 <@ '{"b": "c"}' 96235: sem/tree: add support for producing vectorized data from strings r=cucaroach a=cucaroach tree.ValueHandler exposes raw machine type hooks that are used by vec_handler to build coldata.Vec's. Epic: CRDB-18892 Informs: #91831 Release note: None 96328: udf: allow strict UDF with no arguments r=DrewKimball a=DrewKimball This patch fixes the case when a strict UDF (returns null on null input) has no arguments. Previously, attempting to call such a function would result in `ERROR: reflect: call of reflect.Value.Pointer on zero Value`. Fixes #96326 Release note: None 96366: release: skip nil GitHub events r=celiala a=rail Previously, we referenced `*event.Event`, but in some cases the event objects are `nil`. This PR skips the nil GitHub event objects. Epic: none Release note: None Co-authored-by: adityamaru <[email protected]> Co-authored-by: Jackson Owens <[email protected]> Co-authored-by: Michael Butler <[email protected]> Co-authored-by: Xiang Gu <[email protected]> Co-authored-by: Shivam Saraf <[email protected]> Co-authored-by: Tommy Reilly <[email protected]> Co-authored-by: Drew Kimball <[email protected]> Co-authored-by: Rail Aliiev <[email protected]>
|
Build failed (retrying...): |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements
ALTER TABLE t DROP CONSTRAINT cons_namein declarative schema changer.Supported constraints include Checks, FK, and UniqueWithoutIndex.
Dropping PK or Unique constraints will fall back to legacy schema changer, which in turn spits out an "not supported yet" error.
Epic: None