schemachanger: prettier EXPLAIN (DDL) output #103930

postamar · 2023-05-25T21:41:26Z

This commit adds ID -> name mappings to the scpb.TargetState which lies at the heart of the declarative schema changer, these mappings are then used (when present) to decorate the output of EXPLAIN (DDL) in an effort to make it more understandable.

This commit also adds support for the SHAPE flag in conjunction with EXPLAIN (DDL). The generated output informs on expensive operations such as backfills or validations.

Fixes #104043.

Release note (sql change): EXPLAIN (DDL) statements now have descriptor, index, column, constraint and other ID values decorated with names when available. There is now also a new EXPLAIN (DDL, SHAPE) statement which provides information on costly operations planned by the declarative schema changer, like which index backfills and validations will get performed.

cockroach-teamcity · 2023-05-25T21:41:39Z

This change is

Xiang-Gu

Thanks for doing this; this is super valuable! I don't have any opposition of the technical solution. I have some questions on the wordings of the explain output and a few minor questions/comments.

Why do you want to call this new flag "SHAPE"? I didn't immediately see the meaning.
Do we want to make "explain_shape" directory work similarly to "explain" and "explain_verbose"? That is, for every tested statement in an end-to-end test, we have one file for it under the "explain_shape" directory. Furthermore, when we run the TestEndToEnd_xxx test with a "rewrite" flag, the corresponding file under "explain_shape" gets rewritten (just as the file under "explain" and "explain_verbose")

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

pkg/sql/schemachanger/scbuild/build.go line 324 at r1 (raw file):

	scpb.ForEachNamespace(b, func(_ scpb.Status, ts scpb.TargetStatus, e *scpb.Namespace) {
		dnm, isNew := getOrCreate(e.DescriptorID)
		if isNew || ts == scpb.InvalidTarget || ts == scpb.ToPublic {

why do we want the ts==scpb.InvalidTarget filter?

pkg/sql/schemachanger/scpb/name_mappings.go line 132 at r1 (raw file):

		return name
	}
	return NameMappings(ts.NameMappings).ConstraintName(relationID, constraintID)

Did I understand this correctly: the purpose of the last return clause is for cases where certain name-related elements are needed but not involved in the plan (ts.Target); For example, altering a table won't touch the parent database but we probably want to know its parent database name, which will be stored in the builder state as an element with InvalidTargetStatus by the decomp logic.

pkg/sql/schemachanger/scplan/plan_explain.go line 205 at r1 (raw file):

			var en treeprinter.Node
			var estimatedMemAlloc int
			accountFor := func(label string) string {

I saw this accountFor helper is defined multiple times. Will it be better to make it a function func accountFor(label string, memAcc *int) string and define it once?

pkg/sql/schemachanger/scplan/plan_explain.go line 377 at r1 (raw file):

// ExplainShape returns a human-readable plan rendering for
// EXPLAIN (DDL, SHAPE) statements.

Can you add a bit more comments here about the main purpose of this new EXPLAIN flag? something like "it gives a condensed summary of the operations involved in the schema change with highlights on expensive operations (backfill, validation, etc.), which helps users understand where most of the time will be spent if the schema change is to be executed."

pkg/sql/schemachanger/scplan/plan_explain.go line 446 at r1 (raw file):

func (p Plan) explainBackfillsAndMerges(root treeprinter.Node, ops []scop.Op) error {
	gbs, gms := groupBackfillsAndMerges(ops)

Are we respecting the real ordering of the backfill and merges if the plan involves >1 backfills and merges?

IIUC, backfill and merge always appears in pairs. If we have a plan where the real ordering of the operations are

backfill_1
merge_1
backfill_2
merge_2

Will the logic print out operations in that order? Or, will it be all backfills first and then all merges (i.e. backfill_1 followed by backfill_2 followed by merge_1 followed by merge_2)?

pkg/sql/schemachanger/scplan/plan_explain.go line 596 at r1 (raw file):

	sort.Slice(gbs, func(i, j int) bool {
		if gbs[i].relationID == gbs[j].relationID {
			return gbs[i].srcIndexID < gbs[j].srcIndexID

I have a gut feeling that this might not work properly, because it's possible that we have three backfill/merge groups:
index_a <-- index_b <-- index_c <-- index_d
where we backfill index_b from index_a, backfill index_c from index_b, and so on.

It's possible, depending on the implementation, that index_b.GetID() is bigger than index_c.GetID() (do you remember in my support-add-drop-column-alterPK PR we inflate this chain, de-duplicate, and re-inflate again for the next ALTER TABLE stmt, and so on. I think this makes what I described here possible)

In my PR, I need to do similar things to order the primary indexes and I had logic there to sort by the location of srcIndex (i.e. if 'B.srcIdx = A', then A comes before B in the ordering)

pkg/ccl/schemachangerccl/testdata/explain/create_index line 14 at r1 (raw file):

 │    └── Stage 1 of 1 in StatementPhase
 │         ├── 7 elements transitioning toward PUBLIC
 │         │    ├── ABSENT → BACKFILL_ONLY    SecondaryIndex:{DescID 104 "t1", IndexID 2 "+id1", TemporaryIndexID 3, SourceIndexID 1 "t1_pkey"}

what is this "+id1" name for IndexID 2? Saw a bunch of them

pkg/ccl/schemachangerccl/testdata/explain_shape/create_index line 13 at r1 (raw file):

 ├── execute 2 system table mutations transactions
 ├── backfill using primary index t1_pkey in relation t1
 │     ⇒ +id1: id↗, name↗, money

How to understand/interpret this line? Can we pretty print this step with something like backfill primary index (4) "idx4(i, j, k)" from primary index (2) "idx2(i,j)" in relation "t1", or, if we want to use symbols/shapes, maybe something like "idx2(i,j) <-- idx4(i,j,k)`.

pkg/ccl/schemachangerccl/testdata/explain_shape/create_index line 16 at r1 (raw file):

 ├── execute 2 system table mutations transactions
 ├── merge temporary indexes into backfilled indexes in relation t1
 │     ~t1@[3] ⇒ +id1

Similarly, can we format merging step with something easier to read/understand?

postamar

Thanks very much for having taken a look! This helps.

This flag already exists for DML plans and is used to synthetise them somewhat.
Indeed, yes, already done.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Xiang-Gu)

pkg/sql/schemachanger/scbuild/build.go line 324 at r1 (raw file):