sql/schemachanger: clean up SequenceOwner elements during restore #132202

fqazi · 2024-10-08T21:26:00Z

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: #130778

Release note (bug fix): Addressed a rare bug that could prevent backups taken during a DROP COLUMN operation with a sequence owner from restoring with the error: "rewriting descriptor ids: missing rewrite for in SequenceOwner..."

blathers-crl · 2024-10-08T21:26:04Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

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

cockroach-teamcity · 2024-10-08T21:26:09Z

This change is

Dedej-Bergin · 2024-10-09T17:45:04Z

pkg/sql/catalog/rewrite/rewrite.go line 771 at r1 (raw file):

		}

		// maybeRemoveMissingReference potentially removes and element based on

nitpick: should be 'an' element instead of 'and' element

// maybeRemoveMissingReference potentially removes and element

Dedej-Bergin · 2024-10-09T18:34:05Z

pkg/sql/catalog/rewrite/rewrite.go line 773 at r1 (raw file):

		// 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) {

Not a fan of using the word maybe in this function name. Could we split this up into 2 functions like this:

		missingReferenceIsSequence := func(element scpb.Element, missingID descpb.ID) (removed bool) {
			return reflect.TypeOf(element) == reflect.TypeOf(&scpb.SequenceOwner{}) && 
				element.(*scpb.SequenceOwner).SequenceID == missingID
		}

		removeMissingReference := func(element scpb.Element, missingID descpb.ID) {
			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--
		}

And we could make the if statement below look like this:

				if missingReferenceIsSequence(t.Element(), *id) {
					removeMissingReference(t.Element(), *id)
					// No rewrite is needed and we are done
					return nil
				}

Dedej-Bergin · 2024-10-09T18:35:55Z

Nice fix! Just have some small naming related comments.

Dedej-Bergin · 2024-10-09T19:20:13Z

pkg/sql/catalog/rewrite/rewrite.go line 773 at r1 (raw file):

Previously, Dedej-Bergin (Bergin Dedej) wrote…

Not a fan of using the word maybe in this function name. Could we split this up into 2 functions like this:

		missingReferenceIsSequence := func(element scpb.Element, missingID descpb.ID) (removed bool) {
			return reflect.TypeOf(element) == reflect.TypeOf(&scpb.SequenceOwner{}) && 
				element.(*scpb.SequenceOwner).SequenceID == missingID
		}

		removeMissingReference := func(element scpb.Element, missingID descpb.ID) {
			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--
		}

And we could make the if statement below look like this:

				if missingReferenceIsSequence(t.Element(), *id) {
					removeMissingReference(t.Element(), *id)
					// No rewrite is needed and we are done
					return nil
				}

Alternative:

missingReferenceIsSequence := func(element scpb.Element, missingID descpb.ID) (removed bool) {
				switch e := element.(type) {
				case *scpb.SequenceOwner:
					if e.SequenceID == missingID {
						return true
					}
				}
				return false
			}

Dedej-Bergin

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

fqazi

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @Dedej-Bergin)

pkg/sql/catalog/rewrite/rewrite.go line 773 at r1 (raw file):

Previously, Dedej-Bergin (Bergin Dedej) wrote…

Alternative:

missingReferenceIsSequence := func(element scpb.Element, missingID descpb.ID) (removed bool) {
				switch e := element.(type) {
				case *scpb.SequenceOwner:
					if e.SequenceID == missingID {
						return true
					}
				}
				return false
			}

I simplified this further, so that the switch below is just used. So, its much simpler after that change and I replaced the deletion code with function calls.

fqazi · 2024-10-10T04:22:19Z

@Dedej-Bergin TFTR!

bors r+

132202: sql/schemachanger: clean up SequenceOwner elements during restore r=fqazi a=fqazi 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: #130778 Release note (bug fix): Addressed a rare bug that could prevent backups taken during a DROP COLUMN operation with a sequence owner from restoring with the error: "rewriting descriptor ids: missing rewrite for <id> in SequenceOwner..." Co-authored-by: Faizan Qazi <[email protected]>

craig · 2024-10-10T04:37:56Z

Build failed:

examples_orms

fqazi · 2024-10-10T12:24:58Z

Hit a test flake

bors retry

132166: rac2,kvserver: do not quiesce if send tokens held r=sumeerbhola a=pav-kv This PR prevents range quiescence if RACv2 holds any send tokens for this range. Quiescence would prevent `MsgApp` pings which ensure that the leader reliably learns about the follower store admitting log entries, and causes it to release tokens accordingly. We do not want to end up holding tokens permanently. Resolves #129581 132202: sql/schemachanger: clean up SequenceOwner elements during restore r=fqazi a=fqazi 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: #130778 Release note (bug fix): Addressed a rare bug that could prevent backups taken during a DROP COLUMN operation with a sequence owner from restoring with the error: "rewriting descriptor ids: missing rewrite for <id> in SequenceOwner..." Co-authored-by: Pavel Kalinnikov <[email protected]> Co-authored-by: Faizan Qazi <[email protected]>

craig · 2024-10-10T13:04:43Z

Build failed (retrying...):

unit_tests

Dedej-Bergin

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

pkg/sql/catalog/rewrite/rewrite.go line 773 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

I simplified this further, so that the switch below is just used. So, its much simpler after that change and I replaced the deletion code with function calls.

Nice!

Dedej-Bergin · 2024-10-10T13:16:16Z

bors r+

craig · 2024-10-10T13:16:19Z

Already running a review

132202: sql/schemachanger: clean up SequenceOwner elements during restore r=fqazi a=fqazi 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: #130778 Release note (bug fix): Addressed a rare bug that could prevent backups taken during a DROP COLUMN operation with a sequence owner from restoring with the error: "rewriting descriptor ids: missing rewrite for <id> in SequenceOwner..." 132234: stringer: add license header to stringer-generated files r=rickystewart a=jlinder Some string-generated files did not contain license headers. This change adds license headers to these files. Part of RE-658 Release note: None Co-authored-by: Faizan Qazi <[email protected]> Co-authored-by: James H. Linder <[email protected]>

craig · 2024-10-10T13:53:17Z

Build failed (retrying...):

unit_tests

rafiss · 2024-10-10T14:18:54Z

I believe this will need backports. Have you thought about how far back it should go?

