Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: CREATE TABLE AS in an explicit txn does not flush batched rows #82951

Merged
merged 1 commit into from
Jun 21, 2022
Merged

sql: CREATE TABLE AS in an explicit txn does not flush batched rows #82951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions pkg/sql/create_table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,14 @@ func (n *createTableNode) startExec(params runParams) error {
break
}

// Periodically flush out the batches, so that we don't issue gigantic,
// raft commands.
if ti.currentBatchSize >= ti.maxBatchSize {
if err := tw.flushAndStartNewBatch(params.ctx); err != nil {
return err
}
}

// Populate the buffer.
copy(rowBuffer, n.sourcePlan.Values())

Expand Down
14 changes: 14 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/create_as
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,3 +382,17 @@ query I
SELECT * FROM tab_from_seq
----
2

# Regression test for #81554, where tried to do gigantic batches for CTAS in
# explicit transactions. Use a fixed command size, so that an error is decoupled
# fom the default size.
statement ok
SET CLUSTER SETTING kv.raft.command.max_size='5m'

statement ok
BEGIN;
CREATE TABLE source_tbl_huge AS SELECT 1::CHAR(256) FROM generate_series(1, 500000);
Copy link
Contributor

@postamar postamar Jun 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was curious and tried this on master and got

ERROR: command is too large: 126737600 bytes (max: 67108864)

Perhaps I'm too paranoid but those two values are too close for comfort. I'd be more comfortable if we explicitly set the kv.raft.command.max_size setting for this test to a specific value (like the floor value of 4MB) and explained why we're doing so in the comment for this test. This way we're not relying on the current default value for this setting. The test itself is great otherwise.

COMMIT;

statement ok
SET CLUSTER SETTING kv.raft.command.max_size to default