schemachanger: eager statement phase

[postamar]

This commit reworks how the declarative schema changer performs DROPs by
removing support for synthetic descriptors in the statement transaction.
In-transaction operations are now executed eagerly in the statement
phase instead of being deferred to the pre-commit phase.

The most significant change this entails is to the rules definitions,
since we now no longer have the TXN_DROPPED status. I have taken this
opportunity to rewrite them in a way that involves more element set
definitions, whose properties are enforced by init-time assertions.

The operation implementations in scexec have been simplified somewhat,
not having to concern ourselves with synthetic descriptors any more.
The opgen definitions are also simplified now that they are no longer
peppered with pre-commit phase constraints. Instead, phase constraints
are implied by the number of transitions: more than 1 and those other
than the first are implied to be restricted to post-commit. This makes
all phase constraints implicit.

This commit improves the graph validation logic to detect all cycles,
not just dep-edge cycles, and renders any detected cycle in the detail
of the resulting error.

This commit makes some changes in the test dependencies which mock the
catalog API, as this now has to support both the in-txn and committed
state of the catalog. In the process this commit adds better support for
descriptor modification times.

Release note: None

[cockroach-teamcity]

This change is 

[postamar]

This could certainly be broken up into multiple commits, but I don't have time to break it down before going away, so I'm putting this here in the meantime.

[ajwerner]

This was a deeply pleasant change. There's a rttanalysis to rewrite. Just little things from me. Any rough edges introduced seem smoother than the ones they replace. Thank you for doing this!

Reviewed 89 of 90 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

---

pkg/sql/schemachanger/scdeps/sctestdeps/test_state.go line 41 at r1 (raw file):

// purpose of facilitating end-to-end testing of the declarative schema changer.
type TestState struct {
	committed, uncommitted  nstree.MutableCatalog

can you comment about what this is? what does it mean for something to be committed or uncomitted?

---

pkg/sql/schemachanger/scplan/internal/scgraph/graph.go line 359 at r1 (raw file):

	sb.WriteString("cycle:\n")
	for _, e := range collectCycle(edge) {
		sb.WriteString(screl.NodeString(e.From()))

I feel like this should end up printing the other side of the edge, it currently only prints the From if I read it correctly. The test might fool you becuase the depEdge rule field has a to and from.

---

pkg/ccl/schemachangerccl/testdata/end_to_end/drop_multiregion line 1996 at r1 (raw file):

# end PreCommitPhase
notified job registry to adopt jobs: [1]
commit transaction #1

the inversion here seems bad. I don' think we can send the notification until after the commit

Code quote:

notified job registry to adopt jobs: [1]
commit transaction #1

---

pkg/sql/schemachanger/scplan/internal/rules/testdata/deprules line 1 at r1 (raw file):

rules

wanna add this part back?

Code quote:

rules
----
joinTarget(element, target):

[chengxiong-ruan]

Really like this change 👍 👍 👍

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @chengxiong-ruan, and @postamar)

---

pkg/sql/schemachanger/scdeps/sctestdeps/test_state.go line 122 at r1 (raw file):

func (s *TestState) mvccTimestamp() hlc.Timestamp {
	return hlc.Timestamp{WallTime: defaultOverriddenCreatedAt.UnixNano() + int64(s.txnCounter)}

simple and smart :)

---

pkg/sql/schemachanger/scexec/scmutationexec/column.go line 170 at r1 (raw file):

	col.Type = types.Any
	if col.IsComputed() {
		null := tree.Serialize(tree.DNull)

May I ask why this is necessary?

---

pkg/sql/schemachanger/scplan/internal/opgen/target.go line 78 at r1 (raw file):

			t.from = tbs.from
			t.to = s.to
			if err := tbs.withTransition(s, i == 0); err != nil {

nit, even it seems obvious that i == 0 means is first, but adding /* isFirst */ note is appreciated.

---

pkg/sql/schemachanger/scplan/internal/rules/dep_drop.go line 263 at r1 (raw file):

			return rel.Clauses{
				elementTypes(from, isDescriptor),
				elementTypes(to, func(e scpb.Element) bool {

this is awesome

---

pkg/sql/schemachanger/scplan/internal/scgraph/graph.go line 319 at r1 (raw file):

	pred := make(map[*screl.Node]Edge, order)
	var visit func(n *screl.Node, in Edge)
	visit = func(n *screl.Node, in Edge) {

small improvement, feel free to ignore... but feels like visit should return an error so that we can check if err := visit(n, e); err != nil and return early, otherwise it won't return until visiting all remain edges, though it's cheap since each visit would return immediately without going further.

---

pkg/sql/schemachanger/scplan/internal/scgraph/graph.go line 359 at r1 (raw file):

Previously, ajwerner wrote…

I feel like this should end up printing the other side of the edge, it currently only prints the From if I read it correctly. The test might fool you becuase the depEdge rule field has a to and from.

It seems lgtm since it begins from the edge with target as from, and ends at edge with target as to... maybe I misunderstood what andrew said heh.

[postamar]

Thanks for the kind words. I'll bors as soon as able.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @chengxiong-ruan, and @postamar)

---

pkg/sql/schemachanger/scdeps/sctestdeps/test_state.go line 41 at r1 (raw file):

Previously, ajwerner wrote…

can you comment about what this is? what does it mean for something to be committed or uncomitted?

Done.

---

pkg/sql/schemachanger/scexec/scmutationexec/column.go line 170 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

May I ask why this is necessary?

Added a comment to explain, you're right it's not obvious at all.

---

pkg/sql/schemachanger/scplan/internal/opgen/target.go line 78 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

nit, even it seems obvious that i == 0 means is first, but adding /* isFirst */ note is appreciated.

Done.

---

pkg/sql/schemachanger/scplan/internal/scgraph/graph.go line 319 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

small improvement, feel free to ignore... but feels like visit should return an error so that we can check if err := visit(n, e); err != nil and return early, otherwise it won't return until visiting all remain edges, though it's cheap since each visit would return immediately without going further.

You're right. The newest code is also a bit cleaner. I think the previous code was the way it was because we originally did a topological sort here, but that was removed when I rewrote the stage builder.

---

pkg/sql/schemachanger/scplan/internal/scgraph/graph.go line 359 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

It seems lgtm since it begins from the edge with target as from, and ends at edge with target as to... maybe I misunderstood what andrew said heh.

Yes, that's what I was thinking too. There's a couple of examples of this output in the scgraph test assertions and they lgtm.

---

pkg/ccl/schemachangerccl/testdata/end_to_end/drop_multiregion line 1996 at r1 (raw file):

Previously, ajwerner wrote…

the inversion here seems bad. I don' think we can send the notification until after the commit

Fixed. It was only inverted in the test deps implementation, thankfully.

---

pkg/sql/schemachanger/scplan/internal/rules/testdata/deprules line 1 at r1 (raw file):

Previously, ajwerner wrote…

wanna add this part back?

Whoops. That's what happens when you git checkout --theirs willy-nilly to resolve merge conflicts, I guess.

[ajwerner]

Both of those build failures are flakes which seem unrelated:

docker: Error response from daemon: Head "https://registry-1.docker.io/v2/library/golang/manifests/stretch": received unexpected HTTP status: 500 Internal Server Error.

Service 'kdc' failed to build: Head "https://registry-1.docker.io/v2/library/alpine/manifests/3.14": received unexpected HTTP status: 500 Internal Server Error

I think this is ready to go. Enjoy your vacation.

bors r+

[ajwerner]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @chengxiong-ruan, and @postamar)

[craig]

Build succeeded:

• GitHub CI (Cockroach)