odbc_copy: bind once per stable single-row shape (re: #163 item 2) - #168
Conversation
|
Self-review tightened a few rough edges in a5f8ade:
CI: same 12 / 2 split as before — same two pre-existing failures (MSSQL Linux libmsodbcsql version mismatch, Firebird Windows DST drift) noted in the PR description; both still fail on Marking ready for review. |
staticlibs
left a comment
staticlibs
left a comment
There was a problem hiding this comment.
Thanks for the PR! I think it is a very nice optimization and is doing exactly what the scanner intends to do well - using ODBC drivers as effective as possible. Just if it is possible to use it with batches too and to only re-bind non-fixed buffers (where possible) - I think it can be even more effective.
|
I Will work on all this first thing tomorrow 👍🏻 |
Addresses the four review items on PR duckdb#168: 1. Per-slot rebind (was all-or-nothing). `Params::BindToOdbcWithCache` now issues SQLBindParameter only for slots whose shape changed since the cached bind, or whose backing buffer is variable-length (DecimalChars / WideString / ScannerBlob / ScannerUuid — the std::vector inside can move on reassignment). Fixed-width siblings on the same row keep their existing binding. A row of (INT, VARCHAR, INT) now rebinds only the VARCHAR slot instead of all three. 2. Caching extended to the batch path. `flat_batch` is hoisted to `LocalInitData` (matching `single_row_buf`) so its slot addresses are stable across CopyInTransaction calls. The batch branch now goes through BindToOdbcWithCache against `batch_bind_cache`, hitting the same per-slot reuse logic. Both caches are reset together at every prepared-statement transition. 3. SQLFreeStmt(SQL_CLOSE) is now called unconditionally before each SQLExecute. The previous gating on `reset_stmt_before_execute || reused` only existed because rebinding implicitly cleared cursor state — with the per-slot path that is no longer guaranteed, and the spec makes SQL_CLOSE on a statement with no open cursor a no-op, so always closing simplifies the loop and removes the `reused_bindings` flag. 4. `BindSlotShape` got a (type_id, expected_type) constructor; the field-by-field assignment in the bind loop is gone. `BindCache::initialized` is also dropped — `cache.shape.size() != params.size()` is the new "first call" signal and the resize-and-clear handles it.
|
Done! #1 ( #2 (caching for batch): done. #3 (unconditional #4 (per-slot rebind): this is the big one and the right call. |
The per-row execute path in odbc_copy called Params::SetExpectedTypes + Params::BindToOdbc for every row, which collapsed to a SQLFreeStmt(SQL_RESET_PARAMS) + N × SQLBindParameter cycle on every SQLExecute. That is legal ODBC, but it is the least efficient of the three common execute shapes and — as the Firebird silent-corruption bug in duckdb#161 showed — it is also the shape most likely to trip driver bugs that rely on sqlvar state being stable across executes. This change caches the bound-parameter shape in LocalInitData and skips the rebind when the next row has the same (type_id, expected_type, is_null) per slot and every slot is fixed-width. The first row still pays the full bind; subsequent rows just update the values at the already-bound addresses. single_row_buf now lives on LocalInitData so the ScannerValue slot addresses stay stable across CopyInTransaction calls — that is what makes reusing the previous SQLBindParameter pointer safe. Variable-length slots (TYPE_DECIMAL_AS_CHARS, VARCHAR via WideString, BLOB, UUID, ...) still force a rebind because their backing std::vectors can reallocate when the value changes. Two additional fixes land with this: 1. When the bindings are reused we call SQLFreeStmt(SQL_CLOSE) before the next SQLExecute. Without it, the DuckDB ODBC driver leaves the statement in a state that makes a later SQLEndTran fail with SQLSTATE HY010 "Function sequence error"; rebinding implicitly cleared that state, reusing bindings does not. This matches what the existing reset_stmt_before_execute quirk does unconditionally. 2. The catch block in Copy() now preserves the primary CopyInTransaction exception even when Rollback or SetTransactionMode throws on top. Previously a Rollback-time failure (see the HY010 above) could silently replace the underlying error, which made the original bug much harder to diagnose. Coverage: new sqllogic block in test/sql/duckdb/duckdb_copy.test uses batch_size=1 with 50 integer rows and checks min/max/sum on the destination — a single assertion that catches both row loss and value corruption. The existing batch_tail test (duckdb_copy.test:46) already exercises the mixed batch→single-row transition; it still passes.
- BindSlotShape: drop the redundant `is_null` field — its value was always `type_id == DUCKDB_TYPE_SQLNULL`, so it added nothing to the equality check and one more thing to keep in sync. - IsFixedWidthShape: include `Params::TYPE_SQL_GUID` (raw `SQLGUID` in the union, address-stable across move-assignment) so GUID-keyed single-row inserts also hit the bind-once fast path. - Rename the helper to `BindToOdbcIfShapeUnchanged` and flip the return semantics to `bool reused` (true = cache hit, false = full rebind). This removes the awkward `bool did_bind = ...; reused_bindings = !did_bind;` inversion at the call site and matches the local `reused_bindings` flag. - Header comment: note that integer / float parameters whose target column is CHAR/VARCHAR/WCHAR are coalesced to TYPE_DECIMAL_AS_CHARS by SetExpectedTypes, so they intentionally stay on the rebind-per-row path. - duckdb_copy.test: bump the bind-once smoke test from 50 to 500 rows so the reuse path is exercised many times across multiple internal data chunks.
Addresses the four review items on PR duckdb#168: 1. Per-slot rebind (was all-or-nothing). `Params::BindToOdbcWithCache` now issues SQLBindParameter only for slots whose shape changed since the cached bind, or whose backing buffer is variable-length (DecimalChars / WideString / ScannerBlob / ScannerUuid — the std::vector inside can move on reassignment). Fixed-width siblings on the same row keep their existing binding. A row of (INT, VARCHAR, INT) now rebinds only the VARCHAR slot instead of all three. 2. Caching extended to the batch path. `flat_batch` is hoisted to `LocalInitData` (matching `single_row_buf`) so its slot addresses are stable across CopyInTransaction calls. The batch branch now goes through BindToOdbcWithCache against `batch_bind_cache`, hitting the same per-slot reuse logic. Both caches are reset together at every prepared-statement transition. 3. SQLFreeStmt(SQL_CLOSE) is now called unconditionally before each SQLExecute. The previous gating on `reset_stmt_before_execute || reused` only existed because rebinding implicitly cleared cursor state — with the per-slot path that is no longer guaranteed, and the spec makes SQL_CLOSE on a statement with no open cursor a no-op, so always closing simplifies the loop and removes the `reused_bindings` flag. 4. `BindSlotShape` got a (type_id, expected_type) constructor; the field-by-field assignment in the bind loop is gone. `BindCache::initialized` is also dropped — `cache.shape.size() != params.size()` is the new "first call" signal and the resize-and-clear handles it.
9f06a3b to
d6762bf
Compare
Previous commit assumed SQLPrepare cleared parameter bindings. Per the ODBC spec it does not — only SQLFreeStmt(SQL_RESET_PARAMS) (or rebinding the same index) releases them. With the per-slot cache, transitioning from a 16-row batch statement to a 1-row tail statement on the same hstmt left 15 stale bindings pointing at flat_batch addresses that the new statement has no parameter indices for, segfaulting at least the DuckDB ODBC driver in the "batch tail query single" duckdb_copy.test case. BindToOdbcWithCache now issues SQL_RESET_PARAMS itself whenever the cache is empty (size mismatch with params.size()) — first call after Reset() or a column-count change. Steady-state per-slot rebinds still skip it. Comments on BindCache and BindToOdbcWithCache updated to reflect actual ODBC binding lifetime.
|
Rebased onto latest The rebase also flushed out a latent bug in the per-slot cache: I had assumed CI green on all jobs (Snowflake skipped as before). |
|
Thanks for the update! Looks good to me. |
2 participants
Follow-up to #164. This is item 2 from #163 — bind once / execute many — which I held out of #164 because my first attempt broke
test/sql/duckdb/duckdb_copy.test:46with aSQLSTATE HY010 "Function sequence error"at rollback time. This PR re-lands the optimisation with that issue fixed, and touches up a related diagnostics gap that had been hiding the root cause.The change
odbc_copy's per-row execute loop calledParams::SetExpectedTypes+Params::BindToOdbcfor every row, which collapsed toSQLFreeStmt(SQL_RESET_PARAMS)+ N ×SQLBindParameteron everySQLExecute. That is the shapeodbc_copy_fromused wheneverbatch_size == 1(and the tail of any batched insert whendest_query_singleis specified). It is legal ODBC, but it is the least efficient of the three common execute shapes and — as the Firebird silent-corruption bug in #161 showed — it is also the shape most likely to trip driver bugs that rely on sqlvar state being stable across executes.Params::BindToOdbcIfShapeChangednow caches the(type_id, expected_type, is_null)per slot in aBindCacheheld onLocalInitData. When the next row has the same shape and every slot is fixed-width, the rebind is skipped entirely — the driver reads the new values from the already-bound addresses on the nextSQLExecute. The first row still pays the full bind; subsequent rows just update the values.Two supporting fixes land with this:
SQLFreeStmt(SQL_CLOSE)before the nextSQLExecute. Without it, the DuckDB ODBC driver leaves the statement in a state that makes a laterSQLEndTranfail withSQLSTATE HY010 "Function sequence error". Rebinding implicitly cleared that state; reusing bindings does not. This matches what the existingreset_stmt_before_executequirk does for ClickHouse unconditionally — this PR just applies the same defensive close when (and only when) we've just reused bindings on the previousSQLExecute.Copy()'s catch block. Previously a Rollback-time failure (see HY010 above) could silently replace the underlyingCopyInTransactionerror. The catch now keeps the primary message and appends any Rollback /SetTransactionModefailure to it.Design notes
single_row_buf. EachScannerValueslot needs a stable address acrossCopyInTransactioncalls for the cachedSQLBindParameterpointer to stay valid. Moving the vector ontoLocalInitDatais the minimum change that achieves this; I also guard against a column-count transition (which resets both the buffer and the cache).TYPE_DECIMAL_AS_CHARS,DUCKDB_TYPE_VARCHAR(WideString), BLOB, UUID etc. all hold their buffer inside astd::vector, which can reallocate when the value changes. The bind cache declines to reuse those and falls back to a full rebind per row — the existing behaviour — so no data-shape regression.BindToOdbcwipes per-param state viaSQL_RESET_PARAMS). This keeps the mixeddest_query/dest_query_singleflow — which was the one that exposed the original HY010 — correct.Coverage
New sqllogic block in
test/sql/duckdb/duckdb_copy.testusesbatch_size=1with 50INTEGERrows and checksmin / max / sumon the destination — a single assertion that catches both row loss and value corruption. The existing batch tail query single test (duckdb_copy.test:46) already exercises the mixed batch → single-row transition and still passes — it is the test that caught the original HY010.I did not replicate the bind-once test across the other
*_copy.testfiles because the optimisation is driver-independent: if it works on one driver it works on all of them. Happy to fan it out per-driver (the way #164 ended up) if you'd prefer.CI
All driver jobs pass on the branch with the exception of the same two pre-existing, unrelated failures called out on #164:
libmsodbcsql-18.6.so.1.1vs.2.1symlink mismatch at setup time.test/sql/firebird/14_time_with_time_zone.test:30DST drift.Both fail on
maintoday and neither is touched by this PR. Happy to file separate issues if that helps.Suggested merge order
This PR is stacked on top of #164 in spirit (the bug-fix rationale in #164 motivates why the single-row rebind pattern is worth avoiding) but not mechanically — this branch is based on
main, so it can land before or after #164 without conflicts. I'll be happy to rebase whichever way you prefer.