132129: roachtest: add slow disk perturbation test r=kvoli a=andrewbaptist This change adds a new set of perturbation tests perturbation/*/slowDisk which tests slow disks. We have see support cases where slow disks can cause cluster level availability outages. Epic: none Release note: None 132166: rac2,kvserver: do not quiesce if send tokens held r=sumeerbhola a=pav-kv This PR prevents range quiescence if RACv2 holds any send tokens for this range. Quiescence would prevent `MsgApp` pings which ensure that the leader reliably learns about the follower store admitting log entries, and causes it to release tokens accordingly. We do not want to end up holding tokens permanently. Resolves #129581 132202: sql/schemachanger: clean up SequenceOwner elements during restore r=fqazi a=fqazi 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: #130778 Release note (bug fix): Addressed a rare bug that could prevent backups taken during a DROP COLUMN operation with a sequence owner from restoring with the error: "rewriting descriptor ids: missing rewrite for <id> in SequenceOwner..." Co-authored-by: Andrew Baptist <[email protected]> Co-authored-by: Pavel Kalinnikov <[email protected]> Co-authored-by: Faizan Qazi <[email protected]>

rafiss

the new test that was added in this PR failed:

    end_to_end.go:213: 
        	Error Trace:	github.com/cockroachdb/cockroach/pkg/sql/schemachanger/sctest/end_to_end.go:213
        	            				github.com/cockroachdb/cockroach/pkg/sql/schemachanger/sctest/end_to_end.go:164
        	            				github.com/cockroachdb/datadriven/external/com_github_cockroachdb_datadriven/datadriven.go:118
        	            				github.com/cockroachdb/datadriven/external/com_github_cockroachdb_datadriven/datadriven.go:337
        	            				github.com/cockroachdb/datadriven/external/com_github_cockroachdb_datadriven/datadriven.go:342
        	            				github.com/cockroachdb/datadriven/external/com_github_cockroachdb_datadriven/datadriven.go:211
        	            				github.com/cockroachdb/datadriven/external/com_github_cockroachdb_datadriven/datadriven.go:184
        	            				github.com/cockroachdb/datadriven/external/com_github_cockroachdb_datadriven/datadriven.go:144
        	            				github.com/cockroachdb/datadriven/external/com_github_cockroachdb_datadriven/datadriven.go:117
        	            				github.com/cockroachdb/cockroach/pkg/sql/schemachanger/sctest/end_to_end.go:70
        	            				github.com/cockroachdb/cockroach/pkg/sql/schemachanger/sctest/test_server_factory.go:97
        	            				github.com/cockroachdb/cockroach/pkg/sql/schemachanger/sctest/end_to_end.go:65
        	            				pkg/sql/schemachanger/schemachanger_test/bazel-out/k8-fastbuild/bin/pkg/sql/schemachanger/sctest_generated_test.go:414
        	Error:      	Not equal: 
        	            	expected: "/* setup */\nCREATE TABLE t (i INT PRIMARY KEY, j INT, k int);\nCREATE SEQUENCE sq1 OWNED BY t.j;\nCOMMENT ON TABLE t IS 't has a comment';\nCOMMENT ON COLUMN t.j IS 'j has a comment';\nINSERT INTO t VALUES(-1);\nINSERT INTO t VALUES(-2);\nINSERT INTO t VALUES(-3);\n----\n...\n+object {100 101 t} -> 104\n+object {100 101 sq1} -> 105\n\n/* test */\nALTER TABLE t DROP COLUMN j;\n----\nbegin transaction #1\n# begin StatementPhase\nchecking for feature: ALTER TABLE\nincrement telemetry for sql.schema.alter_table\nincrement telemetry for sql.schema.alter_table.drop_column\nwrite *eventpb.AlterTable to event log:\n  mutationId: 1\n  sql:\n    descriptorId: 104\n    statement: ALTER TABLE ‹defaultdb›.‹public›.‹t› DROP COLUMN ‹j›\n    tag: ALTER TABLE\n    user: root\n  tableName: defaultdb.public.t\n## StatementPhase stage 1 of 1 with 9 MutationType ops\nupsert descriptor #104\n  ...\n         oid: 20\n         width: 64\n  -  - id: 2\n  -    name: j\n  -    nullable: true\n  -    ownsSequenceIds:\n  -    - 105\n  -    type:\n  -      family: IntFamily\n  -      oid: 20\n  -      width: 64\n     - id: 3\n       name: k\n  ...\n       columnNames:\n       - i\n  -    - j\n  +    - crdb_internal_column_2_name_placeholder\n       - k\n       name: primary\n  ...\n     id: 104\n     modificationTime: {}\n  +  mutations:\n  +  - column:\n  +      id: 2\n  +      name: crdb_internal_column_2_name_placeholder\n  +      nullable: true\n  +      ownsSequenceIds:\n  +      - 105\n  +      type:\n  +        family: IntFamily\n  +        oid: 20\n  +        width: 64\n  +    direction: DROP\n  +    mutationId: 1\n  +    state: WRITE_ONLY\n  +  - direction: ADD\n  +    index:\n  +      constraintId: 2\n  +      createdExplicitly: true\n  +      encodingType: 1\n  +      foreignKey: {}\n  +      geoConfig: {}\n  +      id: 2\n  +      interleave: {}\n  +      keyColumnDirections:\n  +      - ASC\n  +      keyColumnIds:\n  +      - 1\n  +      keyColumnNames:\n  +      - i\n  +      name: crdb_internal_index_2_name_placeholder\n  +      partitioning: {}\n  +      sharded: {}\n  +      storeColumnIds:\n  +      - 3\n  +      storeColumnNames:\n  +      - k\n  +      unique: true\n  +      version: 4\n  +    mutationId: 1\n  +    state: BACKFILLING\n  +  - direction: ADD\n  +    index:\n  +      constraintId: 3\n  +      createdExplicitly: true\n  +      encodingType: 1\n  +      foreignKey: {}\n  +      geoConfig: {}\n  +      id: 3\n  +      interleave: {}\n  +      keyColumnDirections:\n  +      - ASC\n  +      keyColumnIds:\n  +      - 1\n  +      keyColumnNames:\n  +      - i\n  +      name: crdb_internal_index_3_name_placeholder\n  +      partitioning: {}\n  +      sharded: {}\n  +      storeColumnIds:\n  +      - 3\n  +      storeColumnNames:\n  +      - k\n  +      unique: true\n  +      useDeletePreservingEncoding: true\n  +      version: 4\n  +    mutationId: 1\n  +    state: DELETE_ONLY\n     name: t\n     nextColumnId: 4\n  -  nextConstraintId: 2\n  +  nextConstraintId: 4\n     nextFamilyId: 1\n  -  nextIndexId: 2\n  +  nextIndexId: 4\n     nextMutationId: 1\n     parentId: 100\n  ...\n       - 3\n       storeColumnNames:\n  -    - j\n  +    - crdb_internal_column_2_name_placeholder\n       - k\n       unique: true\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"2\"\n  +  version: \"3\"\ndelete comment ColumnCommentType(objID: 104, subID: 2)\n# end StatementPhase\n# begin PreCommitPhase\n## PreCommitPhase stage 1 of 2 with 1 MutationType op\nundo all catalog changes within txn #1\npersist all catalog changes to storage\n## PreCommitPhase stage 2 of 2 with 14 MutationType ops\nupsert descriptor #104\n  ...\n         oid: 20\n         width: 64\n  -  - id: 2\n  -    name: j\n  -    nullable: true\n  -    ownsSequenceIds:\n  -    - 105\n  -    type:\n  -      family: IntFamily\n  -      oid: 20\n  -      width: 64\n     - id: 3\n       name: k\n  ...\n     createAsOfTime:\n       wallTime: \"1640995200000000000\"\n  +  declarativeSchemaChangerState:\n  +    authorization:\n  +      userName: root\n  +    currentStatuses: <redacted>\n  +    jobId: \"1\"\n  +    nameMapping:\n  +      columns:\n  +        \"1\": i\n  +        \"3\": k\n  +        \"4294967294\": tableoid\n  +        \"4294967295\": crdb_internal_mvcc_timestamp\n  +      families:\n  +        \"0\": primary\n  +      id: 104\n  +      indexes:\n  +        \"2\": t_pkey\n  +      name: t\n  +    relevantStatements:\n  +    - statement:\n  +        redactedStatement: ALTER TABLE ‹defaultdb›.‹public›.‹t› DROP COLUMN ‹j›\n  +        statement: ALTER TABLE t DROP COLUMN j\n  +        statementTag: ALTER TABLE\n  +    revertible: true\n  +    targetRanks: <redacted>\n  +    targets: <redacted>\n     families:\n     - columnIds:\n  ...\n       columnNames:\n       - i\n  -    - j\n  +    - crdb_internal_column_2_name_placeholder\n       - k\n       name: primary\n  ...\n     id: 104\n     modificationTime: {}\n  +  mutations:\n  +  - column:\n  +      id: 2\n  +      name: crdb_internal_column_2_name_placeholder\n  +      nullable: true\n  +      ownsSequenceIds:\n  +      - 105\n  +      type:\n  +        family: IntFamily\n  +        oid: 20\n  +        width: 64\n  +    direction: DROP\n  +    mutationId: 1\n  +    state: WRITE_ONLY\n  +  - direction: ADD\n  +    index:\n  +      constraintId: 2\n  +      createdExplicitly: true\n  +      encodingType: 1\n  +      foreignKey: {}\n  +      geoConfig: {}\n  +      id: 2\n  +      interleave: {}\n  +      keyColumnDirections:\n  +      - ASC\n  +      keyColumnIds:\n  +      - 1\n  +      keyColumnNames:\n  +      - i\n  +      name: crdb_internal_index_2_name_placeholder\n  +      partitioning: {}\n  +      sharded: {}\n  +      storeColumnIds:\n  +      - 3\n  +      storeColumnNames:\n  +      - k\n  +      unique: true\n  +      version: 4\n  +    mutationId: 1\n  +    state: BACKFILLING\n  +  - direction: ADD\n  +    index:\n  +      constraintId: 3\n  +      createdExplicitly: true\n  +      encodingType: 1\n  +      foreignKey: {}\n  +      geoConfig: {}\n  +      id: 3\n  +      interleave: {}\n  +      keyColumnDirections:\n  +      - ASC\n  +      keyColumnIds:\n  +      - 1\n  +      keyColumnNames:\n  +      - i\n  +      name: crdb_internal_index_3_name_placeholder\n  +      partitioning: {}\n  +      sharded: {}\n  +      storeColumnIds:\n  +      - 3\n  +      storeColumnNames:\n  +      - k\n  +      unique: true\n  +      useDeletePreservingEncoding: true\n  +      version: 4\n  +    mutationId: 1\n  +    state: DELETE_ONLY\n     name: t\n     nextColumnId: 4\n  -  nextConstraintId: 2\n  +  nextConstraintId: 4\n     nextFamilyId: 1\n  -  nextIndexId: 2\n  +  nextIndexId: 4\n     nextMutationId: 1\n     parentId: 100\n  ...\n       - 3\n       storeColumnNames:\n  -    - j\n  +    - crdb_internal_column_2_name_placeholder\n       - k\n       unique: true\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"2\"\n  +  version: \"3\"\nupsert descriptor #105\n  ...\n     createAsOfTime:\n       wallTime: \"1640995200000000000\"\n  +  declarativeSchemaChangerState:\n  +    authorization:\n  +      userName: root\n  +    currentStatuses: <redacted>\n  +    jobId: \"1\"\n  +    nameMapping:\n  +      id: 105\n  +      name: sq1\n  +    relevantStatements:\n  +    - statement:\n  +        redactedStatement: ALTER TABLE ‹defaultdb›.‹public›.‹t› DROP COLUMN ‹j›\n  +        statement: ALTER TABLE t DROP COLUMN j\n  +        statementTag: ALTER TABLE\n  +    revertible: true\n  +    targetRanks: <redacted>\n  +    targets: <redacted>\n     formatVersion: 3\n     id: 105\n  ...\n       start: \"1\"\n     unexposedParentSchemaId: 101\n  -  version: \"1\"\n  +  version: \"2\"\ndelete comment ColumnCommentType(objID: 104, subID: 2)\npersist all catalog changes to storage\ncreate job #1 (non-cancelable: false): \"ALTER TABLE defaultdb.public.t DROP COLUMN j\"\n  descriptor IDs: [104 105]\n# end PreCommitPhase\ncommit transaction #1\nnotified job registry to adopt jobs: [1]\n# begin PostCommitPhase\nbegin transaction #2\ncommit transaction #2\nbegin transaction #3\n## PostCommitPhase stage 1 of 7 with 4 MutationType ops\nupsert descriptor #104\n  ...\n         version: 4\n       mutationId: 1\n  -    state: DELETE_ONLY\n  +    state: WRITE_ONLY\n     name: t\n     nextColumnId: 4\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"3\"\n  +  version: \"4\"\nupsert descriptor #105\n  ...\n       start: \"1\"\n     unexposedParentSchemaId: 101\n  -  version: \"2\"\n  +  version: \"3\"\npersist all catalog changes to storage\nupdate progress of schema change job #1: \"PostCommitPhase stage 2 of 7 with 1 BackfillType op pending\"\ncommit transaction #3\nbegin transaction #4\n## PostCommitPhase stage 2 of 7 with 1 BackfillType op\nbackfill indexes [2] from index #1 in table #104\ncommit transaction #4\nbegin transaction #5\n## PostCommitPhase stage 3 of 7 with 4 MutationType ops\nupsert descriptor #104\n  ...\n         version: 4\n       mutationId: 1\n  -    state: BACKFILLING\n  +    state: DELETE_ONLY\n     - direction: ADD\n       index:\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"4\"\n  +  version: \"5\"\nupsert descriptor #105\n  ...\n       start: \"1\"\n     unexposedParentSchemaId: 101\n  -  version: \"3\"\n  +  version: \"4\"\npersist all catalog changes to storage\nupdate progress of schema change job #1: \"PostCommitPhase stage 4 of 7 with 1 MutationType op pending\"\ncommit transaction #5\nbegin transaction #6\n## PostCommitPhase stage 4 of 7 with 4 MutationType ops\nupsert descriptor #104\n  ...\n         version: 4\n       mutationId: 1\n  -    state: DELETE_ONLY\n  +    state: MERGING\n     - direction: ADD\n       index:\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"5\"\n  +  version: \"6\"\nupsert descriptor #105\n  ...\n       start: \"1\"\n     unexposedParentSchemaId: 101\n  -  version: \"4\"\n  +  version: \"5\"\npersist all catalog changes to storage\nupdate progress of schema change job #1: \"PostCommitPhase stage 5 of 7 with 1 BackfillType op pending\"\ncommit transaction #6\nbegin transaction #7\n## PostCommitPhase stage 5 of 7 with 1 BackfillType op\nmerge temporary indexes [3] into backfilled indexes [2] in table #104\ncommit transaction #7\nbegin transaction #8\n## PostCommitPhase stage 6 of 7 with 5 MutationType ops\nupsert descriptor #104\n  ...\n         version: 4\n       mutationId: 1\n  -    state: MERGING\n  -  - direction: ADD\n  +    state: WRITE_ONLY\n  +  - direction: DROP\n       index:\n         constraintId: 3\n  ...\n         version: 4\n       mutationId: 1\n  -    state: WRITE_ONLY\n  +    state: DELETE_ONLY\n     name: t\n     nextColumnId: 4\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"6\"\n  +  version: \"7\"\nupsert descriptor #105\n  ...\n       start: \"1\"\n     unexposedParentSchemaId: 101\n  -  version: \"5\"\n  +  version: \"6\"\npersist all catalog changes to storage\nupdate progress of schema change job #1: \"PostCommitPhase stage 7 of 7 with 1 ValidationType op pending\"\ncommit transaction #8\nbegin transaction #9\n## PostCommitPhase stage 7 of 7 with 1 ValidationType op\nvalidate forward indexes [2] in table #104\ncommit transaction #9\nbegin transaction #10\n## PostCommitNonRevertiblePhase stage 1 of 4 with 11 MutationType ops\nupsert descriptor #104\n  ...\n           statement: ALTER TABLE t DROP COLUMN j\n           statementTag: ALTER TABLE\n  -    revertible: true\n       targetRanks: <redacted>\n       targets: <redacted>\n  ...\n       direction: DROP\n       mutationId: 1\n  -    state: WRITE_ONLY\n  -  - direction: ADD\n  -    index:\n  -      constraintId: 2\n  -      createdExplicitly: true\n  -      encodingType: 1\n  -      foreignKey: {}\n  -      geoConfig: {}\n  -      id: 2\n  -      interleave: {}\n  -      keyColumnDirections:\n  -      - ASC\n  -      keyColumnIds:\n  -      - 1\n  -      keyColumnNames:\n  -      - i\n  -      name: crdb_internal_index_2_name_placeholder\n  -      partitioning: {}\n  -      sharded: {}\n  -      storeColumnIds:\n  -      - 3\n  -      storeColumnNames:\n  -      - k\n  -      unique: true\n  -      version: 4\n  -    mutationId: 1\n  -    state: WRITE_ONLY\n  +    state: DELETE_ONLY\n     - direction: DROP\n       index:\n  -      constraintId: 3\n  -      createdExplicitly: true\n  +      constraintId: 1\n  +      createdAtNanos: \"1640995200000000000\"\n         encodingType: 1\n         foreignKey: {}\n         geoConfig: {}\n  -      id: 3\n  +      id: 1\n         interleave: {}\n         keyColumnDirections:\n  ...\n         keyColumnNames:\n         - i\n  -      name: crdb_internal_index_3_name_placeholder\n  +      name: crdb_internal_index_1_name_placeholder\n         partitioning: {}\n         sharded: {}\n         storeColumnIds:\n  +      - 2\n         - 3\n         storeColumnNames:\n  +      - crdb_internal_column_2_name_placeholder\n         - k\n         unique: true\n  -      useDeletePreservingEncoding: true\n         version: 4\n       mutationId: 1\n  -    state: DELETE_ONLY\n  +    state: WRITE_ONLY\n     name: t\n     nextColumnId: 4\n  ...\n     parentId: 100\n     primaryIndex:\n  -    constraintId: 1\n  -    createdAtNanos: \"1640995200000000000\"\n  +    constraintId: 2\n  +    createdExplicitly: true\n       encodingType: 1\n       foreignKey: {}\n       geoConfig: {}\n  -    id: 1\n  +    id: 2\n       interleave: {}\n       keyColumnDirections:\n  ...\n       sharded: {}\n       storeColumnIds:\n  -    - 2\n       - 3\n       storeColumnNames:\n  -    - crdb_internal_column_2_name_placeholder\n       - k\n       unique: true\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"7\"\n  +  version: \"8\"\nupsert descriptor #105\n  ...\n           statement: ALTER TABLE t DROP COLUMN j\n           statementTag: ALTER TABLE\n  -    revertible: true\n       targetRanks: <redacted>\n       targets: <redacted>\n  ...\n       start: \"1\"\n     unexposedParentSchemaId: 101\n  -  version: \"6\"\n  +  version: \"7\"\npersist all catalog changes to storage\nupdate progress of schema change job #1: \"PostCommitNonRevertiblePhase stage 2 of 4 with 4 MutationType ops pending\"\nset schema change job #1 to non-cancellable\ncommit transaction #10\nbegin transaction #11\n## PostCommitNonRevertiblePhase stage 2 of 4 with 7 MutationType ops\nupsert descriptor #104\n  ...\n         version: 4\n       mutationId: 1\n  -    state: WRITE_ONLY\n  +    state: DELETE_ONLY\n     name: t\n     nextColumnId: 4\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"8\"\n  +  version: \"9\"\nupsert descriptor #105\n  ...\n       start: \"1\"\n     unexposedParentSchemaId: 101\n  -  version: \"7\"\n  +  version: \"8\"\npersist all catalog changes to storage\nupdate progress of schema change job #1: \"PostCommitNonRevertiblePhase stage 3 of 4 with 12 MutationType ops pending\"\ncommit transaction #11\nbegin transaction #12\n## PostCommitNonRevertiblePhase stage 3 of 4 with 15 MutationType ops\ndelete object namespace entry {100 101 sq1} -> 105\nupsert descriptor #104\n  ...\n     - columnIds:\n       - 1\n  -    - 2\n       - 3\n       columnNames:\n       - i\n  -    - crdb_internal_column_2_name_placeholder\n       - k\n       name: primary\n  ...\n     id: 104\n     modificationTime: {}\n  -  mutations:\n  -  - column:\n  -      id: 2\n  -      name: crdb_internal_column_2_name_placeholder\n  -      nullable: true\n  -      ownsSequenceIds:\n  -      - 105\n  -      type:\n  -        family: IntFamily\n  -        oid: 20\n  -        width: 64\n  -    direction: DROP\n  -    mutationId: 1\n  -    state: DELETE_ONLY\n  -  - direction: DROP\n  -    index:\n  -      constraintId: 1\n  -      createdAtNanos: \"1640995200000000000\"\n  -      encodingType: 1\n  -      foreignKey: {}\n  -      geoConfig: {}\n  -      id: 1\n  -      interleave: {}\n  -      keyColumnDirections:\n  -      - ASC\n  -      keyColumnIds:\n  -      - 1\n  -      keyColumnNames:\n  -      - i\n  -      name: crdb_internal_index_1_name_placeholder\n  -      partitioning: {}\n  -      sharded: {}\n  -      storeColumnIds:\n  -      - 2\n  -      - 3\n  -      storeColumnNames:\n  -      - crdb_internal_column_2_name_placeholder\n  -      - k\n  -      unique: true\n  -      version: 4\n  -    mutationId: 1\n  -    state: DELETE_ONLY\n  +  mutations: []\n     name: t\n     nextColumnId: 4\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"9\"\n  +  version: \"10\"\nupsert descriptor #105\n  ...\n         ownerTableId: 104\n       start: \"1\"\n  +  state: DROP\n     unexposedParentSchemaId: 101\n  -  version: \"8\"\n  +  version: \"9\"\npersist all catalog changes to storage\ncreate job #2 (non-cancelable: true): \"GC for ALTER TABLE defaultdb.public.t DROP COLUMN j\"\n  descriptor IDs: [104]\nupdate progress of schema change job #1: \"PostCommitNonRevertiblePhase stage 4 of 4 with 1 MutationType op pending\"\ncommit transaction #12\nnotified job registry to adopt jobs: [2]\nbegin transaction #13\n## PostCommitNonRevertiblePhase stage 4 of 4 with 4 MutationType ops\nupsert descriptor #104\n  ...\n     createAsOfTime:\n       wallTime: \"1640995200000000000\"\n  -  declarativeSchemaChangerState:\n  -    authorization:\n  -      userName: root\n  -    currentStatuses: <redacted>\n  -    jobId: \"1\"\n  -    nameMapping:\n  -      columns:\n  -        \"1\": i\n  -        \"3\": k\n  -        \"4294967294\": tableoid\n  -        \"4294967295\": crdb_internal_mvcc_timestamp\n  -      families:\n  -        \"0\": primary\n  -      id: 104\n  -      indexes:\n  -        \"2\": t_pkey\n  -      name: t\n  -    relevantStatements:\n  -    - statement:\n  -        redactedStatement: ALTER TABLE ‹defaultdb›.‹public›.‹t› DROP COLUMN ‹j›\n  -        statement: ALTER TABLE t DROP COLUMN j\n  -        statementTag: ALTER TABLE\n  -    targetRanks: <redacted>\n  -    targets: <redacted>\n     families:\n     - columnIds:\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"10\"\n  +  version: \"11\"\nupsert descriptor #105\n  ...\n     createAsOfTime:\n       wallTime: \"1640995200000000000\"\n  -  declarativeSchemaChangerState:\n  -    authorization:\n  -      userName: root\n  -    currentStatuses: <redacted>\n  -    jobId: \"1\"\n  -    nameMapping:\n  -      id: 105\n  -      name: sq1\n  -    relevantStatements:\n  -    - statement:\n  -        redactedStatement: ALTER TABLE ‹defaultdb›.‹public›.‹t› DROP COLUMN ‹j›\n  -        statement: ALTER TABLE t DROP COLUMN j\n  -        statementTag: ALTER TABLE\n  -    targetRanks: <redacted>\n  -    targets: <redacted>\n     formatVersion: 3\n     id: 105\n  ...\n     state: DROP\n     unexposedParentSchemaId: 101\n  -  version: \"9\"\n  +  version: \"10\"\npersist all catalog changes to storage\ncreate job #3 (non-cancelable: true): \"GC for ALTER TABLE defaultdb.public.t DROP COLUMN j\"\n  descriptor IDs: [105]\nupdate progress of schema change job #1: \"all stages completed\"\nset schema change job #1 to non-cancellable\nupdated schema change job #1 descriptor IDs to []\nwrite *eventpb.FinishSchemaChange to event log:\n  sc:\n    descriptorId: 104\nwrite *eventpb.FinishSchemaChange to event log:\n  sc:\n    descriptorId: 105\ncommit transaction #13\nnotified job registry to adopt jobs: [3]\n# end PostCommitPhase\n"
        	            	actual  : "/* setup */\nCREATE TABLE t (i INT PRIMARY KEY, j INT, k int);\nCREATE SEQUENCE sq1 OWNED BY t.j;\nCOMMENT ON TABLE t IS 't has a comment';\nCOMMENT ON COLUMN t.j IS 'j has a comment';\nINSERT INTO t VALUES(-1);\nINSERT INTO t VALUES(-2);\nINSERT INTO t VALUES(-3);\n----\n...\n+object {100 101 t} -> 104\n+object {100 101 sq1} -> 105\n\n/* test */\nALTER TABLE t DROP COLUMN j;\n----\nbegin transaction #1\n# begin StatementPhase\nchecking for feature: ALTER TABLE\nincrement telemetry for sql.schema.alter_table\nincrement telemetry for sql.schema.alter_table.drop_column\nwrite *eventpb.AlterTable to event log:\n  mutationId: 1\n  sql:\n    descriptorId: 104\n    statement: ALTER TABLE ‹defaultdb›.‹public›.‹t› DROP COLUMN ‹j›\n    tag: ALTER TABLE\n    user: root\n  tableName: defaultdb.public.t\n## StatementPhase stage 1 of 1 with 9 MutationType ops\nupsert descriptor #104\n  ...\n         oid: 20\n         width: 64\n  -  - id: 2\n  -    name: j\n  -    nullable: true\n  -    ownsSequenceIds:\n  -    - 105\n  -    type:\n  -      family: IntFamily\n  -      oid: 20\n  -      width: 64\n     - id: 3\n       name: k\n  ...\n       columnNames:\n       - i\n  -    - j\n  +    - crdb_internal_column_2_name_placeholder\n       - k\n       name: primary\n  ...\n     id: 104\n     modificationTime: {}\n  +  mutations:\n  +  - column:\n  +      id: 2\n  +      name: crdb_internal_column_2_name_placeholder\n  +      nullable: true\n  +      ownsSequenceIds:\n  +      - 105\n  +      type:\n  +        family: IntFamily\n  +        oid: 20\n  +        width: 64\n  +    direction: DROP\n  +    mutationId: 1\n  +    state: WRITE_ONLY\n  +  - direction: ADD\n  +    index:\n  +      constraintId: 2\n  +      createdExplicitly: true\n  +      encodingType: 1\n  +      foreignKey: {}\n  +      geoConfig: {}\n  +      id: 2\n  +      interleave: {}\n  +      keyColumnDirections:\n  +      - ASC\n  +      keyColumnIds:\n  +      - 1\n  +      keyColumnNames:\n  +      - i\n  +      name: crdb_internal_index_2_name_placeholder\n  +      partitioning: {}\n  +      sharded: {}\n  +      storeColumnIds:\n  +      - 3\n  +      storeColumnNames:\n  +      - k\n  +      unique: true\n  +      version: 4\n  +    mutationId: 1\n  +    state: BACKFILLING\n  +  - direction: ADD\n  +    index:\n  +      constraintId: 3\n  +      createdExplicitly: true\n  +      encodingType: 1\n  +      foreignKey: {}\n  +      geoConfig: {}\n  +      id: 3\n  +      interleave: {}\n  +      keyColumnDirections:\n  +      - ASC\n  +      keyColumnIds:\n  +      - 1\n  +      keyColumnNames:\n  +      - i\n  +      name: crdb_internal_index_3_name_placeholder\n  +      partitioning: {}\n  +      sharded: {}\n  +      storeColumnIds:\n  +      - 3\n  +      storeColumnNames:\n  +      - k\n  +      unique: true\n  +      useDeletePreservingEncoding: true\n  +      version: 4\n  +    mutationId: 1\n  +    state: DELETE_ONLY\n     name: t\n     nextColumnId: 4\n  -  nextConstraintId: 2\n  +  nextConstraintId: 4\n     nextFamilyId: 1\n  -  nextIndexId: 2\n  +  nextIndexId: 4\n     nextMutationId: 1\n     parentId: 100\n  ...\n       - 3\n       storeColumnNames:\n  -    - j\n  +    - crdb_internal_column_2_name_placeholder\n       - k\n       unique: true\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"2\"\n  +  version: \"3\"\ndelete comment ColumnCommentType(objID: 104, subID: 2)\n# end StatementPhase\n# begin PreCommitPhase\n## PreCommitPhase stage 1 of 2 with 1 MutationType op\nundo all catalog changes within txn #1\npersist all catalog changes to storage\n## PreCommitPhase stage 2 of 2 with 14 MutationType ops\nupsert descriptor #104\n  ...\n         oid: 20\n         width: 64\n  -  - id: 2\n  -    name: j\n  -    nullable: true\n  -    ownsSequenceIds:\n  -    - 105\n  -    type:\n  -      family: IntFamily\n  -      oid: 20\n  -      width: 64\n     - id: 3\n       name: k\n  ...\n     createAsOfTime:\n       wallTime: \"1640995200000000000\"\n  +  declarativeSchemaChangerState:\n  +    authorization:\n  +      userName: root\n  +    currentStatuses: <redacted>\n  +    jobId: \"1\"\n  +    nameMapping:\n  +      columns:\n  +        \"1\": i\n  +        \"3\": k\n  +        \"4294967292\": crdb_internal_origin_timestamp\n  +        \"4294967293\": crdb_internal_origin_id\n  +        \"4294967294\": tableoid\n  +        \"4294967295\": crdb_internal_mvcc_timestamp\n  +      families:\n  +        \"0\": primary\n  +      id: 104\n  +      indexes:\n  +        \"2\": t_pkey\n  +      name: t\n  +    relevantStatements:\n  +    - statement:\n  +        redactedStatement: ALTER TABLE ‹defaultdb›.‹public›.‹t› DROP COLUMN ‹j›\n  +        statement: ALTER TABLE t DROP COLUMN j\n  +        statementTag: ALTER TABLE\n  +    revertible: true\n  +    targetRanks: <redacted>\n  +    targets: <redacted>\n     families:\n     - columnIds:\n  ...\n       columnNames:\n       - i\n  -    - j\n  +    - crdb_internal_column_2_name_placeholder\n       - k\n       name: primary\n  ...\n     id: 104\n     modificationTime: {}\n  +  mutations:\n  +  - column:\n  +      id: 2\n  +      name: crdb_internal_column_2_name_placeholder\n  +      nullable: true\n  +      ownsSequenceIds:\n  +      - 105\n  +      type:\n  +        family: IntFamily\n  +        oid: 20\n  +        width: 64\n  +    direction: DROP\n  +    mutationId: 1\n  +    state: WRITE_ONLY\n  +  - direction: ADD\n  +    index:\n  +      constraintId: 2\n  +      createdExplicitly: true\n  +      encodingType: 1\n  +      foreignKey: {}\n  +      geoConfig: {}\n  +      id: 2\n  +      interleave: {}\n  +      keyColumnDirections:\n  +      - ASC\n  +      keyColumnIds:\n  +      - 1\n  +      keyColumnNames:\n  +      - i\n  +      name: crdb_internal_index_2_name_placeholder\n  +      partitioning: {}\n  +      sharded: {}\n  +      storeColumnIds:\n  +      - 3\n  +      storeColumnNames:\n  +      - k\n  +      unique: true\n  +      version: 4\n  +    mutationId: 1\n  +    state: BACKFILLING\n  +  - direction: ADD\n  +    index:\n  +      constraintId: 3\n  +      createdExplicitly: true\n  +      encodingType: 1\n  +      foreignKey: {}\n  +      geoConfig: {}\n  +      id: 3\n  +      interleave: {}\n  +      keyColumnDirections:\n  +      - ASC\n  +      keyColumnIds:\n  +      - 1\n  +      keyColumnNames:\n  +      - i\n  +      name: crdb_internal_index_3_name_placeholder\n  +      partitioning: {}\n  +      sharded: {}\n  +      storeColumnIds:\n  +      - 3\n  +      storeColumnNames:\n  +      - k\n  +      unique: true\n  +      useDeletePreservingEncoding: true\n  +      version: 4\n  +    mutationId: 1\n  +    state: DELETE_ONLY\n     name: t\n     nextColumnId: 4\n  -  nextConstraintId: 2\n  +  nextConstraintId: 4\n     nextFamilyId: 1\n  -  nextIndexId: 2\n  +  nextIndexId: 4\n     nextMutationId: 1\n     parentId: 100\n  ...\n       - 3\n       storeColumnNames:\n  -    - j\n  +    - crdb_internal_column_2_name_placeholder\n       - k\n       unique: true\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"2\"\n  +  version: \"3\"\nupsert descriptor #105\n  ...\n     createAsOfTime:\n       wallTime: \"1640995200000000000\"\n  +  declarativeSchemaChangerState:\n  +    authorization:\n  +      userName: root\n  +    currentStatuses: <redacted>\n  +    jobId: \"1\"\n  +    nameMapping:\n  +      id: 105\n  +      name: sq1\n  +    relevantStatements:\n  +    - statement:\n  +        redactedStatement: ALTER TABLE ‹defaultdb›.‹public›.‹t› DROP COLUMN ‹j›\n  +        statement: ALTER TABLE t DROP COLUMN j\n  +        statementTag: ALTER TABLE\n  +    revertible: true\n  +    targetRanks: <redacted>\n  +    targets: <redacted>\n     formatVersion: 3\n     id: 105\n  ...\n       start: \"1\"\n     unexposedParentSchemaId: 101\n  -  version: \"1\"\n  +  version: \"2\"\ndelete comment ColumnCommentType(objID: 104, subID: 2)\npersist all catalog changes to storage\ncreate job #1 (non-cancelable: false): \"ALTER TABLE defaultdb.public.t DROP COLUMN j\"\n  descriptor IDs: [104 105]\n# end PreCommitPhase\ncommit transaction #1\nnotified job registry to adopt jobs: [1]\n# begin PostCommitPhase\nbegin transaction #2\ncommit transaction #2\nbegin transaction #3\n## PostCommitPhase stage 1 of 7 with 4 MutationType ops\nupsert descriptor #104\n  ...\n         version: 4\n       mutationId: 1\n  -    state: DELETE_ONLY\n  +    state: WRITE_ONLY\n     name: t\n     nextColumnId: 4\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"3\"\n  +  version: \"4\"\nupsert descriptor #105\n  ...\n       start: \"1\"\n     unexposedParentSchemaId: 101\n  -  version: \"2\"\n  +  version: \"3\"\npersist all catalog changes to storage\nupdate progress of schema change job #1: \"PostCommitPhase stage 2 of 7 with 1 BackfillType op pending\"\ncommit transaction #3\nbegin transaction #4\n## PostCommitPhase stage 2 of 7 with 1 BackfillType op\nbackfill indexes [2] from index #1 in table #104\ncommit transaction #4\nbegin transaction #5\n## PostCommitPhase stage 3 of 7 with 4 MutationType ops\nupsert descriptor #104\n  ...\n         version: 4\n       mutationId: 1\n  -    state: BACKFILLING\n  +    state: DELETE_ONLY\n     - direction: ADD\n       index:\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"4\"\n  +  version: \"5\"\nupsert descriptor #105\n  ...\n       start: \"1\"\n     unexposedParentSchemaId: 101\n  -  version: \"3\"\n  +  version: \"4\"\npersist all catalog changes to storage\nupdate progress of schema change job #1: \"PostCommitPhase stage 4 of 7 with 1 MutationType op pending\"\ncommit transaction #5\nbegin transaction #6\n## PostCommitPhase stage 4 of 7 with 4 MutationType ops\nupsert descriptor #104\n  ...\n         version: 4\n       mutationId: 1\n  -    state: DELETE_ONLY\n  +    state: MERGING\n     - direction: ADD\n       index:\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"5\"\n  +  version: \"6\"\nupsert descriptor #105\n  ...\n       start: \"1\"\n     unexposedParentSchemaId: 101\n  -  version: \"4\"\n  +  version: \"5\"\npersist all catalog changes to storage\nupdate progress of schema change job #1: \"PostCommitPhase stage 5 of 7 with 1 BackfillType op pending\"\ncommit transaction #6\nbegin transaction #7\n## PostCommitPhase stage 5 of 7 with 1 BackfillType op\nmerge temporary indexes [3] into backfilled indexes [2] in table #104\ncommit transaction #7\nbegin transaction #8\n## PostCommitPhase stage 6 of 7 with 5 MutationType ops\nupsert descriptor #104\n  ...\n         version: 4\n       mutationId: 1\n  -    state: MERGING\n  -  - direction: ADD\n  +    state: WRITE_ONLY\n  +  - direction: DROP\n       index:\n         constraintId: 3\n  ...\n         version: 4\n       mutationId: 1\n  -    state: WRITE_ONLY\n  +    state: DELETE_ONLY\n     name: t\n     nextColumnId: 4\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"6\"\n  +  version: \"7\"\nupsert descriptor #105\n  ...\n       start: \"1\"\n     unexposedParentSchemaId: 101\n  -  version: \"5\"\n  +  version: \"6\"\npersist all catalog changes to storage\nupdate progress of schema change job #1: \"PostCommitPhase stage 7 of 7 with 1 ValidationType op pending\"\ncommit transaction #8\nbegin transaction #9\n## PostCommitPhase stage 7 of 7 with 1 ValidationType op\nvalidate forward indexes [2] in table #104\ncommit transaction #9\nbegin transaction #10\n## PostCommitNonRevertiblePhase stage 1 of 4 with 11 MutationType ops\nupsert descriptor #104\n  ...\n           statement: ALTER TABLE t DROP COLUMN j\n           statementTag: ALTER TABLE\n  -    revertible: true\n       targetRanks: <redacted>\n       targets: <redacted>\n  ...\n       direction: DROP\n       mutationId: 1\n  -    state: WRITE_ONLY\n  -  - direction: ADD\n  -    index:\n  -      constraintId: 2\n  -      createdExplicitly: true\n  -      encodingType: 1\n  -      foreignKey: {}\n  -      geoConfig: {}\n  -      id: 2\n  -      interleave: {}\n  -      keyColumnDirections:\n  -      - ASC\n  -      keyColumnIds:\n  -      - 1\n  -      keyColumnNames:\n  -      - i\n  -      name: crdb_internal_index_2_name_placeholder\n  -      partitioning: {}\n  -      sharded: {}\n  -      storeColumnIds:\n  -      - 3\n  -      storeColumnNames:\n  -      - k\n  -      unique: true\n  -      version: 4\n  -    mutationId: 1\n  -    state: WRITE_ONLY\n  +    state: DELETE_ONLY\n     - direction: DROP\n       index:\n  -      constraintId: 3\n  -      createdExplicitly: true\n  +      constraintId: 1\n  +      createdAtNanos: \"1640995200000000000\"\n         encodingType: 1\n         foreignKey: {}\n         geoConfig: {}\n  -      id: 3\n  +      id: 1\n         interleave: {}\n         keyColumnDirections:\n  ...\n         keyColumnNames:\n         - i\n  -      name: crdb_internal_index_3_name_placeholder\n  +      name: crdb_internal_index_1_name_placeholder\n         partitioning: {}\n         sharded: {}\n         storeColumnIds:\n  +      - 2\n         - 3\n         storeColumnNames:\n  +      - crdb_internal_column_2_name_placeholder\n         - k\n         unique: true\n  -      useDeletePreservingEncoding: true\n         version: 4\n       mutationId: 1\n  -    state: DELETE_ONLY\n  +    state: WRITE_ONLY\n     name: t\n     nextColumnId: 4\n  ...\n     parentId: 100\n     primaryIndex:\n  -    constraintId: 1\n  -    createdAtNanos: \"1640995200000000000\"\n  +    constraintId: 2\n  +    createdExplicitly: true\n       encodingType: 1\n       foreignKey: {}\n       geoConfig: {}\n  -    id: 1\n  +    id: 2\n       interleave: {}\n       keyColumnDirections:\n  ...\n       sharded: {}\n       storeColumnIds:\n  -    - 2\n       - 3\n       storeColumnNames:\n  -    - crdb_internal_column_2_name_placeholder\n       - k\n       unique: true\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"7\"\n  +  version: \"8\"\nupsert descriptor #105\n  ...\n           statement: ALTER TABLE t DROP COLUMN j\n           statementTag: ALTER TABLE\n  -    revertible: true\n       targetRanks: <redacted>\n       targets: <redacted>\n  ...\n       start: \"1\"\n     unexposedParentSchemaId: 101\n  -  version: \"6\"\n  +  version: \"7\"\npersist all catalog changes to storage\nupdate progress of schema change job #1: \"PostCommitNonRevertiblePhase stage 2 of 4 with 4 MutationType ops pending\"\nset schema change job #1 to non-cancellable\ncommit transaction #10\nbegin transaction #11\n## PostCommitNonRevertiblePhase stage 2 of 4 with 7 MutationType ops\nupsert descriptor #104\n  ...\n         version: 4\n       mutationId: 1\n  -    state: WRITE_ONLY\n  +    state: DELETE_ONLY\n     name: t\n     nextColumnId: 4\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"8\"\n  +  version: \"9\"\nupsert descriptor #105\n  ...\n       start: \"1\"\n     unexposedParentSchemaId: 101\n  -  version: \"7\"\n  +  version: \"8\"\npersist all catalog changes to storage\nupdate progress of schema change job #1: \"PostCommitNonRevertiblePhase stage 3 of 4 with 12 MutationType ops pending\"\ncommit transaction #11\nbegin transaction #12\n## PostCommitNonRevertiblePhase stage 3 of 4 with 15 MutationType ops\ndelete object namespace entry {100 101 sq1} -> 105\nupsert descriptor #104\n  ...\n     - columnIds:\n       - 1\n  -    - 2\n       - 3\n       columnNames:\n       - i\n  -    - crdb_internal_column_2_name_placeholder\n       - k\n       name: primary\n  ...\n     id: 104\n     modificationTime: {}\n  -  mutations:\n  -  - column:\n  -      id: 2\n  -      name: crdb_internal_column_2_name_placeholder\n  -      nullable: true\n  -      ownsSequenceIds:\n  -      - 105\n  -      type:\n  -        family: IntFamily\n  -        oid: 20\n  -        width: 64\n  -    direction: DROP\n  -    mutationId: 1\n  -    state: DELETE_ONLY\n  -  - direction: DROP\n  -    index:\n  -      constraintId: 1\n  -      createdAtNanos: \"1640995200000000000\"\n  -      encodingType: 1\n  -      foreignKey: {}\n  -      geoConfig: {}\n  -      id: 1\n  -      interleave: {}\n  -      keyColumnDirections:\n  -      - ASC\n  -      keyColumnIds:\n  -      - 1\n  -      keyColumnNames:\n  -      - i\n  -      name: crdb_internal_index_1_name_placeholder\n  -      partitioning: {}\n  -      sharded: {}\n  -      storeColumnIds:\n  -      - 2\n  -      - 3\n  -      storeColumnNames:\n  -      - crdb_internal_column_2_name_placeholder\n  -      - k\n  -      unique: true\n  -      version: 4\n  -    mutationId: 1\n  -    state: DELETE_ONLY\n  +  mutations: []\n     name: t\n     nextColumnId: 4\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"9\"\n  +  version: \"10\"\nupsert descriptor #105\n  ...\n         ownerTableId: 104\n       start: \"1\"\n  +  state: DROP\n     unexposedParentSchemaId: 101\n  -  version: \"8\"\n  +  version: \"9\"\npersist all catalog changes to storage\ncreate job #2 (non-cancelable: true): \"GC for ALTER TABLE defaultdb.public.t DROP COLUMN j\"\n  descriptor IDs: [104]\nupdate progress of schema change job #1: \"PostCommitNonRevertiblePhase stage 4 of 4 with 1 MutationType op pending\"\ncommit transaction #12\nnotified job registry to adopt jobs: [2]\nbegin transaction #13\n## PostCommitNonRevertiblePhase stage 4 of 4 with 4 MutationType ops\nupsert descriptor #104\n  ...\n     createAsOfTime:\n       wallTime: \"1640995200000000000\"\n  -  declarativeSchemaChangerState:\n  -    authorization:\n  -      userName: root\n  -    currentStatuses: <redacted>\n  -    jobId: \"1\"\n  -    nameMapping:\n  -      columns:\n  -        \"1\": i\n  -        \"3\": k\n  -        \"4294967292\": crdb_internal_origin_timestamp\n  -        \"4294967293\": crdb_internal_origin_id\n  -        \"4294967294\": tableoid\n  -        \"4294967295\": crdb_internal_mvcc_timestamp\n  -      families:\n  -        \"0\": primary\n  -      id: 104\n  -      indexes:\n  -        \"2\": t_pkey\n  -      name: t\n  -    relevantStatements:\n  -    - statement:\n  -        redactedStatement: ALTER TABLE ‹defaultdb›.‹public›.‹t› DROP COLUMN ‹j›\n  -        statement: ALTER TABLE t DROP COLUMN j\n  -        statementTag: ALTER TABLE\n  -    targetRanks: <redacted>\n  -    targets: <redacted>\n     families:\n     - columnIds:\n  ...\n       time: {}\n     unexposedParentSchemaId: 101\n  -  version: \"10\"\n  +  version: \"11\"\nupsert descriptor #105\n  ...\n     createAsOfTime:\n       wallTime: \"1640995200000000000\"\n  -  declarativeSchemaChangerState:\n  -    authorization:\n  -      userName: root\n  -    currentStatuses: <redacted>\n  -    jobId: \"1\"\n  -    nameMapping:\n  -      id: 105\n  -      name: sq1\n  -    relevantStatements:\n  -    - statement:\n  -        redactedStatement: ALTER TABLE ‹defaultdb›.‹public›.‹t› DROP COLUMN ‹j›\n  -        statement: ALTER TABLE t DROP COLUMN j\n  -        statementTag: ALTER TABLE\n  -    targetRanks: <redacted>\n  -    targets: <redacted>\n     formatVersion: 3\n     id: 105\n  ...\n     state: DROP\n     unexposedParentSchemaId: 101\n  -  version: \"9\"\n  +  version: \"10\"\npersist all catalog changes to storage\ncreate job #3 (non-cancelable: true): \"GC for ALTER TABLE defaultdb.public.t DROP COLUMN j\"\n  descriptor IDs: [105]\nupdate progress of schema change job #1: \"all stages completed\"\nset schema change job #1 to non-cancellable\nupdated schema change job #1 descriptor IDs to []\nwrite *eventpb.FinishSchemaChange to event log:\n  sc:\n    descriptorId: 104\nwrite *eventpb.FinishSchemaChange to event log:\n  sc:\n    descriptorId: 105\ncommit transaction #13\nnotified job registry to adopt jobs: [3]\n# end PostCommitPhase\n"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -177,2 +177,4 @@
        	            	   +        "3": k
        	            	+  +        "4294967292": crdb_internal_origin_timestamp
        	            	+  +        "4294967293": crdb_internal_origin_id
        	            	   +        "4294967294": tableoid
        	            	@@ -685,2 +687,4 @@
        	            	   -        "3": k
        	            	+  -        "4294967292": crdb_internal_origin_timestamp
        	            	+  -        "4294967293": crdb_internal_origin_id
        	            	   -        "4294967294": tableoid
        	Test:       	TestEndToEndSideEffects_drop_column_sequence_owner
        	Messages:   	drop_column_sequence_owner

