sql: add bulkio.column_backfill.update_chunk_size_threshold_bytes #83544

postamar · 2022-06-28T21:38:28Z

This commit adds this cluster setting, defaulting to 10 MiB, which
complements the bulkio.column_backfill.batch_size setting to limit the
size of the column backfiller update batches.

Fixes #83199.

Release note (bug fix): fixes bug where ADD/DROP COLUMN with the legacy
schema changer could fail on tables with large rows due to exceeding the
raft command max size.

cockroach-teamcity · 2022-06-28T21:38:35Z

This change is

postamar · 2022-06-29T11:00:41Z

pkg/sql/backfill.go

+// an update batch is run at once when adding or removing columns.
+var columnBackfillUpdateRunThresholdBytes = settings.RegisterIntSetting(
+	settings.TenantWritable,
+	"bulkio.column_backfill.update_run_threshold_bytes",


I've been struggling to name this, happy to take any suggestion.

I know this is hard... but I'm taking a shot...columnBackfillUpdateBatchThreshold ?

That sounds like an improvement. I think we quite like this bytes suffix for these kinds of values, though, I see them a lot, it must be some kind of style convention.

postamar · 2022-06-29T11:01:23Z

pkg/sql/backfill.go

+	settings.TenantWritable,
+	"bulkio.column_backfill.update_run_threshold_bytes",
+	"the batch size in bytes above which an update is immediately run when adding/removing columns",
+	10<<20,                  /* 10 MiB */


I took this default value because that's also the default max size when scanning.

postamar · 2022-06-29T11:03:05Z

pkg/sql/backfill/backfill.go

+		//
+		if updateRunThresholdBytes > 0 && b.ApproximateMutationBytes() >= int(updateRunThresholdBytes) {
+			break
+		}


I found that a threshold was more convenient than an absolute max size, both because it was easier to implement and because then I wouldn't have to worry about batches with zero puts, in the case where one put is just that big. What do we do then? not update the row? It didn't make sense, so I used a threshold.

sounds good to me

okay, so, in addition to the old 64 MiB, you put this inside the for loop to say that if the to-be-put batch exceeds the threshold then break and flush, right?

Almost, it's in addition to the number-of-rows limit (default 200). The 64 MiB limit we've changed in the customer support issues doesn't come into play here, that's a hard limit on the size of raft commands, which if exceeded causes the schema change to be blocked. This new setting I'm adding helps ensure we never reach that hard limit.

yeah, my mistake, I meant the 200-row limit. Thanks for your explanation!

postamar · 2022-06-29T11:05:08Z

pkg/sql/logictest/testdata/logic_test/alter_table

+10
+
+statement ok
+ROLLBACK;


This subtest only exercises in-txn column backfills, I couldn't figure out how to retrieve the traces in the other case. Those traces are emitted, according to the debugger at least, but I couldn't collect them. Anyway I figured that this would be enough.

Yeah, I like the tests you added as well.

Can we also have a "correctness" test? Namely, if set this threshold to be small and add a column, we expect it to use >1 batch and succeed by being able to observe the newly added column with the correct default value.

I'm not sure it's that valuable, we already arbitrarily break down batches in 200-row increments, but it also doesn't cost anything, so I'll add a SELECT.

postamar · 2022-06-29T11:05:48Z

pkg/sql/logictest/testdata/logic_test/alter_table

-205       test_serial_b_seq  PUBLIC  204
-204       test_serial        PUBLIC  NULL
+207       test_serial_b_seq  PUBLIC  206
+206       test_serial        PUBLIC  NULL


These and the others all get bumped because my subtest non-transactionally increments the descriptor ID counter twice.

why don't we put your subtest at the end of the file?

In a sane world I would, however, doing so often causes annoying merge conflicts when backporting.

postamar · 2022-06-29T11:06:43Z

pkg/sql/rowexec/columnbackfiller.go

-				false, /*traceKV*/
+				updateRunThresholdBytes,
+				true, /*alsoCommit*/
+				cb.flowCtx.TraceKV,


I figure I'd leave this change in, as the debugger assures me that this flag is set when SET tracing = on,kv.

chengxiong-ruan

LGTM

chengxiong-ruan · 2022-06-29T13:57:22Z

pkg/sql/backfill.go

+// an update batch is run at once when adding or removing columns.
+var columnBackfillUpdateRunThresholdBytes = settings.RegisterIntSetting(
+	settings.TenantWritable,
+	"bulkio.column_backfill.update_run_threshold_bytes",


I know this is hard... but I'm taking a shot...columnBackfillUpdateBatchThreshold ?

chengxiong-ruan · 2022-06-29T14:00:33Z

pkg/sql/backfill/backfill.go

+		//
+		if updateRunThresholdBytes > 0 && b.ApproximateMutationBytes() >= int(updateRunThresholdBytes) {
+			break
+		}


sounds good to me

chengxiong-ruan · 2022-06-29T14:03:46Z

pkg/sql/logictest/testdata/logic_test/alter_table

+10
+
+statement ok
+ROLLBACK;


Yeah, I like the tests you added as well.

ajwerner

Reviewed 5 of 7 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @postamar and @rhu713)

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

Previously, postamar (Marius Posta) wrote…

That sounds like an improvement. I think we quite like this bytes suffix for these kinds of values, though, I see them a lot, it must be some kind of style convention.

I prefer Batch to Run or Update, and I like the bytes suffix. The other term used in the code is Chunk, which I also like, maybe the most: columnBackfillChunkSizeThresholdBytes?

There was an incident once involving a bytes threshold and a customer assuming megabytes. It was bad.

This commit adds this cluster setting, defaulting to 10 MiB, which complements the bulkio.column_backfill.batch_size setting to limit the size of the column backfiller update batches. Fixes cockroachdb#83199. Release note (bug fix): fixes bug where ADD/DROP COLUMN with the legacy schema changer could fail on tables with large rows due to exceeding the raft command max size.

postamar · 2022-06-30T20:16:03Z

bors r+

postamar · 2022-06-30T20:16:14Z

Thanks for the reviews

craig · 2022-06-30T22:31:53Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-07-01T02:03:28Z

Build succeeded:

GitHub CI (Cockroach)

blathers-crl · 2022-07-01T02:03:39Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 119dc67 to blathers/backport-release-21.2-83544: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2.x failed. See errors above.

error creating merge commit from 119dc67 to blathers/backport-release-22.1-83544: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

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

postamar · 2022-07-04T10:43:51Z

We can't backport this PR as it modifies the backfiller spec protobuf.

postamar · 2022-07-05T15:13:55Z

According to @ajwerner it's fine to backport. I'll do so shortly.

postamar force-pushed the fix-83199 branch 2 times, most recently from baccb3a to 103452c Compare June 29, 2022 10:59

postamar commented Jun 29, 2022

View reviewed changes

postamar requested review from ajwerner, a team and rhu713 June 29, 2022 11:07

postamar marked this pull request as ready for review June 29, 2022 11:07

postamar requested a review from a team June 29, 2022 11:07

postamar requested a review from a team as a code owner June 29, 2022 11:07

postamar added backport-21.2.x labels Jun 29, 2022

chengxiong-ruan approved these changes Jun 29, 2022

View reviewed changes

ajwerner approved these changes Jun 29, 2022

View reviewed changes

postamar force-pushed the fix-83199 branch from 103452c to 431295c Compare June 30, 2022 14:21

postamar force-pushed the fix-83199 branch from 431295c to 119dc67 Compare June 30, 2022 14:24

postamar changed the title ~~sql: add bulkio.column_backfill.update_run_threshold_bytes cluster se…~~ sql: add bulkio.column_backfill.update_chunk_size_threshold_bytes Jun 30, 2022

Xiang-Gu approved these changes Jun 30, 2022

View reviewed changes

craig bot merged commit e2767f9 into cockroachdb:master Jul 1, 2022

postamar deleted the fix-83199 branch July 4, 2022 10:43

postamar removed backport-21.2.x labels Jul 4, 2022

This was referenced Jul 5, 2022

release-22.1: sql: add bulkio.column_backfill.update_chunk_size_threshold_bytes #83816

Merged

release-21.2: sql: add bulkio.column_backfill.update_chunk_size_threshold_bytes #83817

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add bulkio.column_backfill.update_chunk_size_threshold_bytes #83544

sql: add bulkio.column_backfill.update_chunk_size_threshold_bytes #83544

postamar commented Jun 28, 2022 •

edited

Loading

cockroach-teamcity commented Jun 28, 2022

postamar Jun 29, 2022

chengxiong-ruan Jun 29, 2022

postamar Jun 29, 2022

postamar Jun 29, 2022

postamar Jun 29, 2022

chengxiong-ruan Jun 29, 2022

Xiang-Gu Jun 30, 2022

postamar Jun 30, 2022

Xiang-Gu Jun 30, 2022

postamar Jun 29, 2022

chengxiong-ruan Jun 29, 2022

Xiang-Gu Jun 30, 2022

postamar Jun 30, 2022

postamar Jun 29, 2022

Xiang-Gu Jun 30, 2022

postamar Jun 30, 2022

postamar Jun 29, 2022

chengxiong-ruan left a comment

chengxiong-ruan Jun 29, 2022

chengxiong-ruan Jun 29, 2022

chengxiong-ruan Jun 29, 2022

ajwerner left a comment

postamar commented Jun 30, 2022

postamar commented Jun 30, 2022

craig bot commented Jun 30, 2022

craig bot commented Jul 1, 2022

blathers-crl bot commented Jul 1, 2022

postamar commented Jul 4, 2022

postamar commented Jul 5, 2022

sql: add bulkio.column_backfill.update_chunk_size_threshold_bytes #83544

sql: add bulkio.column_backfill.update_chunk_size_threshold_bytes #83544

Conversation

postamar commented Jun 28, 2022 • edited Loading

cockroach-teamcity commented Jun 28, 2022

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

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

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

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

ajwerner left a comment

Choose a reason for hiding this comment

postamar commented Jun 30, 2022

postamar commented Jun 30, 2022

craig bot commented Jun 30, 2022

craig bot commented Jul 1, 2022

blathers-crl bot commented Jul 1, 2022

postamar commented Jul 4, 2022

postamar commented Jul 5, 2022

postamar commented Jun 28, 2022 •

edited

Loading