sql/schemachanger: prerequisite changes for DROP COLUMN #84157

ajwerner · 2022-07-10T19:32:16Z

This PR comes in a number of commits and is best reviewed commit-by-commit. This PR contains cleanup needed to implement DROP COLUMN (#84072). The actual support for that feature will follow in a separate PR.

Two commits do some internal refactoring of the builder.
The next commit adjust the planning rules which over-constrained the graph.
The next fixes a rather thorny bug related to multiple dep edges between the same two nodes not being handled correctly.
The next commit augments descriptor decomposition to include columns referenced in expressions.
The next commit adds an IndexSet to catid.
The next commit fixes a bug which prevents adding a dropping index back to a table.
The next commit reworks MakeFirstMutationPublic in order to make introducing a new filter easier.
Fixes a bug in scstage which lead to ignoring revertibility in the Statement and PreCommit phases

cockroach-teamcity · 2022-07-10T19:32:35Z

This change is

ajwerner · 2022-07-12T18:24:37Z

This needs another commit to make the changefeed behavior happy, and maybe there's a sql test or two which needs tweaking, but this is ready for an early read.

ajwerner · 2022-07-13T13:18:14Z

There are two more things that need to be fixed on this:

the changefeed treats the primary index swap as a schema change in a way that it should not. This one is straightforward enough to fix.
the TestRaceWithBackfill test is flakey for a very good reason: we need to move the dropped column to DELETE_ONLY before we swap the primary index containing it out of the primary index slot. On some level this is running amuck of the revertibility boundaries as we've defined them. I'm going to try to finesse this without reworking the framework, but this might prove hard and be the spark needed to get rid of that wart and replace it with a more right thing.

Today's going to be very busy, so I don't think I'll be able to resolve both before EOD. I hope to have it all ready to merge some time tomorrow.

postamar

In light of the above comment I reviewed all but the last commit. I recommend you put up a separate PR with these so that we can ship these welcome changes right away. The only major comment I have is that if you need to track referenced columns in existing expressions then you should add those IDs to the Expression element building block and populate them during scdecomp.

Reviewed 2 of 2 files at r1, 3 of 3 files at r2, 8 of 8 files at r3, 11 of 11 files at r4, 2 of 2 files at r5, 2 of 2 files at r6, 1 of 1 files at r7, 5 of 5 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

-- commits line 42 at r6:
reword

pkg/sql/catalog/index_id_set.go line 90 at r6 (raw file):

// numbers are shown as ranges. For example, for the set {1, 2, 3  5, 6, 10},
// the output is "(1-3,5,6,10)".
func (s IndexIDSet) String() string { return s.set.String() }

This is all fine but should we move this and its cousins to catid?

pkg/sql/schemachanger/scbuild/builder_state.go line 559 at r5 (raw file):

	}
	return cols
}

Shouldn't these column references be in the scpb.Expression element instead, and populated by scdecomp?

pkg/sql/schemachanger/scplan/plan_explain.go line 237 at r4 (raw file):

					}
					rn.AddLine(fmt.Sprintf("rules: [%s]", strings.Join(rules, ", ")))
				}

Can you treeprinterify the rules to have them printed as separate tree nodes instead, or at least on separate lines? It's good to print them all, but like this it won't age well.

pkg/sql/schemachanger/scplan/internal/rules/dep_index_and_column.go line 366 at r3 (raw file):

	)

	// TODO(ajwerner): Understand this rule and why it needs to exist.

The reason the name is tied to the column type instead of the column is because otherwise DROP TABLE would be sad. See rules further below involving IsRelationBeingDropped (I thought we'd gotten rid of those? idk)

Whether that still applies is unclear.

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

		if de, ok := e.(*DepEdge); ok {
			sb.WriteString(de.rules.String())

unintentional newline?

pkg/sql/logictest/testdata/logic_test/new_schema_changer line 176 at r4 (raw file):

                  RunningStatus: all stages completed

noise

pkg/sql/schemachanger/scplan/plan_test.go line 176 at r4 (raw file):

				screl.ElementString(de.To().Element()), de.To().CurrentStatus)
			fmt.Fprintf(&deps, "  kind: %s\n", de.Kind())
			fmt.Fprintf(&deps, "  rule: %s\n", de.RuleNames())

s/rule/rules/

This refactors the code a tad and extracts some helpers. Release note: None

Extracted a few helpers for code reuse. Release note: None

One rule was too strict, and another would only match the secondary index and not the temp index. Also, the variable names were swapped. Release note: None

ajwerner

TFTR!

I've peeled the last commit off of this PR and reworked the descriptor so this can merge. I'll put up the last commit separately.

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

-- commits line 42 at r6: