sql/schemachanger: add back TXN_DROPPED, stop using OFFLINE #86639

ajwerner · 2022-08-23T06:02:59Z

The first two commits enable synthetic descriptors to be used during transaction
execution to influence the virtual tables.

We added the OFFLINE and performed write operations during drops to move the
descriptor into that state in order to deal with the fact that synthetically
dropped descriptors would not appear so in virtual tables. Fortunately, this
shortcoming has been rectified. We now revive the TXN_DROPPED state. This
proves to be important because the OFFLINE state is not backwards compatible,
and the TXN_DROPPED state is. Ideally we'd find a way to move the descriptors
straight to DROPPED in the pre-commit case for all the cases we today support,
but doing that would require an overhaul of the dependency rules which, at this
point in the cycle, feels risky.

Fixes #86626
Fixes #77148
Fixes #86691

Release justification: Fixes a release-blocking backwards incompatibility.

Release note: None

cockroach-teamcity · 2022-08-23T06:03:08Z

This change is

ajwerner · 2022-08-23T13:58:07Z

This turns out to be insufficient; we need to make it such that the bulk of the work in the drop cases does happen during pre-commit. Otherwise, we're stuck with incompatibilities.

Xiang-Gu · 2022-08-29T19:39:07Z

I fixed a few build errors, rebase on Master, and ran --rewrite on affected tests in the above push.

CI now reports test failures; we will need to look into them next.

Xiang-Gu · 2022-08-30T14:53:45Z

pkg/sql/schemachanger/scplan/internal/rules/helpers.go

+			descriptor.typeFilter(IsDescriptor),
+			descriptor.joinTarget(),
+			joinOnDescIDUntyped(descriptor.el, element, "id"),
+			descriptor.targetStatus(scpb.ToAbsent),


should this line be descriptor.targetStatus(scpb.ToPublic)?

No, we want this rule to unify if we are dropping the descriptor. The NotJoin implies negation where it is used if everything inside the rule unifies.

Xiang-Gu · 2022-08-30T15:42:32Z

pkg/sql/schemachanger/scplan/internal/rules/dep_drop_object.go

+				from.typeFilter(IsDescriptor),
+				from.el.AttrEqVar(screl.DescID, "_"),
+				from.el.AttrEqVar(rel.Self, to.el),
+				to.el.AttrEqVar(rel.Self, to.el),


Isn't this line always true and hence redundant?

Yes, removed

Xiang-Gu · 2022-08-30T15:44:38Z

pkg/sql/schemachanger/scplan/internal/rules/dep_drop_object.go

