sql: add index recommendation to EXPLAIN output

[nehageorge]

indexrec: disable inverted index recommendations

Previously, we would erroneously give index recommendations of non-
inverted indexes for indexes that should be inverted. In this PR,
we omit index candidates that required inverted indexes. Specifically,
this includes JSON columns, array columns, and spatial data columns.

Release note: None

sql: add index recommendation to EXPLAIN

Previously, the EXPLAIN output only showed the query plan. Since users
often use EXPLAIN to see why a query is poorly performing, it would
be useful for them to also see index recommendations to make the query
faster. In this commit, index recommendations are added to the bottom of
the EXPLAIN output. See the indexrec package under pkg/sql/opt/indexrec
to understand how index recommendations are generated.

Release note (sql change): The output of the EXPLAIN SQL statement has changed.
Below the plan, we now output index recommendations for the SQL statement being
"EXPLAIN-ed", if there are any. These index recommendations are indexes the
user could add or indexes they could replace to make the given query faster.

Closes #72761.

indexrec: refactor index recommendation output

Previously, we manually formatted the string for the index recommendation's
SQL output. This commit takes advantage of the existing functionality for
formatting SQL statements, specifically CREATE INDEX and DROP INDEX. This
will make it easier to add more types of index recommendations in the future.

Release note: None

[cockroach-teamcity]

This change is 

[rytaft]

Nice! I think you're going to need to update a lot of test output...

Reviewed 8 of 8 files at r1, 10 of 10 files at r2, 6 of 6 files at r3, 2 of 2 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nehageorge)

---

pkg/sql/plan_opt.go, line 530 at r5 (raw file):

	// find potential index candidates in the memo.
	if _, isExplain := opc.p.stmt.AST.(*tree.Explain); isExplain {
		if err := opc.makeQueryIndexRecommendation(f); err != nil {

doesn't necessarily need to be in this PR, but make sure that you check a session / cluster setting here so users can optionally disable this feature

---

pkg/sql/planner.go, line 228 at r5 (raw file):

	// indexRecommendations is a string containing index recommendations for the
	// planned statement. This is only set for EXPLAIN statements.
	indexRecommendations string

I think instrumentationHelper would be a better place for this. I'd love to avoid adding another field to planner

---

pkg/sql/opt/indexrec/index_candidate_set.go, line 370 at r4 (raw file):

}

func requiresInvertedIndex(c *cat.Column) bool {

It would be good to just reuse this existing function instead if you can:

cockroach/pkg/sql/catalog/colinfo/col_type_info.go

Line 121 in 25e38dd

func ColumnTypeIsIndexable(t *types.T) bool {

---

pkg/sql/opt/testutils/opttester/testdata/index-candidates-recommendations, line 18 at r4 (raw file):

# Ensure that columns that require inverted indexes do not have candidates. This
# is temporary until we support inverted index recommendations.

it would be good to also add tests here for predicates that would actually use an inverted index, e.g.,
j @> '[1]' or a @> '{1}'

[nehageorge]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nehageorge and @rytaft)

---

pkg/sql/opt/testutils/opttester/testdata/index-candidates-recommendations, line 18 at r4 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

it would be good to also add tests here for predicates that would actually use an inverted index, e.g.,
j @> '[1]' or a @> '{1}'

I don't know if that would be valuable given my design. Since we don't have rules for *memo.ContainsExpr and *memo.ContainedByExpr, these types of predicates wouldn't get an index candidate either way. Alternatively, we have a rule for *memo.IsExpr, so the fact that there is no candidate here is an indicator that the code is working as intended.

[nehageorge]

Yep, you're right. On it.

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

---

pkg/sql/plan_opt.go, line 530 at r5 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

doesn't necessarily need to be in this PR, but make sure that you check a session / cluster setting here so users can optionally disable this feature

Yep, will do in another PR.

---

pkg/sql/opt/indexrec/index_candidate_set.go, line 370 at r4 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

It would be good to just reuse this existing function instead if you can:

cockroach/pkg/sql/catalog/colinfo/col_type_info.go

Line 121 in 25e38dd

func ColumnTypeIsIndexable(t *types.T) bool {

Done.

---

pkg/sql/planner.go, line 228 at r5 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

I think instrumentationHelper would be a better place for this. I'd love to avoid adding another field to planner

This makes sense. Done.

[rytaft]

Reviewed 6 of 6 files at r6, 3 of 3 files at r7, 4 of 4 files at r8, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rytaft)

---

pkg/sql/opt/testutils/opttester/testdata/index-candidates-recommendations, line 18 at r4 (raw file):

Previously, nehageorge wrote…

I don't know if that would be valuable given my design. Since we don't have rules for *memo.ContainsExpr and *memo.ContainedByExpr, these types of predicates wouldn't get an index candidate either way. Alternatively, we have a rule for *memo.IsExpr, so the fact that there is no candidate here is an indicator that the code is working as intended.

True, these cases will mostly be useful once you add support for recommending inverted indexes. Feel free to hold off until then.

[mgartner]

I haven't reviewed the entire PR, but I left a few comments to consider. Looks great so far!

Reviewed 13 of 13 files at r10, 63 of 63 files at r11.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @nehageorge, and @rytaft)

---

pkg/sql/plan_opt.go, line 668 at r11 (raw file):

// query is outputted based on which hypothetical indexes are helpful in the
// optimal plan.
func (opc *optPlanningCtx) makeQueryIndexRecommendation(f *norm.Factory) error {

nit: When f is passed in as an argument, it's not obvious that this is the same factory in opc.optimizer. I think it'd be clearer to have no argument and do f := opc.optimizer.Factory() at the top of this function.

---

pkg/sql/plan_opt.go, line 673 at r11 (raw file):

	// Use the optimizer to fully normalize the memo. The sort enforcer is added
	// to the memo in this step.

Why is The sort enforcer is added to the memo in this step. relevant here?

---

pkg/sql/plan_opt.go, line 705 at r11 (raw file):

	// Update the saved memo's metadata with the original table information.
	// Prepare to re-optimize and create an executable plan.

nit: Explain that re-initializing the optimizer also re-initializes the factory.

---

pkg/sql/plan_opt.go, line 717 at r11 (raw file):

// hasVirtualTables returns true if there are any virtual tables in the query
// metadata. Otherwise, it returns false.
func hasVirtualTables(f *norm.Factory) bool {

nit: this could also be a method of optPlanningCtx.

---

pkg/sql/opt/cat/table.go, line 142 at r11 (raw file):

	// LookupColumnOrdinal returns the ordinal of the column with the given ID. A
	// cache makes the lookup O(1).

Adding a function to cat.Table that is not used internally by the optimizer, and that involves descpb.ColumnID seems like an anti-pattern. I'm going to spend some time to see if some of the code that relies on cat.Table in opt_catalog.go can be reworked so you don't have to do this.

---

pkg/sql/opt/exec/execbuilder/testdata/explain, line 1621 at r11 (raw file):

EXPLAIN (OPT, MEMO) SELECT * FROM tc JOIN t ON k=a
----
memo (optimized, ~10KB, required=[presentation: info:10])

I haven't finished reviewing the PR, but a ~29% reduction in the size of the memo here is certainly surprising. I saw other tests where memos shrank. Do you know what is causing this?

---

pkg/sql/opt/indexrec/index_candidate_set.go, line 172 at r11 (raw file):

		if colID == 0 {
			continue
		}

I would think it would be impossible for an ordering column to have an ID of 0. In what case does this occur?

---

pkg/sql/opt/indexrec/index_recommendation_set.go, line 153 at r11 (raw file):

	}

	var output []string

Is it possible to know the number of output lines based on the size of irs? If so, you could allocate an exactly-sized slice here with make.

---

pkg/sql/opt/indexrec/testdata/index-candidates-recommendations, line 25 at r10 (raw file):

index-candidates
SELECT * FROM t4 WHERE k = 2 AND a IS NULL OR f > 2

nit: use j in the query filter, e.g., j->'a' = '1'

[nehageorge]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner, @nehageorge, and @rytaft)

---

pkg/sql/plan_opt.go, line 673 at r11 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Why is The sort enforcer is added to the memo in this step. relevant here?

The reason I included it is because otherwise we would not have to call Optimize at all prior to determining index candidates. (We would have all the information we need in the Build memo). I can make that more clear in the comment.

---

pkg/sql/opt/cat/table.go, line 142 at r11 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Adding a function to cat.Table that is not used internally by the optimizer, and that involves descpb.ColumnID seems like an anti-pattern. I'm going to spend some time to see if some of the code that relies on cat.Table in opt_catalog.go can be reworked so you don't have to do this.

As per our discussion, I will refactor this.

---

pkg/sql/opt/exec/execbuilder/testdata/explain, line 1621 at r11 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I haven't finished reviewing the PR, but a ~29% reduction in the size of the memo here is certainly surprising. I saw other tests where memos shrank. Do you know what is causing this?

Yea, I believe it comes from the fact that the memo goes through this flow before optimization in addition to the typical flow:

savedMemo := opc.optimizer.DetachMemo()
opc.optimizer.Init(f.EvalContext(), &opc.catalog)
if err := f.AssignPlaceholders(savedMemo); err != nil {
		return err
}

The reason why this flow causes a difference could potentially warrant further investigation. I thought that it was fine because the code path is different, but maybe this is not expected?

---

pkg/sql/opt/indexrec/index_candidate_set.go, line 172 at r11 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I would think it would be impossible for an ordering column to have an ID of 0. In what case does this occur?

IIRC this occurred and was previously causing a test to fail. I am currently trying to figure out which case this happens in.

---

pkg/sql/opt/indexrec/testdata/index-candidates-recommendations, line 25 at r10 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: use j in the query filter, e.g., j->'a' = '1'

As I mentioned to Becca previously, this makes sense if we consider the actual use of inverted indexes, but since FindIndexCandidateSet doesn't currently match a *memo.FetchValExpr, it would not be a candidate in any case.

[nehageorge]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)

