oxidecomputer / oxidecomputer/omicron

populate() submits dbinit.sql as one batch, putting SET CLUSTER SETTING inside an implicit transaction — the comment above it asserts the opposite

Open
#11,267 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
572
Forks
97
Avg merge
2d 12h
Merged PRs (30d)
96

Description

Description

schema/crdb/dbinit.sql opens with a cluster setting and a comment explaining
why it is placed where it is:

/*
 * Reduce the index backfill batch size from the default of 50,000 to 5,000.
 * ...
 * This must be outside the transaction because SET CLUSTER SETTING cannot be
 * used inside a transaction.
 */
SET CLUSTER SETTING bulkio.index_backfill.batch_size = 5000;

BEGIN;

The statement is indeed outside the explicit BEGIN. But
test-utils/src/dev/db.rs:938 submits the whole file in a single call:

pub async fn populate(
    client: &tokio_postgres::Client,
) -> Result<(), anyhow::Error> {
    let sql = include_str!("../../../schema/crdb/dbinit.sql");
    client.batch_execute(sql).await.context("populating Omicron database")
}

batch_execute sends the file as one simple query. A multi-statement simple
query runs as an implicit transaction. So the statement is outside the
explicit transaction and inside an implicit one, and the comment's guarantee
does not hold — it holds only because the CockroachDB version currently pinned
(v22.1.22) tolerates it.

On CockroachDB 22.2 the same file fails:

failed to populate database: populate

Caused by:
    0: populating Omicron database
    1: db error
    2: ERROR: SET CLUSTER SETTING cannot be used inside a multi-statement transaction

I hit this while running the simulated stack against 22.2, but the reason to
report it is not my configuration — it is that the safety property the comment
claims is not actually provided by the code, and the next CockroachDB major
version bump will surface that. The comment is load-bearing documentation for
anyone editing dbinit.sql; someone adding a second SET CLUSTER SETTING
alongside the first would reasonably trust it.

Steps to reproduce

  1. Point omicron at a CockroachDB 22.2 binary (update tools/cockroachdb_version
    so the startup version check passes).
  2. cargo xtask omicron-dev run-all

Expected result

SET CLUSTER SETTING executes outside any transaction, as the comment states,
and schema population proceeds.

Actual result

Population aborts on the first statement of the file. CockroachDB 22.1 accepts
it; 22.2 does not.

Suggested fix

Execute statements that cannot run in a transaction on their own, rather than
relying on their position within the file. For example, in populate():

let mut rest = String::with_capacity(sql.len());
for line in sql.lines() {
    if line.trim_start().to_uppercase().starts_with("SET CLUSTER SETTING") {
        client.batch_execute(line).await.with_context(|| {
            format!("applying cluster setting: {line}")
        })?;
    } else {
        rest.push_str(line);
        rest.push('\n');
    }
}
client.batch_execute(&rest).await.context("populating Omicron database")

A tidier variant would be a dedicated list of pre-transaction statements, or a
sentinel comment in the SQL marking the split point, so the behaviour is
explicit rather than inferred from a prefix match.

At minimum, the comment in dbinit.sql should be corrected to say that the
statement's placement is safe only because batch_execute's implicit
transaction currently accepts it on the pinned version.

The same consideration applies to wipe() (dbwipe.sql), which uses the same
batch_execute pattern.

Environment

omicron df990b0578fbee4afcf805423a20d85e23544e0d (2026-09-04)
cockroachdb v22.2.19 (pinned version is v22.1.22-64-g86fdbfca06)
platform aarch64-unknown-linux-gnu, Ubuntu 24.04
mode simulated (cargo xtask omicron-dev run-all)

Not architecture-specific: the same failure occurs on any platform running
CockroachDB 22.2.


Disclosure: this issue was investigated and written up with AI assistance
(Claude). Everything in it was measured rather than inferred — the timings,
error output, version numbers and reproduction steps are all from real runs on
real hardware, and where a fix is suggested it is one I am actually running. I
have read it through before filing. Happy to clarify anything or test a patch.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with populate() at test-utils/src/dev/db.rs:938 and the transaction structure in schema/crdb/dbinit.sql; reproduce with CockroachDB 22.2 using cargo xtask omicron-dev run-all. Check the related wipe() path and dbwipe.sql as well. Done means schema population succeeds with SET CLUSTER SETTING outside a transaction and the documented safety guarantee is accurate.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust, sql
Domain
databases
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.