Repository navigation
Preserve rowid values in VACUUM INTO output - #5846
Conversation
| /// Column names for the current table being copied | ||
| current_table_columns: Vec<String>, | ||
| /// Unescaped column names for shadowed rowid alias detection | ||
| current_table_column_names: Vec<String>, |
There was a problem hiding this comment.
thanks for the feedback, I removed current_table_column_names and switched alias detection to existing schema metadata.
|
can you also add sqltest you could also add a compatibility test in tests/integration/query_processing/test_vacuum.rs showing how sqlite preserves and turso did not |
|
could u please add test cases for below scenarios too explicit INTEGER PRIMARY KEY just to be more safe as they have specific tests in sqlite too. |
There was a problem hiding this comment.
Pull request overview
This PR updates Limbo’s VACUUM INTO implementation to preserve hidden rowid values for ordinary rowid tables (matching SQLite behavior), and adds regression/compatibility coverage to prevent future rowid renumbering regressions.
Changes:
- Preserve hidden
rowidvalues duringVACUUM INTOtable-copy by explicitly selecting/inserting an available rowid alias (rowid,_rowid_,oid) when not shadowed by a real column. - Add integration tests covering rowid preservation (including shadowed rowid aliases) and compatibility comparisons against SQLite’s
VACUUM INTO. - Add runner-level SQL tests for
VACUUM INTOerror cases (empty path, disallowed in transaction).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
core/vdbe/execute.rs |
Adjusts the VACUUM table-copy SELECT/INSERT generation to include an accessible rowid alias so destination tables preserve rowid values. |
tests/integration/query_processing/test_vacuum.rs |
Adds integration/compat tests validating rowid preservation and behavior when rowid aliases are shadowed by user columns. |
testing/runner/tests/vacuum_into.sqltest |
Adds SQLTest coverage for VACUUM INTO argument/transaction error handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merging this PR will improve performance by 17.71%
Performance Changes
Comparing Footnotes
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let dest_dir = TempDir::new()?; | ||
| let dest_path = dest_dir.path().join("vacuumed_rowid.db"); | ||
| conn.execute(format!("VACUUM INTO '{}'", dest_path.to_str().unwrap()))?; | ||
|
|
There was a problem hiding this comment.
These new tests build the VACUUM INTO SQL by interpolating dest_path directly inside single quotes. This will break if the temp path contains a ' character (common on Windows when usernames contain apostrophes) and can make the test suite environment-dependent. Consider reusing escape_sqlite_string_literal() for the Turso VACUUM INTO path as well (or otherwise ensure proper SQL string literal escaping).
| turso_assert!( | ||
| idx < data_columns.len(), | ||
| "rowid alias column index out of bounds for table columns", | ||
| { "idx": idx, "columns_len": data_columns.len() } | ||
| ); | ||
| data_columns.remove(idx); | ||
| excluded_rowid_alias_column = true; |
There was a problem hiding this comment.
data_columns.remove(idx) is guarded by turso_assert!, which will panic in non-antithesis builds if schema/PRAGMA column ordering ever diverges (e.g., from corruption or future PRAGMA changes). Since VACUUM INTO can be run on user-provided DBs, prefer returning a LimboError::Corrupt/ParseError when the index is out of bounds instead of panicking.
| turso_assert!( | |
| idx < data_columns.len(), | |
| "rowid alias column index out of bounds for table columns", | |
| { "idx": idx, "columns_len": data_columns.len() } | |
| ); | |
| data_columns.remove(idx); | |
| excluded_rowid_alias_column = true; | |
| if idx < data_columns.len() { | |
| data_columns.remove(idx); | |
| excluded_rowid_alias_column = true; | |
| } |
This PR fixes `VACUUM INTO` so it preserves hidden rowid values for ordinary rowid tables instead of renumbering them in the destination database. Issue tursodatabase#5805 reported that `VACUUM INTO` produced a destination DB where rowids were renumbered (e.g. rowid `5` became `1`), while SQLite preserves rowid values for ordinary rowid tables. Closes tursodatabase#5805 Reviewed-by: Avinash Sajjanshetty (@avinassh) Closes tursodatabase#5846
…vascript' from app/dependabot Bumps [undici](https://github.com/nodejs/undici) from 8.10.0 to 8.11.2. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/nodejs/undici/releases">undici's releases</a>.</em></p> <blockquote> <h2>v8.11.2</h2> <h2>What's Changed</h2> <ul> <li>fix: close rejected HTTP/2 WebSocket handshake streams by <a href="https://github.com/mcollina"><code>@mcollina</code></a> in <a hre f="https://github.com/nodejs/undici/pull/5873">nodejs/undici#58 73</a></li> <li>fix: avoid reconnecting aborted requests by <a href="https://github.com/Junaid-PK"><code>@Junaid-PK</code></a> in <a h ref="https://github.com/nodejs/undici/pull/5846">nodejs/undici# 5846</a></li> </ul> <h2>New Contributors</h2> <ul> <li><a href="https://github.com/Junaid-PK"><code>@Junaid-PK</code></a> made their first contribution in <a href="https://github.com/nodejs/ undici/pull/5846">nodejs/undici#5846</a></li> </ul> <p><strong>Full Changelog</strong>: <a href="https://github.com/nodejs/undici/compare/v8 .11.1...v8.11.2">nodejs/undici@v8.11.1...v8.1 1.2</a></p> <h2>v8.11.1</h2> <h2>What's Changed</h2> <ul> <li>deslopify websocket test by <a href="https://github.com/KhafraDev"><code>@KhafraDev</code></a> in <a h ref="https://github.com/nodejs/undici/pull/5851">nodejs/undici# 5851</a></li> <li>Revert "fix: preserve HTTP/2 for legacy fetch consumers" by <a href="https://github.com/mcollina"><code>@mcollina</code></a> in <a hre f="https://github.com/nodejs/undici/pull/5860">nodejs/undici#58 60</a></li> <li>fix websocketstream bugs by <a href="https://github.com/KhafraDev"><code>@KhafraDev</code></a> in <a h ref="https://github.com/nodejs/undici/pull/5863">nodejs/undici# 5863</a></li> <li>fix hang on 3xx response with large body by <a href="https://github.com/KhafraDev"><code>@KhafraDev</code></a> in <a h ref="https://github.com/nodejs/undici/pull/5850">nodejs/undici# 5850</a></li> <li>docs: warn about buffering response bodies by <a href="https://github.com/mcollina"><code>@mcollina</code></a> in <a hre f="https://github.com/nodejs/undici/pull/5872">nodejs/undici#58 72</a></li> <li>test: cover native fetch gzip decoding through the legacy bridge by <a href="https://github.com/mcollina"><code>@mcollina</code></a> in <a hre f="https://github.com/nodejs/undici/pull/5874">nodejs/undici#58 74</a></li> </ul> <p><strong>Full Changelog</strong>: <a href="https://g ithub.com/nodejs/undici/compare/v8.11.0...v8.11.1">https://github.com/no dejs/undici/compare/v8.11.0...v8.11.1</a></p> <h2>v8.11.0</h2> <h2>What's Changed</h2> <ul> <li>fix: complete upgrade diagnostics lifecycle by <a href="https://github.com/BridgeAR"><code>@BridgeAR</code></a> in <a hre f="https://github.com/nodejs/undici/pull/5761">nodejs/undici#57 61</a></li> <li>build(deps): bump step-security/harden-runner from 2.19.4 to 2.21.0 by <a href="https://github.com/dependabot"><code>@dependabot</code></a>[bot] in <a href="https://github.com/nodejs/undici/pull/5753">nodejs/ undici#5753</a></li> <li>fix(decompress): join a repeated content- encoding header by <a href="https://github.com/luantaraschi"><code>@luantaraschi</code></a> in <a href="https://github.com/nodejs/undici/pull/5664">nodejs/ undici#5664</a></li> <li>fix(cookies): keep a cookie named <strong>proto</strong> in getCookies by <a href="https://github.com/luantaraschi"><code>@luantaraschi</code></a> in <a href="https://github.com/nodejs/undici/pull/5663">nodejs/ undici#5663</a></li> <li>build(deps-dev): bump jest from 30.5.0 to 30.5.1 by <a href="https://github.com/dependabot"><code>@dependabot</code></a>[bot] in <a href="https://github.com/nodejs/undici/pull/5784">nodejs/ undici#5784</a></li> <li>build(deps): bump got from 15.1.0 to 16.0.0 in /benchmarks by <a href="https://github.com/dependabot"><code>@dependabot</code></a>[bot] in <a href="https://github.com/nodejs/undici/pull/5785">nodejs/ undici#5785</a></li> <li>fix: selectively re-enable SIMD for ppc64 by <a href="https://github.com/richardlau"><code>@richardlau</code></a> in <a href="https://github.com/nodejs/undici/pull/5776">nodejs/undici #5776</a></li> <li>use private fields for brand checks by <a href="https://github.com/KhafraDev"><code>@KhafraDev</code></a> in <a h ref="https://github.com/nodejs/undici/pull/5792">nodejs/undici# 5792</a></li> <li>fix(h2): do not unref a session with open streams by <a href="https://github.com/kjsik11"><code>@kjsik11</code></a> in <a hr ef="https://github.com/nodejs/undici/pull/5782">nodejs/undici#5 782</a></li> <li>fix(h2): avoid pool fan-out during ALPN negotiation by <a href="https://github.com/mcollina"><code>@mcollina</code></a> in <a href="https://github.com/nodejs/undici/pull/5799">nodejs/undici #5799</a></li> <li>fix issue <a href="https://github.com/nodejs/undici/issues/5715">#5715</a> by <a href="https://github.com/KhafraDev"><code>@KhafraDev</code></a> in <a href="https://github.com/nodejs/undici/pull/5805">nodejs/ undici#5805</a></li> <li>ci: extend macOS Node.js workflow timeout by <a href="https://github.com/mcollina"><code>@mcollina</code></a> in <a hre f="https://github.com/nodejs/undici/pull/5775">nodejs/undici#57 75</a></li> <li>fix(env-http-proxy-agent): NO_PROXY wildcard semantics by <a href="https://github.com/mcollina"><code>@mcollina</code></a> in <a href="https://github.com/nodejs/undici/pull/5777">nodejs/und ici#5777</a></li> <li>fix(h2): do not replay a request whose response already started after GOAWAY by <a href="https://github.com/kjsik11"><code>@kjsik11</code></a> in <a href= "https://github.com/nodejs/undici/pull/5788">nodejs/undici#5788 </a></li> <li>test: skip HTTP/2 churn test on Node.js 25 on all platforms by <a href="https://github.com/kjsik11"><code>@kjsik11</code></a> in <a href= "https://github.com/nodejs/undici/pull/5790">nodejs/undici#5790 </a></li> <li>fix(proxy-agent): preserve repeated iterable headers by <a href="https://github.com/GiHoon1123"><code>@GiHoon1123</code></a> in <a href="https://github.com/nodejs/undici/pull/5779">nodejs/undici #5779</a></li> <li>docs(errors): document ProxyConnectionError (UND_ERR_PRX_CONN) by <a href="https://github.com/kilisamemarisaaa"><cod e>@kilisamemarisaaa</code></a> in <a href="https://github.com/ nodejs/undici/pull/5781">nodejs/undici#5781</a></li> <li>dataurl parsing improvements + update WPT by <a href="https://github.com/KhafraDev"><code>@KhafraDev</code></a> in <a h ref="https://github.com/nodejs/undici/pull/5812">nodejs/undici# 5812</a></li> <li>fix(decompress): treat inherited Object.prototype names as unsupported encodings by <a href="https://github.com/NgoQuocVie t2001"><code>@NgoQuocViet2001</code></a> in <a href="https://redirect.g ithub.com/nodejs/undici/pull/5807">nodejs/undici#5807</a></li> <li>test(h2): stabilize http2-connection by <a href="https://github.com/ViniciusDev26"><code>@ViniciusDev26</code></a> in <a href="https://github.com/nodejs/undici/pull/5801">nodejs/ undici#5801</a></li> <li>fix: preserve HTTP/2 for non-upgrade legacy requests by <a href="https://github.com/mcollina"><code>@mcollina</code></a> in <a hre f="https://github.com/nodejs/undici/pull/5811">nodejs/undici#58 11</a></li> <li>fix: skip drain dispatch for clientTtl-evicted clients in PoolBase by <a href="https://github.com/vwong"><code>@vwong</code></a> in <a href="htt ps://github.com/nodejs/undici/pull/5677">nodejs/undici#5677</a> </li> <li>chore: update tests blocking release by <a href="https://github.com/vwong"><code>@vwong</code></a> in <a href="htt ps://github.com/nodejs/undici/pull/5828">nodejs/undici#5828</a> </li> <li>build(deps): bump uWebSockets.js from v20.68.0 to v20.70.0 in /benchmarks by <a href="https://github.com/dependabot"><code>@dependabot</code></a>[bot] in <a href="https://github.com/nodejs/undici/pull/5824">nodejs/ undici#5824</a></li> <li>fix(deduplicate): settle requests when composed with retry by <a href="https://github.com/mcollina"><code>@mcollina</code></a> in <a hre f="https://github.com/nodejs/undici/pull/5817">nodejs/undici#58 17</a></li> <li>fix(cache): do not report aborted on successful cache hits by <a href="https://github.com/Yahiro025"><code>@Yahiro025</code></a> in <a h ref="https://github.com/nodejs/undici/pull/5819">nodejs/undici# 5819</a></li> </ul> <!-- raw HTML omitted --> </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="nodejs/undici@7e016ad7e5bd6540170 069a9a28b1e633b29664f"><code>7e016ad</code></a> Bumped v8.11.2 (<a href= "https://github.com/nodejs/undici/issues/5886">#5886</a>)</li> <li><a href="nodejs/undici@fcdd5a615ab49beda87 aafe14ad30106cfe3b0d5"><code>fcdd5a6</code></a> fix: skip reconnecting for aborted requests (<a href="https://github.com/nodejs/undici /issues/5846">#5846</a>)</li> <li><a href="https://github.com/nodejs/und ici/commit/dd28c5c55f4a25f7c441fd4365b1191f96c3b8bf"><code>dd28c5c</code ></a> fix: close rejected HTTP/2 WebSocket handshake streams (<a href="h ttps://github.com/nodejs/undici/issues/5873">#5873</a>)</li> <li><a href="nodejs/undici@5e0a2f8e02be8f1155f eab2c52f55731d3bb84a4"><code>5e0a2f8</code></a> Bumped v8.11.1 (<a href= "https://github.com/nodejs/undici/issues/5871">#5871</a>)</li> <li><a href="nodejs/undici@2b728989ac5b9d247ec a2094bb264ced76f7cef6"><code>2b72898</code></a> test: cover native fetch gzip decoding through the legacy bridge (<a href="https://redirect.githu b.com/nodejs/undici/issues/5874">#5874</a>)</li> <li><a href="https://gi thub.com/nodejs/undici/commit/636bf515c0c8497dd0ca68917e4e38577c27669e"> <code>636bf51</code></a> docs: warn about buffering response bodies (<a href="https://github.com/nodejs/undici/issues/5872">#5872</a>)< /li> <li><a href="nodejs/undici@8145595e221779 980d91ff611f14e5a984661388"><code>8145595</code></a> fix hang on 3xx response with large body (<a href="https://github.com/nodejs/un dici/issues/5850">#5850</a>)</li> <li><a href="https://github.com/nodejs /undici/commit/f3cbe486706da6163b4ca226022e74df73168dd5"><code>f3cbe48</ code></a> fix websocketstream bugs (<a href="https://github.com /nodejs/undici/issues/5863">#5863</a>)</li> <li><a href="https://github. com/nodejs/undici/commit/1d4ea0a883bf33275d00c4db55377892180b2747"><code >1d4ea0a</code></a> Revert "fix: preserve HTTP/2 for legacy fetch consumers" (<a href="https://github.com/nodejs/undici/issu es/5860">#5860</a>)</li> <li><a href="https://github.com/nodejs/undici/c ommit/77d7b3f8dd2d8114b686e32d8d28bd3dde06bc33"><code>77d7b3f</code></a> deslopify websocket test (<a href="https://github.com/nodejs/un dici/issues/5851">#5851</a>)</li> <li>Additional commits viewable in <a href="https://github.com/nodejs/undici/compare/v8.10.0...v8.11.2">compar e view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security- vulnerabilities/about-dependabot-security-updates#about-compatibility- scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/tursodatabase/turso/network/alerts). </details> Closes #9861
Description
This PR fixes
VACUUM INTOso it preserves hidden rowid values for ordinary rowid tables instead of renumbering them in the destination database.Motivation and context
Issue #5805 reported that
VACUUM INTOproduced a destination DB where rowids were renumbered (e.g. rowid5became1), while SQLite preserves rowid values for ordinary rowid tables.Closes #5805
Description of AI Usage