---

pkg/sql/plan_opt.go, line 668 at r11 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: When f is passed in as an argument, it's not obvious that this is the same factory in opc.optimizer. I think it'd be clearer to have no argument and do f := opc.optimizer.Factory() at the top of this function.

Done.

---

pkg/sql/plan_opt.go, line 705 at r11 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: Explain that re-initializing the optimizer also re-initializes the factory.

Done.

---

pkg/sql/plan_opt.go, line 717 at r11 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: this could also be a method of optPlanningCtx.

Done.

---

pkg/sql/opt/cat/table.go, line 142 at r11 (raw file):

Previously, nehageorge wrote…

As per our discussion, I will refactor this.

Done.

---

pkg/sql/opt/indexrec/index_candidate_set.go, line 172 at r11 (raw file):

Previously, nehageorge wrote…

IIRC this occurred and was previously causing a test to fail. I am currently trying to figure out which case this happens in.

I couldn't seem to track it down. To speed up the process, I temporarily commented out these lines to see if it breaks anything.

---

pkg/sql/opt/indexrec/index_recommendation_set.go, line 153 at r11 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Is it possible to know the number of output lines based on the size of irs? If so, you could allocate an exactly-sized slice here with make.

It is, done.

[nehageorge]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)

---

pkg/sql/opt/indexrec/index_candidate_set.go, line 172 at r11 (raw file):

Previously, nehageorge wrote…

I couldn't seem to track it down. To speed up the process, I temporarily commented out these lines to see if it breaks anything.

Looks like CI is passing. My mistake, I was likely confusing the error of getting a TableID of 0 with a ColumnID of 0.

[rytaft]

Nicely done!

Reviewed 2 of 13 files at r10, 60 of 60 files at r13, 60 of 60 files at r14, 5 of 5 files at r15, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @nehageorge, and @rytaft)

---

