sql: disable gossip-based physical planning by default

[yuzefovich]

Now that we improved handling of draining procedure in the virtual clusters in 24.3 timeframe, I believe it's time to begin fully deprecating the gossip-based physical planning, and as the first step this commit introduces a cluster setting to control which method is used in single-tenant deployments (instance-based planning being the default). I plan to have the setting as an escape hatch in case we find problems when rolling out this change and to remove the setting altogether after 25.2.

One of the differences between two planning methods is slightly adjusted in this commit. In particular, in the instance-based planning we stitch together consecutive spans whenever EnsureSafeSplitKey for the keys in the spans returns the same prefix. The idea is that spans corresponding to different column families of the same SQL row must be placed on the same instance. However, the condition on the length of the keys returned by EnsureSafeSplitKey is too broad and captures more cases than needed (i.e. two keys that cannot be part of the same SQL row end up in the same SpanPartition). To partially alleviate this difference, this commit introduces "boundary granularity" knob which indicates whether consecutive spans might be part of the same SQL row, and we now use the stitching logic only if so. All callers of PartitionSpans have been audited, and we need the stitching logic only in two call sites that correspond to planning of table readers. Longer term, if we change the encoding so that column family IDs are encoded in a special way, we'll be able to tell definitively whether stitching is needed simply based on the key itself, and this additional logic could be removed.

Epic: None

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[DrewKimball]

Nice!

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @yuzefovich)

---

pkg/sql/fingerprint_span.go line 121 at r1 (raw file):

	}

	spanPartitions, err := dsp.PartitionSpans(ctx, planCtx, []roachpb.Span{span}, PartitionSpansBoundDefault)

Note: we can reach here through the builtin function crdb_internal.fingerprint()

---

pkg/sql/distsql_physical_planner.go line 1567 at r1 (raw file):

			// address #112702, we'll be able to make the check here precise.
			if safeKey, err := keys.EnsureSafeSplitKey(span.Key); err == nil && len(safeKey) > 0 {
				if safeKey.Equal(lastKey) {

I know this is how it was already, but why don't we compare the key against the last end key (if it's set) instead? Won't this only catch the case when the preceding span covers a single row?

---

pkg/sql/distsql_physical_planner.go line 1569 at r1 (raw file):

				if safeKey.Equal(lastKey) {
					if log.V(1) {
						log.Infof(ctx, "stitchin span %s into the previous span partition", span)

[nit] stitchin -> stitching

[yuzefovich]

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

---

pkg/sql/distsql_physical_planner.go line 1567 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

I know this is how it was already, but why don't we compare the key against the last end key (if it's set) instead? Won't this only catch the case when the preceding span covers a single row?

Not sure I understand your question and proposal, could you please provide more context?

The current code works in the following way:

• if start keys for two consecutive spans have the same "safe split key", then we think that two spans might be from the same SQL row, so we must put the spans into the same spanPartition.
• the code assumes that it's only possible to break a single SQL row into multiple spans IF the original row-covering span didn't include any other rows. In other words, the code assumes that something like (roachpb.Span[row1_start, row2_middle], roachpb.Span[row2_middle, row2_end]) will never be passed as an argument to PartitionSpans. This is the case since we only operate at column-family-level spans in the span.Builder where we break a single-row-span into multiple spans when some CFs are not needed.

IIUC you're concerned about the case I described above, right?

---

pkg/sql/distsql_physical_planner.go line 1569 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

[nit] stitchin -> stitching

Done.

---

pkg/sql/fingerprint_span.go line 121 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Note: we can reach here through the builtin function crdb_internal.fingerprint()

Good point. This builtin was added in #91124 and suggests that the main usage for it is for internal testing of cluster-to-cluster replication. I looked over all the call sites, and it still remains the case in the code base, and we use this builtin for ensuring correctness of tenant replication, so way above the column families boundary within a single SQL row. It's possible that a user will issue a query with such a span argument that falls within a row, but it seems ok to me to ignore this case given that this is crdb_internal.* builtin that is not documented.

[DrewKimball]

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

---

pkg/sql/distsql_physical_planner.go line 1567 at r1 (raw file):

the code assumes that it's only possible to break a single SQL row into multiple spans IF the original row-covering span didn't include any other rows.

This is what I was missing - in that case, ignore my comment.

---

pkg/sql/fingerprint_span.go line 121 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Good point. This builtin was added in #91124 and suggests that the main usage for it is for internal testing of cluster-to-cluster replication. I looked over all the call sites, and it still remains the case in the code base, and we use this builtin for ensuring correctness of tenant replication, so way above the column families boundary within a single SQL row. It's possible that a user will issue a query with such a span argument that falls within a row, but it seems ok to me to ignore this case given that this is crdb_internal.* builtin that is not documented.

SGTM

[yuzefovich]

TFTR!

bors r+

[craig]

Build failed (retrying...):

• macos_amd64_build

[craig]

Build succeeded:

• local_roachtest_fips
• macos_amd64_build
• acceptance
• macos_arm64_build
• check_generated_code
• linux_arm64_build
• linux_amd64_build
• lint
• windows_build
• examples_orms
• linux_amd64_fips_build
• local_roachtest
• docker_image_amd64
• unit_tests