sql/schemachanger: cleaned some dep rules in declarative schema changer#82907
sql/schemachanger: cleaned some dep rules in declarative schema changer#82907Xiang-Gu wants to merge 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
373d8ad to
90b2157
Compare
ajwerner
left a comment
There was a problem hiding this comment.
This needs more writes. I'm curious to look at the fallout.
Reviewed 4 of 22 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @Xiang-Gu)
pkg/sql/schemachanger/scpb/elements.proto line 171 at r1 (raw file):
// TODO(postamar): remove this when we can have more expressive rule defs // See the dep rules for how this is used, and why it's not ideal. bool is_relation_being_dropped = 10;
reserve the ID instead of just deleting it:
reserved 10;
pkg/sql/schemachanger/scpb/elements.proto line 242 at r1 (raw file):
// TODO(postamar): remove this when we can have more expressive rule defs // See the dep rules for how this is used, and why it's not ideal. bool is_relation_being_dropped = 10;
same with reserving the ID.
190a75a to
056c5d3
Compare
ajwerner
left a comment
There was a problem hiding this comment.
Good changes! Thanks for pushing on this stuff!
| test | ||
| ALTER TABLE db.public.tbl ADD COLUMN m INT NOT NULL UNIQUE DEFAULT nextval('db.public.sq1') |
There was a problem hiding this comment.
Honestly I'm starting to think that every new test case should go in a new file. What do you think?
| ## PostCommitPhase stage 11 of 13 with 1 BackfillType op | ||
| merge temporary indexes [11] into backfilled indexes [10] in table #106 | ||
| commit transaction #13 | ||
| begin transaction #14 | ||
| ## PostCommitPhase stage 12 of 13 with 1 ValidationType op |
There was a problem hiding this comment.
aside that's irrelevant to this change: we should probably update the statuses in the descriptors between backfill and validation. I think the backfill progress will mean that we don't backfill again, but something is somewhat unsatisfying about going from MERGE_ONLY to PUBLIC for me.
| + - ABSENT | ||
| - PUBLIC | ||
| - PUBLIC | ||
| - - WRITE_ONLY |
There was a problem hiding this comment.
I think this column is becoming public too early. It shouldn't become public until the index has been fully added. We don't want the logical side-effects of the schema change to leak if we're going to roll back. If you don't handle that with new rules here, please file an issue.
| - i | ||
| - name: crdb_internal_index_3_name_placeholder | ||
| + name: crdb_internal_index_1_name_placeholder | ||
| + name: tbl_pkey |
There was a problem hiding this comment.
this is a problem, no?
Doesn't this mean that at this point, we have two indexes/constraints with this name?
Three dependency rules were found to be problematic:
1. "dependents removed after index no longer public": this one
turns out to have its logic "backwards" and we fixed it;
2. "column type removed right before column when not dropping
relation": this dep rule was never used; it was removed.
3. "partial predicate removed right before secondary index when not
dropping relation": similarly, this rule was never used; it
was removed.
Consequently to the removal of 2 and 3 above, we can also remove the
some peripheral codes used by 2 and 3.
Release note: None
056c5d3 to
623720e
Compare
Xiang-Gu
left a comment
There was a problem hiding this comment.
Thank you! I've addressed your comments and RFAL!
Reviewable status:
pkg/sql/schemachanger/scpb/elements.proto line 171 at r1 (raw file):
Previously, ajwerner wrote…
reserve the ID instead of just deleting it:
reserved 10;
done
pkg/sql/schemachanger/scpb/elements.proto line 242 at r1 (raw file):
Previously, ajwerner wrote…
same with reserving the ID.
done
pkg/sql/schemachanger/testdata/alter_table_add_column line 513 at r2 (raw file):
Previously, ajwerner wrote…
this is a problem, no?
Doesn't this mean that at this point, we have two indexes/constraints with this name?
right. We fixed a buggy dep rule but that buggy dep rule hides a defect in MakeDroppedPrimaryIndexDeleteAndWriteOnly. That defect only exhibit itself when the buggy dep rule is removed.
Thank you for your explanation and help on this.
pkg/sql/schemachanger/testdata/alter_table_add_column line 2669 at r2 (raw file):
Previously, ajwerner wrote…
Honestly I'm starting to think that every new test case should go in a new file. What do you think?
I agree. Let me do it in a follow-up PR!
pkg/sql/schemachanger/testdata/alter_table_add_column line 3294 at r2 (raw file):
Previously, ajwerner wrote…
I think this column is becoming public too early. It shouldn't become public until the index has been fully added. We don't want the logical side-effects of the schema change to leak if we're going to roll back. If you don't handle that with new rules here, please file an issue.
thanks for catching this. I filed #82953 for this.
pkg/sql/schemachanger/testdata/alter_table_add_column line 3590 at r2 (raw file):
Previously, ajwerner wrote…
aside that's irrelevant to this change: we should probably update the statuses in the descriptors between backfill and validation. I think the backfill progress will mean that we don't backfill again, but something is somewhat unsatisfying about going from MERGE_ONLY to PUBLIC for me.
Can you elaborate on this? I thought the next staus after MERGE_ONLY is MERGED, not PUBLIC?
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 13 of 22 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status:
pkg/sql/schemachanger/testdata/alter_table_add_column line 3590 at r2 (raw file):
Previously, Xiang-Gu (Xiang Gu) wrote…
Can you elaborate on this? I thought the next staus after
MERGE_ONLYisMERGED, notPUBLIC?
Right, do you see a place where we write the status as MERGED? It goes MERGE_ONLY-[backfill]->MERGED-[validate]->VALIDATED->PUBLIC. There's no mutation stages to record the fact that the index has changed state.
|
while working on this PR, another blocking bug was discovered and described in #83018 |
|
I agree that "dependents removed after index no longer public" needs to be fixed but I'm not convinced that "column type removed right before column when not dropping relation" and "partial predicate removed right before secondary index when not dropping relation" should be removed. To me, the reason they're seemingly never used is that we don't implement DROP COLUMN or DROP INDEX yet. But, when we do, we need to update type references whenever removing a column with a user-defined type or a type reference in the compute expression, and whenever removing a partial index with a type reference in the partial predicate expression. In fact, I checked out your DROP INDEX implementation branch to validate this by doing and I expected to see this "partial predicate removed right before secondary index when not dropping relation" rule show up but the output was bizarrely wrong: We see that The element is subject to |
There was a problem hiding this comment.
@postamar Thank you for providing your review. Here are some responses:
-
my branch of
DROP INDEXis slightly outdated in that it usesdropElement(b,e)but it really should useb.Drop(e). I've updated this. -
The "dependents removed after index no longer public" rule is wrong in that the 'from' and 'to' nodes are written "backwards". A fix is not present on the
DROP INDEXbranch though, because I want to make independent, smaller changes that focus on cleaning up dep rules, and hence this PR. -
So, if you pull my
DROP INDEXbranch, and manually fix the "dependents removed after index no longer public" dep rule. You will see exactly what you expected -- the "dependents removed after index no longer public" rule contradicts with "partial predicate removed right before secondary index when not dropping relation" and thus failed to plan the stages. Here is the dep graph.
- Greate, then if we go ahead and further remove the other two rules -- "column type removed right before column when not dropping relation" and "partial predicate removed right before secondary index when not dropping relation" -- and run it against the example you gave above. It succeeded this time and here is the output:
• Schema change plan for DROP INDEX ‹defaultdb›.public.‹t›@‹p›;
│
├── • StatementPhase
│ │
│ └── • Stage 1 of 1 in StatementPhase
│ │
│ ├── • 3 elements transitioning toward ABSENT
│ │ │
│ │ ├── • SecondaryIndexPartial:{DescID: 104, IndexID: 2}
│ │ │ │ PUBLIC → ABSENT
│ │ │ │
│ │ │ └── • SameStagePrecedence dependency from VALIDATED SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0}
│ │ │ rule: "dependents removed after index no longer public"
│ │ │
│ │ ├── • SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0}
│ │ │ PUBLIC → VALIDATED
│ │ │
│ │ └── • IndexName:{DescID: 104, Name: p, IndexID: 2}
│ │ │ PUBLIC → ABSENT
│ │ │
│ │ └── • SameStagePrecedence dependency from VALIDATED SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0}
│ │ rule: "dependents removed after index no longer public"
│ │
│ └── • 4 Mutation operations
│ │
│ ├── • MakeDroppedNonPrimaryIndexDeleteAndWriteOnly
│ │ IndexID: 2
│ │ TableID: 104
│ │
│ ├── • SetIndexName
│ │ IndexID: 2
│ │ Name: crdb_internal_index_2_name_placeholder
│ │ TableID: 104
│ │
│ ├── • RemoveDroppedIndexPartialPredicate
│ │ IndexID: 2
│ │ TableID: 104
│ │
│ └── • UpdateTableBackReferencesInTypes
│ BackReferencedTableID: 104
│ TypeIDs:
│ - 105
│ - 106
│
├── • PreCommitPhase
│ │
│ └── • Stage 1 of 1 in PreCommitPhase
│ │
│ └── • 4 Mutation operations
│ │
│ ├── • SetJobStateOnDescriptor
│ │ DescriptorID: 104
│ │ Initialize: true
│ │
│ ├── • SetJobStateOnDescriptor
│ │ DescriptorID: 105
│ │ Initialize: true
│ │
│ ├── • SetJobStateOnDescriptor
│ │ DescriptorID: 106
│ │ Initialize: true
│ │
│ └── • CreateSchemaChangerJob
│ Authorization:
│ AppName: $ cockroach sql
│ UserName: root
│ DescriptorIDs:
│ - 104
│ - 105
│ - 106
│ JobID: 1
│ NonCancelable: true
│ RunningStatus: PostCommitNonRevertiblePhase stage 1 of 2 with 1 MutationType op pending
│ Statements:
│ - statement: DROP INDEX t@p
│ redactedstatement: DROP INDEX ‹defaultdb›.public.‹t›@‹p›
│ statementtag: DROP INDEX
│
└── • PostCommitNonRevertiblePhase
│
├── • Stage 1 of 2 in PostCommitNonRevertiblePhase
│ │
│ ├── • 1 element transitioning toward ABSENT
│ │ │
│ │ └── • SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0}
│ │ VALIDATED → DELETE_ONLY
│ │
│ └── • 5 Mutation operations
│ │
│ ├── • MakeDroppedIndexDeleteOnly
│ │ IndexID: 2
│ │ TableID: 104
│ │
│ ├── • SetJobStateOnDescriptor
│ │ DescriptorID: 104
│ │
│ ├── • SetJobStateOnDescriptor
│ │ DescriptorID: 105
│ │
│ ├── • SetJobStateOnDescriptor
│ │ DescriptorID: 106
│ │
│ └── • UpdateSchemaChangerJob
│ IsNonCancelable: true
│ JobID: 1
│ RunningStatus: PostCommitNonRevertiblePhase stage 2 of 2 with 3 MutationType ops pending
│
└── • Stage 2 of 2 in PostCommitNonRevertiblePhase
│
├── • 1 element transitioning toward ABSENT
│ │
│ └── • SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0}
│ │ DELETE_ONLY → ABSENT
│ │
│ ├── • Precedence dependency from ABSENT SecondaryIndexPartial:{DescID: 104, IndexID: 2}
│ │ rule: "dependents removed before index"
│ │
│ └── • Precedence dependency from ABSENT IndexName:{DescID: 104, Name: p, IndexID: 2}
│ rule: "dependents removed before index"
│
└── • 7 Mutation operations
│
├── • LogEvent
│ Element:
│ SecondaryIndex:
│ embeddedIndex:
│ indexId: 2
│ isCreatedExplicitly: true
│ keyColumnDirections:
│ - ASC
│ keyColumnIds:
│ - 2
│ keySuffixColumnIds:
│ - 1
│ tableId: 104
│ EventBase:
│ Authorization:
│ AppName: $ cockroach sql
│ UserName: root
│ Statement: DROP INDEX ‹defaultdb›.public.‹t›@‹p›
│ StatementTag: DROP INDEX
│ TargetMetadata:
│ SourceElementID: 1
│ SubWorkID: 1
│ TargetStatus: 1
│
├── • CreateGcJobForIndex
│ IndexID: 2
│ StatementForDropJob:
│ Statement: DROP INDEX defaultdb.public.t@p
│ TableID: 104
│
├── • MakeIndexAbsent
│ IndexID: 2
│ TableID: 104
│
├── • RemoveJobStateFromDescriptor
│ DescriptorID: 104
│ JobID: 1
│
├── • RemoveJobStateFromDescriptor
│ DescriptorID: 105
│ JobID: 1
│
├── • RemoveJobStateFromDescriptor
│ DescriptorID: 106
│ JobID: 1
│
└── • UpdateSchemaChangerJob
IsNonCancelable: true
JobID: 1
RunningStatus: all stages completed
It's greate to see that, in the statement phase, as we are transition SecondaryIndexPartial from 'public' to 'absent', we also updated table back-references in types!
- finally, as to "While the index is still valid the partial predicate should definitely still stick around!", you are absolutely right! It should be enforced, not from a dep rule, but from this line at
The fact that SecondaryIndexPartial still transitioned from PUBLIC to ABSENT in the statement phase means there might be a bug in the planner.
If we find and fix that bug, we would have a contradictory rules again. Then we can take a closer look at "dependents removed after index no longer public" and maybe change it -- after all, in certain cases like this one, we do want the dependent (the predicate) to stick around even after index no longer public.
Reviewable status:
postamar
left a comment
There was a problem hiding this comment.
Thanks, this all makes sense. Good catch on the revertibility constraint being ignored. I think I may have an idea of what's wrong here.
Turns out, I don't! Welp. |
|
I'm closing this PR as the changes are subsumed in Andrew's PR. |
Three dependency rules were found to be problematic:
turns out to have its logic "backwards" and we fixed it;
relation": this dep rule was never used; it was removed.
dropping relation": similarly, this rule was never used; it
was removed.
Consequently to the removal of 2 and 3 above, we can also remove the
some peripheral codes used by 2 and 3.
A small step toward a more principled and better-tested dep
rule modeling.
Related: #82902
Release note: None