+	registerDepRule(
+		"descriptor txn_drop before descriptor drop",
+		scgraph.PreviousStagePrecedence,
+		"descriptor", "dependent",


If this rule is about making sure one descriptor reached TXN_DROPPED before reaching DROPPED in a previous stage, then we probably should change the name of the second variable dependent to, maybe, descriptor_self.

Good catch, fixed

Xiang-Gu · 2022-08-30T15:54:04Z

pkg/sql/schemachanger/scplan/internal/rules/dep_two_version.go

+	type statusFunc = func(
+		from nodeVars, fromStatus scpb.Status, to nodeVars, toStatus scpb.Status,
+	) rel.Clause
+	addRules := func(c depElement, targetStatus scpb.TargetStatus, sf statusFunc) {


it seems sf statusFunc is not used. Did I miss something?

No, you're right. Removed.

Xiang-Gu · 2022-08-30T15:58:59Z

pkg/sql/schemachanger/scplan/internal/scgraph/edge.go

+	// PreviousTransactionPrecedence is like PreviousStagePrecedence but does
+	// not allow the transition to occur if the current phase is not PostCommit.


Can you be elobarate a bit more on the comment? Does it meanfrom must be reached before to in a previous stage, and both stages must be in PostCommit phase? If so, what do you think of the name PreviousStageInPostCommitPrecendence?

It means that from must be reached before to in a previous stage and if from was reached in StatementPhase than to cannot be reached in PreCommitPhase.

I'm open to both better naming and a better comment.

does not allow the transition to occur unless the current phase is at least PostCommitPhase, because StatementPhase and PreCommitPhase are special in that they take place in the same transaction?

ajwerner

Thanks for taking a look and fixing the issues. The validation things are interesting. I'll get to the bottom of them one way or another. I think I addressed the basic comments, but did it on the old version of the branch. I'll find and add back your changes.

ajwerner · 2022-08-30T16:16:28Z

pkg/sql/schemachanger/scplan/internal/scgraph/edge.go

+	// PreviousTransactionPrecedence is like PreviousStagePrecedence but does
+	// not allow the transition to occur if the current phase is not PostCommit.


It means that from must be reached before to in a previous stage and if from was reached in StatementPhase than to cannot be reached in PreCommitPhase.

ajwerner · 2022-08-30T16:16:44Z

pkg/sql/schemachanger/scplan/internal/scgraph/edge.go

+	// PreviousTransactionPrecedence is like PreviousStagePrecedence but does
+	// not allow the transition to occur if the current phase is not PostCommit.


I'm open to both better naming and a better comment.

ajwerner · 2022-08-30T16:44:05Z

pkg/sql/schemachanger/scplan/internal/rules/dep_two_version.go

+	type statusFunc = func(
+		from nodeVars, fromStatus scpb.Status, to nodeVars, toStatus scpb.Status,
+	) rel.Clause
+	addRules := func(c depElement, targetStatus scpb.TargetStatus, sf statusFunc) {


No, you're right. Removed.

ajwerner · 2022-08-30T17:07:06Z

pkg/sql/schemachanger/scplan/internal/rules/dep_drop_object.go

+	registerDepRule(
+		"descriptor txn_drop before descriptor drop",
+		scgraph.PreviousStagePrecedence,
+		"descriptor", "dependent",


Good catch, fixed

ajwerner · 2022-08-30T17:07:15Z

pkg/sql/schemachanger/scplan/internal/rules/dep_drop_object.go

+				from.typeFilter(IsDescriptor),
+				from.el.AttrEqVar(screl.DescID, "_"),
+				from.el.AttrEqVar(rel.Self, to.el),
+				to.el.AttrEqVar(rel.Self, to.el),


Yes, removed

ajwerner · 2022-08-30T17:13:21Z

pkg/sql/schemachanger/scplan/internal/rules/helpers.go

+			descriptor.typeFilter(IsDescriptor),
+			descriptor.joinTarget(),
+			joinOnDescIDUntyped(descriptor.el, element, "id"),
+			descriptor.targetStatus(scpb.ToAbsent),


No, we want this rule to unify if we are dropping the descriptor. The NotJoin implies negation where it is used if everything inside the rule unifies.

postamar

This is coming along nicely!

postamar · 2022-08-30T21:07:14Z

pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go

@@ -560,9 +560,18 @@ func (s *TestState) MustReadDescriptor(ctx context.Context, id descpb.ID) catalo
 	return desc
 }

+// AddSyntheticDescriptor is part of the scexec.Catalog interface.
+func (s *TestState) AddSyntheticDescriptor(desc catalog.MutableDescriptor) {
+	s.LogSideEffectf("added synthetic descriptor: %v", desc)


Can we instead print the yaml diff between the synthetic descriptor and the existing immutable descriptor if it exists?

postamar · 2022-08-30T21:07:58Z

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

@@ -117,4 +117,6 @@ type MutationVisitorStateUpdater interface {

 	// RefreshStats refresh stats for a given descriptor.
 	RefreshStats(id descpb.ID)
+	MarkDescriptorSynthetic(id descpb.ID)
+	//	MarkDescriptorSynthetic(id descpb.ID)


postamar · 2022-08-30T21:20:14Z

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

@@ -322,7 +322,7 @@ func (sb stageBuilder) isOutgoingOpEdgeAllowed(e *scgraph.OpEdge) bool {
 	if e.Type() != sb.opType {
 		return false
 	}
-	if !e.IsPhaseSatisfied(sb.bs.phase) {
+	if !e.IsPhaseSatisfied(sb.bs.phase) && !sb.bc.g.IsNoOp(e) {


Nice. By the way, you probably should remove hasOpEdgeWithOps and the logic around it in this PR. Search for // TODO(postamar): uphold the 2-version invariant using dep rules instead.

postamar · 2022-08-30T21:25:13Z

pkg/sql/schemachanger/rel/schema_rules.go

+func makeRandomVars(n int) (ret []Var) {
+	ret = make([]Var, n)
+	for i := range ret {
+		ret[i] = Var(hex.EncodeToString(uuid.MakeV4().GetBytes()))


This is going to hurt legibility when debugging. Can't we instead use a reserved prefix followed by a counter? You probably can assert that this reserved prefix isn't used in vars outside of rel.

postamar · 2022-08-30T21:28:31Z

pkg/sql/schemachanger/rel/query_build.go

+	// if we have a not-join, then we need to find the slots for the inputs
+	// and we have to build the sub-query, which is a whole new query, and
+	// we have to then figure out its depth. In general, its depth should be
+	// the current join depth, so the current


incomplete comment

Moved into processNotJoin

postamar · 2022-08-30T21:48:25Z

pkg/sql/schemachanger/scplan/internal/rules/dep_two_version.go

+
+}
+
+// notJoinOnNodeWithStatusIn is a cache to memoize getNotJoinOnNodeWithStatusIn. a not-join rule which will


incomplete comment

Removed the tail

postamar · 2022-08-30T21:48:46Z

pkg/sql/schemachanger/scplan/internal/scgraph/edge.go

-// IsPhaseSatisfied returns true iff the operations can run in the given phase.
-func (oe *OpEdge) IsPhaseSatisfied(phase scop.Phase) bool {
-	return phase >= oe.minPhase
-}


Good riddance.

postamar · 2022-08-30T21:49:36Z

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

@@ -361,7 +358,7 @@ func (sb stageBuilder) nextTargetState(t currentTargetState) currentTargetState
 		} else {
 			next.hasOpEdgeWithOps = true
 		}
-	}
+	}*/


Just get rid of this, as suggested in a previous comment, along with hasOpEdgeWithOps.

postamar · 2022-08-30T21:50:14Z

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

-		if sb.isUnmetInboundDep(de) {
-			ret = true
+		ret = sb.isUnmetInboundDep(de)
+		if ret {


nit: is this really more readable?

No, there was some other code in an earlier iteration.

I combined it.

postamar · 2022-08-30T21:50:44Z

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

+		return !fromIsFulfilled ||
+			(sb.bs.phase <= scop.PreCommitPhase &&
+				// If it has been fulfilled implicitly because it's the initial
+				// status, then it doesn't matter the current phase, let it go.


then the current phase doesn't matter?

postamar

My comments are just nits, so LGTM.

We added the `OFFLINE` and performed write operations during drops to move the descriptor into that state in order to deal with the fact that synthetically dropped descriptors would not appear so in virtual tables. Fortunately, this shortcoming has been rectified. We now revive the TXN_DROPPED state. This proves to be important because the `OFFLINE` state is not backwards compatible, and the `TXN_DROPPED` state is. Ideally we'd find a way to move the descriptors straight to `DROPPED` in the pre-commit case for all the cases we today support, but doing that would require an overhaul of the dependency rules which,at this point in the cycle, feels risky. Fixes cockroachdb#86626 Release justification: Fixes a release-blocking backwards incompatibility. Release note: None

…reCommit We can schedule non-revertible operations into PreCommit so long as the next stage is PostCommitNonRevertible. Release justification: part of bug fix Release note: None

…dges Release note: None

This is just to avoid bugs later. Release justification: cleanup Release note: None

ajwerner

I took your changes and addressed the comments.

ajwerner · 2022-09-01T02:08:59Z

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

+		return !fromIsFulfilled ||
+			(sb.bs.phase <= scop.PreCommitPhase &&
+				// If it has been fulfilled implicitly because it's the initial
+				// status, then it doesn't matter the current phase, let it go.


ajwerner · 2022-09-01T02:09:39Z

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

-		if sb.isUnmetInboundDep(de) {
-			ret = true
+		ret = sb.isUnmetInboundDep(de)
+		if ret {


No, there was some other code in an earlier iteration.

ajwerner · 2022-09-01T02:10:50Z

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

-		if sb.isUnmetInboundDep(de) {
-			ret = true
+		ret = sb.isUnmetInboundDep(de)
+		if ret {


I combined it.

ajwerner · 2022-09-01T02:13:12Z

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

@@ -361,7 +358,7 @@ func (sb stageBuilder) nextTargetState(t currentTargetState) currentTargetState
 		} else {
 			next.hasOpEdgeWithOps = true
 		}
-	}
+	}*/


ajwerner · 2022-09-01T02:13:55Z

pkg/sql/schemachanger/scplan/internal/scgraph/edge.go

+	// PreviousTransactionPrecedence is like PreviousStagePrecedence but does
+	// not allow the transition to occur if the current phase is not PostCommit.


ajwerner · 2022-09-01T02:14:28Z

pkg/sql/schemachanger/scplan/internal/rules/dep_two_version.go

+
+}
+
+// notJoinOnNodeWithStatusIn is a cache to memoize getNotJoinOnNodeWithStatusIn. a not-join rule which will


Removed the tail

ajwerner · 2022-09-01T02:16:01Z

pkg/sql/schemachanger/scplan/internal/rules/dep_two_version.go

+			panic(err)
+		}
+	}
+	_ = screl.ForEachElementType(func(el scpb.Element) error {


Nice, deleted the new one.

ajwerner · 2022-09-01T02:30:26Z

pkg/sql/schemachanger/rel/query_build.go

+	// if we have a not-join, then we need to find the slots for the inputs
+	// and we have to build the sub-query, which is a whole new query, and
+	// we have to then figure out its depth. In general, its depth should be
+	// the current join depth, so the current


Moved into processNotJoin

ajwerner · 2022-09-01T02:31:26Z

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

@@ -117,4 +117,6 @@ type MutationVisitorStateUpdater interface {

 	// RefreshStats refresh stats for a given descriptor.
 	RefreshStats(id descpb.ID)
+	MarkDescriptorSynthetic(id descpb.ID)
+	//	MarkDescriptorSynthetic(id descpb.ID)


postamar · 2022-09-01T12:11:19Z

CI is failing due to a compilation error:
pkg/sql/schemachanger/rel/query_build.go:323:3: unnecessary assignment to the blank identifier (S1005)

Let's 🚢 🇮🇹. My very minor concern about the var generator can wait another day.

This patch endows rel with the ability to define not-join rules which allow extra variables to be introduced inside the clauses. When a not-join rule is invoked, it leads to the outer query finding a contradiction if the rule fully unifies. Fixes cockroachdb#77148 Release justification: part of a broader bug-fix Release note: None

…ules Previously we had implicit rules in opgen which were used to enforce sequencing between transitions. This was brittle and made it hard to collapse certain edges. Now that we can represent a not-join, we can add the rules we want selectively, only when descriptors are not being dropped. Fixes cockroachdb#86691 Fixes cockroachdb#86626 Release justification: fixes a bug Release note: None

ajwerner · 2022-09-01T13:36:24Z

TFTR!

bors r=postamar

craig · 2022-09-01T14:54:41Z

Build succeeded:

Bazel Essential CI (Cockroach)

ajwerner force-pushed the ajwerner/add-back-txn-dropped branch from c3c22c6 to 5e13723 Compare August 23, 2022 13:51

ajwerner changed the title ~~sql/catalog/descs: combine synthetic descriptors with stored descriptors~~ sql/schemachanger: add back TXN_DROPPED, stop using OFFLINE Aug 23, 2022

ajwerner force-pushed the ajwerner/add-back-txn-dropped branch from 5e13723 to 1f1ee74 Compare August 25, 2022 17:29

Xiang-Gu mentioned this pull request Aug 26, 2022

tests: new roachtest for job backward compatibility in new schema changer #86950

Merged

Xiang-Gu force-pushed the ajwerner/add-back-txn-dropped branch 2 times, most recently from f3e316b to ab3df2b Compare August 29, 2022 16:51

Xiang-Gu reviewed Aug 30, 2022

View reviewed changes

ajwerner force-pushed the ajwerner/add-back-txn-dropped branch from ab3df2b to ea5ba71 Compare August 30, 2022 17:22

ajwerner commented Aug 30, 2022

View reviewed changes

ajwerner force-pushed the ajwerner/add-back-txn-dropped branch 3 times, most recently from 8763145 to cff1905 Compare August 30, 2022 20:47

postamar reviewed Aug 30, 2022

View reviewed changes

postamar approved these changes Aug 30, 2022

View reviewed changes

Xiang-Gu mentioned this pull request Aug 31, 2022

rules: suppress a dep-rule in special case #87196

Merged

postamar mentioned this pull request Aug 31, 2022

#86639 but based on #87067 #87223

Closed

ajwerner force-pushed the ajwerner/add-back-txn-dropped branch from cff1905 to a6c8876 Compare August 31, 2022 21:12

ajwerner added 4 commits August 31, 2022 22:54

sql/schemachanger/scplan/internal/scstage: accept non-revertible in P…

3e568bc

…reCommit We can schedule non-revertible operations into PreCommit so long as the next stage is PostCommitNonRevertible. Release justification: part of bug fix Release note: None

sql/schemachanger/scplan/internal/scstage: implicitly satisfy no-op e…

cd4959e

…dges Release note: None

sql/schemachanger/rel: internally store rules with random vars

24800a8

This is just to avoid bugs later. Release justification: cleanup Release note: None

ajwerner force-pushed the ajwerner/add-back-txn-dropped branch from a6c8876 to 6a7477d Compare September 1, 2022 03:14

ajwerner commented Sep 1, 2022

View reviewed changes

ajwerner marked this pull request as ready for review September 1, 2022 03:35

ajwerner requested review from a team and miretskiy September 1, 2022 03:35

ajwerner force-pushed the ajwerner/add-back-txn-dropped branch 4 times, most recently from e67e63e to 2ada0c2 Compare September 1, 2022 06:34

ajwerner force-pushed the ajwerner/add-back-txn-dropped branch from 2ada0c2 to 1051bf0 Compare September 1, 2022 12:52

ajwerner added 2 commits September 1, 2022 08:53

ajwerner force-pushed the ajwerner/add-back-txn-dropped branch from 1051bf0 to affdfee Compare September 1, 2022 12:53

ajwerner mentioned this pull request Sep 1, 2022

sql/schemachanger/rel: support not-join subqueries #86753

Closed

craig bot merged commit 9911916 into cockroachdb:master Sep 1, 2022

ajwerner deleted the ajwerner/add-back-txn-dropped branch September 1, 2022 14:54

		// PreviousTransactionPrecedence is like PreviousStagePrecedence but does
		// not allow the transition to occur if the current phase is not PostCommit.


		}

		// notJoinOnNodeWithStatusIn is a cache to memoize getNotJoinOnNodeWithStatusIn. a not-join rule which will

sql/schemachanger: add back TXN_DROPPED, stop using OFFLINE #86639

sql/schemachanger: add back TXN_DROPPED, stop using OFFLINE #86639

Conversation

ajwerner commented Aug 23, 2022 • edited Loading

cockroach-teamcity commented Aug 23, 2022

ajwerner commented Aug 23, 2022

Xiang-Gu commented Aug 29, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajwerner 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

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

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 left a comment

Choose a reason for hiding this comment

ajwerner 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

postamar commented Sep 1, 2022

ajwerner commented Sep 1, 2022

craig bot commented Sep 1, 2022

ajwerner commented Aug 23, 2022 •

edited

Loading