Increase SQLITE_LIMIT_VDBE_OP to 250000 from 25000

[joshthoward]

No description provided.

[clanky-the-clanker]

💬 Approved with comments — A small, targeted change that raises SQLITE_LIMIT_VDBE_OP tenfold and adds a regression test. The only finding is a SUGGESTION: confirm the test fails at the old limit, and record why 250000 was chosen.

Findings: 1 suggestion

_{The decision is advisory: this comment never approves, blocks or requests changes on the pull request.}

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #7790.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## SUGGESTION (1)

### [Code Quality] Confirm the new quick_check regression test (100 tables, one index each) actually fails with the old 25000 limit, and note why 250000 was chosen for this security limit.
- **File:** src/workerd/util/sqlite-test.c++:1802
- **Issue:** Confirm the new quick_check regression test (100 tables, one index each) actually fails with the old 25000 limit, and note why 250000 was chosen for this security limit.

---

📋 Code Quality: 💬 1 comment

The limit change is correct. The test may not fail at the old limit, and the PR doesn't say why 250000 was chosen.

Recommendation: 💬 Optional Nits

• [SUGGESTION] Confirm the new quick_check regression test (100 tables, one index each) actually fails with the old 25000 limit, and note why 250000 was chosen for this security limit. (src/workerd/util/sqlite-test.c++:1802)

View full review

• src/workerd/util/sqlite-test.c++:1802-1820: the new test builds 100 tables, each with one index, then runs PRAGMA quick_check. This review couldn't run the test, so it's unclear whether that schema actually produces a VDBE program larger than 25000 ops. If it doesn't, the test passes with the old limit and doesn't guard the regression. Run it once against 25000 to confirm it fails. If it passes, increase the table or index count until the program is over 25000 ops and still well under 250000.
• src/workerd/util/sqlite.c++:1450: SQLITE_LIMIT_VDBE_OP is set in setupSecurity() as a hardening limit, and the PR description is empty. A short note on why 250000 was chosen would help future readers, either in the commit message or in a comment next to the call. The note should also confirm that memory metering and CPU limits still bound the cost of a single prepare or step.

The new test comment describes the code as it is now, as the repository's comment guidelines ask. The test uses KJ types throughout.

---

✅ All other checks passed

📝 Documentation: ✅ LGTM

No documentation issues. The new test comment explains the current behavior clearly.

---

_{Reviewed commit: 9426fffe}

[ask-bonk]

Since last review: 0 resolved, 1 still open, 0 new.

The revised patch only reformats the SQLite quick_check regression fixture.

Not re-run: docs, design-simplicity (no author changes in their files since the last review)

---

_{Reviewed commit: 9426fffe · github run}

[ask-bonk]

[WARNING] This never exercises the previous 25,000-op ceiling: running the same 100-table schema with .limit vdbe_op 25000 returned ok in SQLite 3.45.1, so reverting the production limit would still pass this test. Grow the fixture to a size demonstrated to fail at the old cap (500 table/index pairs fails there and passes at 250,000 in that version).

Suggested change

	for (auto i: kj::zeroTo(100)) {
	for (auto i: kj::zeroTo(500)) {

[RossRBM]

Reviewing head f083b58: I checked the corresponding generic D1 fixture on unchanged official workerd 1.20261012.1 and 1.20261016.1 (Miniflare5.20261012.1-beta, Node24.15.0).

Both released runtimes produce:

Empty table/index pairs	Global quick_check	D1 EXPLAIN instructions
100, matching the current PR schema	ok	3,306
500, same schema family	ok	16,106
1,000, same schema family	SQLITE_NOMEM	preparation also fails

Each table uses exactly id INTEGER PRIMARY KEY, a TEXT NOT NULL, b INTEGER, c BLOB, plus its index on a. Every trial creates a fresh transient instance. Negative NULL inserts are refused, all counts remain zero, telemetry is disabled and outbound calls are forbidden. A separate constrained-column negative control from #7439 still fails at the known boundary with CHECK enforcement intact. The complete portable probe ran eight controlled trials.

On these released D1 versions, increasing the corresponding fixture to500 still does not demonstrate the old-limit failure. Could the C++ regression be verified to fail with25,000 and pass with250,000, using a fixture such as the1,000-table case or the constrained-column family? These D1 observations do not themselves execute or validate the C++ unit test or this unmerged candidate.

Separate native SQLite3.51.3 completes all checks; its1,000-table EXPLAIN has32,100 instructions. That is not a count measured from failed D1 preparation, original-file verification or a hosted engine measurement.

The bounded engine change is a promising candidate. Once accepted, could the fixed official release and supported hosted-D1 availability/qualification procedure be identified? We retain unchanged whole-database integrity/capture checks and have not executed an unmerged/custom engine, altered constraints, changed a deployed migration or performed a new hosted operation. No private schema, application data, database/account identifiers or credentials are included.