[WIP] scbuild: support DROP TYPE ... CASCADE #96503

postamar · 2023-02-03T16:03:33Z

Recently, we've added support for constraint removal in the declarative
schema changer. This now makes it possible to support DROP TYPE ...
CASCADE statements. As a result, DROP OWNED BY now also performs
correctly when user-defined types are involved and no longer returns an
error.

This commit also extends the coverage of the declarative schema
changer's ALTER TABLE ... DROP COLUMN support, which no longer punts to
the legacy schema changer when the column to drop is referenced in
a constraint. Now, the constraint gets dropped.

Fixes #51480.
Fixes #55908.

Release note (sql change): DROP TYPE ... CASCADE is now supported and no
longer returns an error. Consequently DROP OWNED BY no longer returns an
error when it tries to drop a type in a cascading manner.

cockroach-teamcity · 2023-02-03T16:03:44Z

This change is

postamar · 2023-02-04T04:30:46Z

The changes required to support DROP TYPE ... CASCADE are, in my opinion, basically sound, but these are revealing quite a few existing bugs worth looking into. These bugs could equally be triggered by dropping sequences referenced in check constraints and/or in view queries.

Recently, we've added support for constraint removal in the declarative schema changer. This now makes it possible to support DROP TYPE ... CASCADE statements. As a result, DROP OWNED BY now also performs correctly when user-defined types are involved and no longer returns an error. This commit also extends the coverage of the declarative schema changer's ALTER TABLE ... DROP COLUMN support, which no longer punts to the legacy schema changer when the column to drop is referenced in a constraint. Now, the constraint gets dropped. The declarative schema changer still differs from Postgres behaviour when the column to drop is part of the primary key. Whereas postgres happily removes the primary key constraint and the column, CRDB is unable to do so straigtforwardly. What to do here remains an open question, there are many possible solutions, in the meantime we return an error. Fixes cockroachdb#51480. Fixes cockroachdb#55908. Release note (sql change): DROP TYPE ... CASCADE is now supported and no longer returns an error. Consequently DROP OWNED BY no longer returns an error when it tries to drop a type in a cascading manner.

postamar

Here are some notes on the current state of affairs as I go away on leave. Some are clear action items for 23.1. Still, I'm hopeful that there's not much that needs to be done for DROP TYPE CASCADE to be acceptably completed for that release.

postamar · 2023-02-04T13:47:34Z

pkg/sql/catalog/rewrite/rewrite.go

+				state.CurrentStatuses = append(state.CurrentStatuses[:i], state.CurrentStatuses[i+1:]...)
+				state.TargetRanks = append(state.TargetRanks[:i], state.TargetRanks[i+1:]...)
+				i--
+				continue


I'm not sure whether this error-handling part is correct but the changes in the WalkDescIDs closure I'm confident about. The rewrite logic really needs some love ASAP.

postamar · 2023-02-04T13:48:23Z

pkg/sql/catalog/tabledesc/structured.go

+			return nil
+		}
+		return f(&c.Predicate)
+	}


It appears that we completely forgot about unique-without-index partial expressions here. Did we forget about them elsewhere in this package? Do we validate them properly? What would it take to remove the "experimental" designation for this feature?

postamar · 2023-02-04T13:50:17Z

pkg/sql/logictest/testdata/logic_test/drop_type

-statement error pgcode 0A000 unimplemented: cannot drop type "test.schema_to_drop.(_)?typ" because other objects \(\[test\.public\.t\]\) still depend on it
+skipif config local-legacy-schema-changer
+skipif config local-mixed-22.2-23.1
+statement error pgcode 42P10 column "k" is referenced by the primary key


DROP TYPE ... CASCADE still chokes when the type is used by a column in a PK. To be fully postgres-compatible we should do a primary index swap and set the new PK to a new rowid column. I haven't bothered.

postamar · 2023-02-04T13:51:24Z

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_drop_column.go

-			// TODO(ajwerner): Support dropping FOREIGN KEY constraints.
-			panic(errors.Wrap(scerrors.NotImplementedError(n),
-				"dropping of FOREIGN KEY constraints not supported"))
+			dropConstraintByID(b, e.TableID, e.ConstraintID)


These changes here and above in this file should make it into the 23.1 release and be adequately tested, independently of DROP TYPE ... CASCADE.

postamar · 2023-02-04T13:52:13Z

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go

 			return false
 		}
 		// Otherwise, return true to signal the removal of the element.
+		// TRANSIENT targets also need to be considered here, as we do not want
+		// them to come into existence at all, even if they are to go away
+		// eventually.