pkg/sql/plan_opt.go, line 528 at r14 (raw file):

	// For index recommendations, after building we must interrupt the flow to
	// find potential index candidates in the memo.
	if _, isExplain := opc.p.stmt.AST.(*tree.Explain); isExplain && !opc.hasVirtualTables() {

Seems like you might want to be able to support index recommendation as long as there is at least one non-virtual table, right? Maybe add a TODO?

---

pkg/sql/opt/exec/execbuilder/testdata/union, line 10 at r14 (raw file):

query T
EXPLAIN SELECT v FROM uniontest UNION SELECT k FROM uniontest

In the future we should consider including set operations when determining the index candidates. This query would benefit from one index on k and one index on v, so it could perform a streaming set operation. Obviously not something you should add now, but maybe open an issue?

---

pkg/sql/opt/indexrec/index_recommendation_set.go, line 30 at r13 (raw file):

// its hypothetical indexes when optimizing, and then prunes the stored columns
// after, this may result in the best plan not being chosen. Meaning, a plan
// might not be recommended because the optimizer includes the overhead of

nit: it's strange to say "a plan is recommended". The optimizer chooses a plan, which determines which indexes are recommended.

---

pkg/sql/opt/indexrec/index_recommendation_set.go, line 129 at r13 (raw file):

			if index.indexOrdinal == indexOrd {
				// Update newStoredColOrds to include all stored column ordinals that
				// are in scannedColOrds.

nit: update comment since you removed newStoredColOrds

[nehageorge]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @rytaft)

---

pkg/sql/plan_opt.go, line 528 at r14 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Seems like you might want to be able to support index recommendation as long as there is at least one non-virtual table, right? Maybe add a TODO?

Good point. Moved this check to addIndexToCandidates in index_candidate_set.go so we avoid candidates from virtual tables.

---

pkg/sql/opt/exec/execbuilder/testdata/explain, line 1621 at r11 (raw file):

Previously, nehageorge wrote…

Yea, I believe it comes from the fact that the memo goes through this flow before optimization in addition to the typical flow:

savedMemo := opc.optimizer.DetachMemo()
opc.optimizer.Init(f.EvalContext(), &opc.catalog)
if err := f.AssignPlaceholders(savedMemo); err != nil {
		return err
}

The reason why this flow causes a difference could potentially warrant further investigation. I thought that it was fine because the code path is different, but maybe this is not expected?

Added a function to copy the memo's size when copying the memo, to avoid this discrepancy.

---

pkg/sql/opt/exec/execbuilder/testdata/union, line 10 at r14 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

In the future we should consider including set operations when determining the index candidates. This query would benefit from one index on k and one index on v, so it could perform a streaming set operation. Obviously not something you should add now, but maybe open an issue?

Will create an issue.

---

pkg/sql/opt/indexrec/index_recommendation_set.go, line 30 at r13 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: it's strange to say "a plan is recommended". The optimizer chooses a plan, which determines which indexes are recommended.

Done.

---

pkg/sql/opt/indexrec/index_recommendation_set.go, line 129 at r13 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: update comment since you removed newStoredColOrds

Done.

[rytaft]

Reviewed 8 of 8 files at r16, 1 of 3 files at r18, 2 of 2 files at r19, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner, @nehageorge, and @rytaft)

---

pkg/sql/plan_opt.go, line 702 at r16 (raw file):

		savedMemo.RootProps(),
		replaceNoopFn,
	)

Why did you change this?

If this is needed, seems worth turning this into another function called Copy or CopyMemo, with replaceNoopFn defined in factory.go (or instead of replaceNoopFn just use -- and possibly rename -- the existing function CopyScalarWithoutPlaceholders that does the same thing)

---