rafiss

looks like it just needs a rebase and rewrite.

but in case you didn't see my earlier comment, please make sure to add backport labels.

fqazi · 2024-10-10T14:22:04Z

@rafiss Let me rebase this and add labels

fqazi · 2024-10-10T14:25:15Z

bors r-

craig · 2024-10-10T14:25:18Z

Canceled.

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 operation with a sequence owner from restoring with the error: "rewriting descriptor ids: missing rewrite for <id> in SequenceOwner..."

fqazi · 2024-10-10T14:39:59Z

bors r+

132129: roachtest: add slow disk perturbation test r=kvoli a=andrewbaptist This change adds a new set of perturbation tests perturbation/*/slowDisk which tests slow disks. We have see support cases where slow disks can cause cluster level availability outages. Epic: none Release note: None 132166: rac2,kvserver: do not quiesce if send tokens held r=sumeerbhola a=pav-kv This PR prevents range quiescence if RACv2 holds any send tokens for this range. Quiescence would prevent `MsgApp` pings which ensure that the leader reliably learns about the follower store admitting log entries, and causes it to release tokens accordingly. We do not want to end up holding tokens permanently. Resolves #129581 132202: sql/schemachanger: clean up SequenceOwner elements during restore r=fqazi a=fqazi 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: #130778 Release note (bug fix): Addressed a rare bug that could prevent backups taken during a DROP COLUMN operation with a sequence owner from restoring with the error: "rewriting descriptor ids: missing rewrite for <id> in SequenceOwner..." Co-authored-by: Andrew Baptist <[email protected]> Co-authored-by: Pavel Kalinnikov <[email protected]> Co-authored-by: Faizan Qazi <[email protected]>

craig · 2024-10-10T14:53:35Z

Build failed (retrying...):

examples_orms

craig · 2024-10-10T15:56:44Z

Build succeeded:

blathers-crl · 2024-10-10T15:56:51Z

Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches.

Issue #130778: branch-release-23.1, branch-release-23.2, branch-release-24.1, branch-release-24.2.

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

blathers-crl · 2024-10-10T15:57:26Z

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 e08e89b to blathers/backport-release-23.1-132202: 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 23.1.x failed. See errors above.

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

fqazi requested a review from a team as a code owner October 8, 2024 21:26

Dedej-Bergin approved these changes Oct 9, 2024

View reviewed changes

fqazi force-pushed the fixRestoreDropSequence branch from 3849f33 to 00c8c6a Compare October 10, 2024 02:22

fqazi commented Oct 10, 2024

View reviewed changes

fqazi force-pushed the fixRestoreDropSequence branch from 00c8c6a to fcb1652 Compare October 10, 2024 03:00

Dedej-Bergin reviewed Oct 10, 2024

View reviewed changes

rafiss requested changes Oct 10, 2024

View reviewed changes

rafiss approved these changes Oct 10, 2024

View reviewed changes

fqazi added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Oct 10, 2024

fqazi added backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2 labels Oct 10, 2024

fqazi force-pushed the fixRestoreDropSequence branch from fcb1652 to e08e89b Compare October 10, 2024 14:36

craig bot merged commit 188d9fe into cockroachdb:master Oct 10, 2024
23 checks passed

blathers-crl bot mentioned this pull request Oct 10, 2024

roachtest: backup-restore/online-restore failed [cluster backup missing sequence] #130778

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql/schemachanger: clean up SequenceOwner elements during restore #132202

sql/schemachanger: clean up SequenceOwner elements during restore #132202

fqazi commented Oct 8, 2024

blathers-crl bot commented Oct 8, 2024

cockroach-teamcity commented Oct 8, 2024

Dedej-Bergin commented Oct 9, 2024

Dedej-Bergin commented Oct 9, 2024

Dedej-Bergin commented Oct 9, 2024

Dedej-Bergin commented Oct 9, 2024

Dedej-Bergin left a comment

fqazi left a comment

fqazi commented Oct 10, 2024

craig bot commented Oct 10, 2024

fqazi commented Oct 10, 2024

craig bot commented Oct 10, 2024

Dedej-Bergin left a comment

Dedej-Bergin commented Oct 10, 2024

craig bot commented Oct 10, 2024

craig bot commented Oct 10, 2024

rafiss commented Oct 10, 2024

rafiss left a comment

rafiss left a comment

fqazi commented Oct 10, 2024

fqazi commented Oct 10, 2024

craig bot commented Oct 10, 2024

fqazi commented Oct 10, 2024

craig bot commented Oct 10, 2024

craig bot commented Oct 10, 2024

blathers-crl bot commented Oct 10, 2024

blathers-crl bot commented Oct 10, 2024

sql/schemachanger: clean up SequenceOwner elements during restore #132202

sql/schemachanger: clean up SequenceOwner elements during restore #132202

Conversation

fqazi commented Oct 8, 2024

blathers-crl bot commented Oct 8, 2024

cockroach-teamcity commented Oct 8, 2024

Dedej-Bergin commented Oct 9, 2024

Dedej-Bergin commented Oct 9, 2024

Dedej-Bergin commented Oct 9, 2024

Dedej-Bergin commented Oct 9, 2024

Dedej-Bergin left a comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

fqazi commented Oct 10, 2024

craig bot commented Oct 10, 2024

fqazi commented Oct 10, 2024

craig bot commented Oct 10, 2024

Dedej-Bergin left a comment

Choose a reason for hiding this comment

Dedej-Bergin commented Oct 10, 2024

craig bot commented Oct 10, 2024

craig bot commented Oct 10, 2024

rafiss commented Oct 10, 2024

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

fqazi commented Oct 10, 2024

fqazi commented Oct 10, 2024

craig bot commented Oct 10, 2024

fqazi commented Oct 10, 2024

craig bot commented Oct 10, 2024

craig bot commented Oct 10, 2024

blathers-crl bot commented Oct 10, 2024

blathers-crl bot commented Oct 10, 2024