This was a bug, which needs to be fixed regardless.

postamar · 2023-02-04T13:53:26Z

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go

+	for _, qe := range s.q {
+		qe.dropUndroppedBackReferencedElements(n)
+	}
+}


I had to rewrite this to drop the descriptors wholesale in a first pass before fiddling with individual column drops which may otherwise not work because zone configs or whatever. This change is worth keeping as it's more sensible anyways.

postamar · 2023-02-04T13:54:15Z

pkg/sql/schemachanger/scdecomp/decomp.go

+			for _, id := range columnType.ComputeExpr.UsesTypeIDs {
+				newClosedTypeIDs.Add(id)
+			}
+			columnType.ClosedTypeIDs = newClosedTypeIDs.Ordered()


We really need to decompose the compute expr into a separate element in this release.

postamar · 2023-02-04T13:55:23Z

pkg/sql/schemachanger/scexec/scmutationexec/constraint.go

@@ -122,14 +122,7 @@ func (i *immediateVisitor) MakeValidatedCheckConstraintPublic(
 		if c := mutation.GetConstraint(); c != nil &&
 			c.ConstraintType == descpb.ConstraintToUpdate_CHECK &&
 			c.Check.ConstraintID == op.ConstraintID {
-			// Remove the mutation from the mutation slice. The `MakeMutationComplete`
-			// call will mark the check in the public "Checks" slice as VALIDATED.
-			err = tbl.MakeMutationComplete(mutation)


Here and elsewhere in this file I just inlined this method and found to result to be simpler and less error-prone. In any case this was failing when reverting a DROP CONSTRAINT schema change, MakeMutationComplete would choke on the fact that the mutation was ADD and the validating state was Dropping.

postamar · 2023-02-04T13:57:56Z

pkg/sql/schemachanger/scplan/internal/scstage/build.go

+			// Remove satisfied targets once the schema change is no longer
+			// revertible.
+			continue
+		}


We could be more aggressive in removing targets for the schema change state, and from releasing descriptors from the schema change job. I'm not sure how necessary it is, though. I think previously I'd left "done" descriptors in the job on the assumption that in the non-revertible post-commit phase the job would finish soon anyway and this wouldn't matter, but this perhaps interferes with the cumulative backup tests. To be investigated.

postamar · 2023-02-04T13:58:43Z

pkg/sql/schemachanger/sctest/cumulative.go

+	    split_part(create_statement, ';', 1),
+	    'CREATE VIEW (.*) AS SELECT .*',
+	    'CREATE VIEW \1 AS SELECT ...'
+	) AS create_statement


The view query gets rewritten with OID type references, so this needs to be done.

postamar · 2023-05-16T15:39:38Z

Closing this because this depends on DROP COLUMN being implemented properly. Presently, we haven't implemented dropping a column which is featured in the PK, which is annoying if the column is also of a user-defined type. Postgres is quite happy to drop columns in the PK by also dropping the PK constraint. In CRDB this is more awkward due to the PK constraint being tightly coupled to the primary index, but we could conceivably work around this by adding a rowid column and using that as a PK instead, assuming there are no other suitable unique constraints on the table.

postamar force-pushed the drop-type-cascade branch 3 times, most recently from 0b1a7c0 to b8381af Compare February 4, 2023 04:27

postamar changed the title ~~scbuild: support DROP TYPE ... CASCADE~~ [WIP] scbuild: support DROP TYPE ... CASCADE Feb 4, 2023

postamar force-pushed the drop-type-cascade branch from b8381af to 1d84a91 Compare February 4, 2023 13:46

postamar commented Feb 4, 2023

View reviewed changes

postamar closed this May 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] scbuild: support DROP TYPE ... CASCADE #96503

[WIP] scbuild: support DROP TYPE ... CASCADE #96503

postamar commented Feb 3, 2023

cockroach-teamcity commented Feb 3, 2023

postamar commented Feb 4, 2023

postamar left a comment

postamar Feb 4, 2023

postamar Feb 4, 2023

postamar Feb 4, 2023

postamar Feb 4, 2023

postamar Feb 4, 2023

postamar Feb 4, 2023

postamar Feb 4, 2023

postamar Feb 4, 2023

postamar Feb 4, 2023

postamar Feb 4, 2023

postamar commented May 16, 2023

[WIP] scbuild: support DROP TYPE ... CASCADE #96503

[WIP] scbuild: support DROP TYPE ... CASCADE #96503

Conversation

postamar commented Feb 3, 2023

cockroach-teamcity commented Feb 3, 2023

postamar commented Feb 4, 2023

postamar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

postamar commented May 16, 2023