Skip to content

Commit

Permalink
sql/schemachanger: clean up SequenceOwner elements during restore
Browse files Browse the repository at this point in the history
Previously, when restoring a backup taken in middle of a DROP COLUMN,
where a column had a sequence owner assigned, it was possible for the
backup to be unrestorable. This would happen because the sequence
reference would have been dropped in the plan, but the seqeunce owner
element was still within the state. To address this, this test updates
the rewrite logic to clean up any SequenceOwner elements which have the
referenced sequence already removed.

Fixes: cockroachdb#130778

Release note (bug fix): Addressed a rare bug that could prevent backups
taken during a DROP COLUMN where the column was sequence owner. The
restore would fail with:  "rewriting descriptor ids: missing rewrite for
<id> in SequenceOwner..."
  • Loading branch information
fqazi committed Oct 8, 2024
1 parent 9666a73 commit eec105a
Show file tree
Hide file tree
Showing 14 changed files with 1,493 additions and 0 deletions.
28 changes: 28 additions & 0 deletions pkg/ccl/schemachangerccl/backup_base_generated_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 24 additions & 0 deletions pkg/sql/catalog/rewrite/rewrite.go
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,23 @@ func rewriteSchemaChangerState(
data.SchemaID = rewrite.ParentSchemaID
continue
}

// maybeRemoveMissingReference potentially removes and element based on
// missing references, when it is safe to do so.
maybeRemoveMissingReference := func(element scpb.Element, missingID descpb.ID) (removed bool) {
switch e := element.(type) {
case *scpb.SequenceOwner:
if e.SequenceID == missingID {
state.Targets = append(state.Targets[:i], state.Targets[i+1:]...)
state.CurrentStatuses = append(state.CurrentStatuses[:i], state.CurrentStatuses[i+1:]...)
state.TargetRanks = append(state.TargetRanks[:i], state.TargetRanks[i+1:]...)
i--
return true
}
}
return false
}

if err := screl.WalkDescIDs(t.Element(), func(id *descpb.ID) error {
if *id == descpb.InvalidID {
// Some descriptor ID fields in elements may be deliberately unset.
Expand All @@ -775,6 +792,13 @@ func rewriteSchemaChangerState(
}
rewrite, ok := descriptorRewrites[*id]
if !ok {
// If the reference was removed, no rewrite is needed
// and we are done.
if maybeRemoveMissingReference(t.Element(), *id) {
return nil
}
// Otherwise, something is gravely wrong and the state cannot
// be repaired.
return errors.Errorf("missing rewrite for id %d in %s", *id, screl.ElementString(t.Element()))
}
*id = rewrite.ID
Expand Down
42 changes: 42 additions & 0 deletions pkg/sql/schemachanger/sctest_generated_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
setup
CREATE TABLE t (i INT PRIMARY KEY, j INT, k int);
CREATE SEQUENCE sq1 OWNED BY t.j;
COMMENT ON TABLE t IS 't has a comment';
COMMENT ON COLUMN t.j IS 'j has a comment';
INSERT INTO t VALUES(-1);
INSERT INTO t VALUES(-2);
INSERT INTO t VALUES(-3);
----

stage-exec phase=PostCommitPhase stage=:
INSERT INTO t VALUES($stageKey);
INSERT INTO t VALUES($stageKey + 1);
UPDATE t SET k=$stageKey;
UPDATE t SET k=i;
DELETE FROM t WHERE i=-1;
DELETE FROM t WHERE i=$stageKey;
INSERT INTO t VALUES($stageKey);
INSERT INTO t VALUES(-1);
----

# Each insert will be injected twice per stage, plus 3 injected
# at the start.
stage-query phase=PostCommitPhase stage=:
SELECT count(*)=($successfulStageCount*2)+3 FROM t;
----
true

stage-exec phase=PostCommitNonRevertiblePhase stage=:
INSERT INTO t VALUES($stageKey);
INSERT INTO t VALUES($stageKey + 1);
UPDATE t SET k=$stageKey;
UPDATE t SET k=i;
DELETE FROM t WHERE i=-1;
DELETE FROM t WHERE i=$stageKey;
INSERT INTO t VALUES($stageKey);
INSERT INTO t VALUES(-1);
----

# Each insert will be injected twice per stage, plus 3 injected
# at the start.
stage-query phase=PostCommitNonRevertiblePhase stage=:
SELECT count(*)=($successfulStageCount*2)+3 FROM t;
----
true

test
ALTER TABLE t DROP COLUMN j
----
Loading

0 comments on commit eec105a

Please sign in to comment.