pkg/sql/opt/indexrec/index_candidate_set.go, line 348 at r19 (raw file):

) {
	// Do not add candidates from virtual tables.
	if currTable.IsVirtualTable() {

should you add a test for a query that uses one virtual table and one non-virtual?

---

pkg/sql/opt/norm/factory.go, line 255 at r16 (raw file):

	// Rewrite the memory estimate to get rid of potential discrepancies.
	f.mem.CopyMemEstimateFrom(from.Memo())

Why do we need this? If the new size is smaller then shouldn't we be using the new size? I think certain things are removed from the memo when it's detached and copied.

[nehageorge]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @rytaft)

---

pkg/sql/plan_opt.go, line 702 at r16 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Why did you change this?

If this is needed, seems worth turning this into another function called Copy or CopyMemo, with replaceNoopFn defined in factory.go (or instead of replaceNoopFn just use -- and possibly rename -- the existing function CopyScalarWithoutPlaceholders that does the same thing)

I was discussing with Marcus via Slack and he brought up that AssignPlaceholders could be misleading as that isn't what is being done. So this was an alternative. I renamed the function to CopyScalarNoop.

---

pkg/sql/opt/indexrec/index_candidate_set.go, line 348 at r19 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

should you add a test for a query that uses one virtual table and one non-virtual?

Done.

---

pkg/sql/opt/norm/factory.go, line 255 at r16 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Why do we need this? If the new size is smaller then shouldn't we be using the new size? I think certain things are removed from the memo when it's detached and copied.

Oh that makes sense. I thought it was an incorrect computation. Removed this function.

[rytaft]

Reviewed 3 of 7 files at r20, 1 of 3 files at r21, 3 of 3 files at r22, 2 of 2 files at r23, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner, @nehageorge, and @rytaft)

---

pkg/sql/plan_opt.go, line 702 at r16 (raw file):

Previously, nehageorge wrote…

I was discussing with Marcus via Slack and he brought up that AssignPlaceholders could be misleading as that isn't what is being done. So this was an alternative. I renamed the function to CopyScalarNoop.

How about CopyExprNoop? This memo will include non-scalar expressions, and there's no reason to limit the function to scalars since it operates on opt.Exprs

[rytaft]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner, @nehageorge, and @rytaft)

---

pkg/sql/plan_opt.go, line 702 at r16 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

How about CopyExprNoop? This memo will include non-scalar expressions, and there's no reason to limit the function to scalars since it operates on opt.Exprs

Or to be more explicit CopyWithoutAssigningPlaceholders. I think the "Scalar" piece was mostly what was distracting me. Anyway, up to you.

[nehageorge]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @rytaft)

---

pkg/sql/plan_opt.go, line 702 at r16 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Or to be more explicit CopyWithoutAssigningPlaceholders. I think the "Scalar" piece was mostly what was distracting me. Anyway, up to you.

Makes sense. I renamed the function.

[rytaft]

Reviewed 7 of 7 files at r24, 2 of 2 files at r25, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)

[mgartner]

Great work!

Reviewed 60 of 60 files at r13, 51 of 60 files at r14, 1 of 8 files at r16, 1 of 7 files at r20, 7 of 7 files at r24, 2 of 2 files at r25, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)

---

pkg/sql/opt/norm/factory.go, line 255 at r16 (raw file):

Previously, nehageorge wrote…

Oh that makes sense. I thought it was an incorrect computation. Removed this function.

I suggested this because the memo size in EXPLAIN (MEMO, VERBOSE) ... statements changed. Previously there was no detachment and copy step in EXPLAIN, so the memEstimate included sizes of expressions added to metadata. Now that we are copying the memo, memEstimate is reset to 0, and the EXPLAIN (MEMO, VERBOSE) ... memo size does not include the size of the metadata expressions. I believe this was causing the memo sizes in tests to change.

But I'm not sure how important this is, and my intuition is that it's not critical to have the previous behavior, so maybe we leave as-is?

[nehageorge]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)

---

pkg/sql/opt/norm/factory.go, line 255 at r16 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I suggested this because the memo size in EXPLAIN (MEMO, VERBOSE) ... statements changed. Previously there was no detachment and copy step in EXPLAIN, so the memEstimate included sizes of expressions added to metadata. Now that we are copying the memo, memEstimate is reset to 0, and the EXPLAIN (MEMO, VERBOSE) ... memo size does not include the size of the metadata expressions. I believe this was causing the memo sizes in tests to change.

But I'm not sure how important this is, and my intuition is that it's not critical to have the previous behavior, so maybe we leave as-is?

Makes sense. After evaluating the situation further, it seems like it's not critical to preserve the previous behaviour. Also, manually overwriting the memo size seems like it might be unnecessary. I'm going to leave it as is.

[nehageorge]

TFTRs!!

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)