backfill: Fixed a bug where we ignore indexes to backfill #103476

Xiang-Gu · 2023-05-16T21:26:36Z

This PR contains three misc bug fixes/refactorings:

backfill: index backfill ignores the IndexToBackfill field but rather backfills all new indexes on the table
node formatting storage parameter: storage parameter was previously omitted in the formatting of certain node.
element result set filter: refactor to make it cleaner

They were discovered while working on #99526 but they are tangent enough to be of its own separate PR.
Epic: None

blathers-crl · 2023-05-16T21:26:41Z

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 · 2023-05-16T21:26:48Z

This change is

chengxiong-ruan

just nits, lgtm overall

chengxiong-ruan · 2023-05-17T15:04:03Z

pkg/sql/backfill/backfill.go

+// If `whitelist` is non-nil, we only allow those in this white list.
+func (ib *IndexBackfiller) initIndexes(desc catalog.TableDescriptor, whitelist []catid.IndexID) {
+	var whitelistAsSet catid.IndexSet
+	if whitelist != nil {


I think we should do len(whitelist) > 0 instead.

chengxiong-ruan · 2023-05-17T15:05:07Z

pkg/sql/sem/tree/create.go

@@ -1161,6 +1161,11 @@ func (node *UniqueConstraintTableDef) Format(ctx *FmtCtx) {
 			ctx.WriteString(" VISIBILITY " + fmt.Sprintf("%.2f", 1-invisibility))
 		}
 	}
+	if node.StorageParams != nil {


I think this worth some parser tests

chengxiong-ruan · 2023-05-17T15:06:59Z

pkg/sql/backfill/backfill.go

 	mon *mon.BytesMonitor,
 ) error {

 	// Initialize ib.added.
-	ib.initIndexes(desc)
+	ib.initIndexes(desc, indexesToBackfill)


Is there a way to test this? I think probably a unit test for the initIndexes method is sufficient, that logic here should trivial, but nice to make sure the method actually set thing right.

postamar

LGTM modulo some straightforward changes

postamar · 2023-05-17T14:56:31Z

pkg/sql/backfill/backfill.go

@@ -505,7 +506,7 @@ func (ib *IndexBackfiller) InitForLocalUse(
 ) error {

 	// Initialize ib.added.
-	ib.initIndexes(desc)
+	ib.initIndexes(desc, nil /* whitelist */)


s/whitelist/allowlist, applies elsewhere also

postamar · 2023-05-17T14:57:45Z

pkg/sql/backfill/backfill.go

@@ -640,11 +641,12 @@ func (ib *IndexBackfiller) InitForDistributedUse(
 	ctx context.Context,
 	flowCtx *execinfra.FlowCtx,
 	desc catalog.TableDescriptor,
+	indexesToBackfill []catid.IndexID,


s/indexesToBackfill/allowlist for consistency, also you might as well make this a catid.IndexSet here. This value comes from a protobuf which IIRC makes zero-length slices nil by default.

postamar · 2023-05-17T15:16:47Z

pkg/sql/backfill/backfill.go

+// If `whitelist` is non-nil, we only allow those in this white list.
+func (ib *IndexBackfiller) initIndexes(desc catalog.TableDescriptor, whitelist []catid.IndexID) {
+	var whitelistAsSet catid.IndexSet
+	if whitelist != nil {


nit: this nil-check is superfluous

postamar · 2023-05-17T15:19:15Z

pkg/sql/sem/tree/create.go

@@ -1161,6 +1161,11 @@ func (node *UniqueConstraintTableDef) Format(ctx *FmtCtx) {
 			ctx.WriteString(" VISIBILITY " + fmt.Sprintf("%.2f", 1-invisibility))
 		}
 	}
+	if node.StorageParams != nil {


postamar · 2023-05-17T15:19:51Z

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go

@@ -392,6 +392,22 @@ func isColumnFilter(_ scpb.Status, _ scpb.TargetStatus, e scpb.Element) bool {
 	return isColumn
 }

+func orFilter(
+	f1, f2 func(_ scpb.Status, _ scpb.TargetStatus, _ scpb.Element) bool,


nit: you could make orFilter variadic

fqazi

Reviewed 3 of 3 files at r1, 2 of 2 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @chengxiong-ruan, @rharding6373, and @Xiang-Gu)

pkg/sql/backfill/backfill.go line 644 at r1 (raw file):

	flowCtx *execinfra.FlowCtx,
	desc catalog.TableDescriptor,
	indexesToBackfill []catid.IndexID,

Probably that nil has a special meaning above to indicate that everything is whitelisted.

pkg/sql/sem/tree/create.go line 1164 at r2 (raw file):

Previously, postamar (Marius Posta) wrote…

agreed

+1

rharding6373

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

Xiang-Gu

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

pkg/sql/backfill/backfill.go line 509 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

s/whitelist/allowlist, applies elsewhere also

Done.

pkg/sql/backfill/backfill.go line 644 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

s/indexesToBackfill/allowlist for consistency, also you might as well make this a catid.IndexSet here. This value comes from a protobuf which IIRC makes zero-length slices nil by default.

changed the name to allowList but I don't think I can use a catid.IndexSet bc, like Faizan pointed out, a nil allowList means everything is allowed (the old behavior before my change), so we need something whose zero value is nil.

pkg/sql/backfill/backfill.go line 644 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Probably that nil has a special meaning above to indicate that everything is whitelisted.

Done.

pkg/sql/backfill/backfill.go line 649 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Is there a way to test this? I think probably a unit test for the initIndexes method is sufficient, that logic here should trivial, but nice to make sure the method actually set thing right.

done

pkg/sql/backfill/backfill.go line 737 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

nit: this nil-check is superfluous

nil means "everything is allowed" rather than "nothing is allowed". I made this decision so that nil input gives the old behavior before this change.

pkg/sql/backfill/backfill.go line 737 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

I think we should do len(whitelist) > 0 instead.

whitelist can be nil

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go line 396 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

nit: you could make orFilter variadic

done

pkg/sql/sem/tree/create.go line 1164 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

+1

It's implicitly tested because all statements ever appeared in a logic test will be invoked on VerifyStatementPrettyRoundtrip which utilize this code path in order to round-trip.

Xiang-Gu · 2023-05-25T15:40:55Z

bors r+

chengxiong-ruan · 2023-05-25T16:00:21Z

pkg/sql/backfill/backfill.go line 737 at r1 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

whitelist can be nil

len(nil) also returns 0

Xiang-Gu

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

pkg/sql/backfill/backfill.go line 737 at r1 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

len(nil) also returns 0

good point!

Xiang-Gu · 2023-05-25T16:22:18Z

bors r-

craig · 2023-05-25T16:22:20Z

Canceled.

Previously, when declarative schema changer emit BackfillIndex operation with a target index ID, the backfill logic ignores this and would backfill all existing adding indexes. This will cause problem as we are transitioning into a paradigm where we will have to construct multiple primary indexes to complete an schema change statement.

Previously, when we round-trip an ADD PRIMARY KEY statement with storage parameter, the storage parameter will be dropped. This PR fixes it.

This commit cleaned our element results set filters to be more principled.

Xiang-Gu · 2023-05-25T16:25:58Z

bors r+

craig · 2023-05-25T17:40:31Z

Build succeeded:

Bazel Essential CI (Cockroach)

Xiang-Gu force-pushed the bug/backfill-only-target branch from 7a7e701 to a5cf86e Compare May 16, 2023 22:56

Xiang-Gu marked this pull request as ready for review May 16, 2023 22:56

Xiang-Gu requested review from a team as code owners May 16, 2023 22:56

Xiang-Gu requested a review from rharding6373 May 16, 2023 22:56

Xiang-Gu mentioned this pull request May 16, 2023

Support ADD COLUMN, ALTER PRIMARY KEY, DROP COLUMN in one stmt #99526

Merged

chengxiong-ruan approved these changes May 17, 2023

View reviewed changes

postamar approved these changes May 17, 2023

View reviewed changes

fqazi approved these changes May 17, 2023

View reviewed changes

rharding6373 approved these changes May 17, 2023

View reviewed changes

Xiang-Gu force-pushed the bug/backfill-only-target branch from a5cf86e to 3feb5f9 Compare May 24, 2023 21:33

Xiang-Gu commented May 24, 2023

View reviewed changes

Xiang-Gu force-pushed the bug/backfill-only-target branch from 3feb5f9 to 83e07fc Compare May 25, 2023 15:00

Xiang-Gu commented May 25, 2023

View reviewed changes

Xiang-Gu added 3 commits May 25, 2023 12:25

tree: Fixed bug for formatting ADD PRIMARY KEY with storage param

a2545b4

Previously, when we round-trip an ADD PRIMARY KEY statement with storage parameter, the storage parameter will be dropped. This PR fixes it.

schemachanger: Refactor element result set filters

fef97d9

This commit cleaned our element results set filters to be more principled.

Xiang-Gu force-pushed the bug/backfill-only-target branch from 83e07fc to fef97d9 Compare May 25, 2023 16:25

craig bot merged commit 2060b07 into cockroachdb:master May 25, 2023

Xiang-Gu deleted the bug/backfill-only-target branch May 25, 2023 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backfill: Fixed a bug where we ignore indexes to backfill #103476

backfill: Fixed a bug where we ignore indexes to backfill #103476

Xiang-Gu commented May 16, 2023 •

edited

Loading

blathers-crl bot commented May 16, 2023

cockroach-teamcity commented May 16, 2023

chengxiong-ruan left a comment

chengxiong-ruan May 17, 2023

chengxiong-ruan May 17, 2023

postamar May 17, 2023

chengxiong-ruan May 17, 2023 •

edited

Loading

postamar left a comment

postamar May 17, 2023

postamar May 17, 2023

postamar May 17, 2023

postamar May 17, 2023

postamar May 17, 2023

fqazi left a comment

rharding6373 left a comment

Xiang-Gu left a comment

Xiang-Gu commented May 25, 2023

chengxiong-ruan commented May 25, 2023

Xiang-Gu left a comment

Xiang-Gu commented May 25, 2023

craig bot commented May 25, 2023

Xiang-Gu commented May 25, 2023

craig bot commented May 25, 2023

backfill: Fixed a bug where we ignore indexes to backfill #103476

backfill: Fixed a bug where we ignore indexes to backfill #103476

Conversation

Xiang-Gu commented May 16, 2023 • edited Loading

blathers-crl bot commented May 16, 2023

cockroach-teamcity commented May 16, 2023

chengxiong-ruan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chengxiong-ruan May 17, 2023 • edited Loading

Choose a reason for hiding this comment

postamar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

rharding6373 left a comment

Choose a reason for hiding this comment

Xiang-Gu left a comment

Choose a reason for hiding this comment

Xiang-Gu commented May 25, 2023

chengxiong-ruan commented May 25, 2023

Xiang-Gu left a comment

Choose a reason for hiding this comment

Xiang-Gu commented May 25, 2023

craig bot commented May 25, 2023

Xiang-Gu commented May 25, 2023

craig bot commented May 25, 2023

Xiang-Gu commented May 16, 2023 •

edited

Loading

chengxiong-ruan May 17, 2023 •

edited

Loading