From b0439f03feeee28a246d070bdb05841ddd83c5d8 Mon Sep 17 00:00:00 2001 From: "F.D.Castel" Date: Fri, 24 Apr 2026 18:17:38 -0300 Subject: [PATCH 1/5] odbc_copy: bind once per stable single-row shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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/odbc-scanner#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. --- src/functions/odbc_copy.cpp | 62 ++++++++++++++++++++++----- src/include/params.hpp | 39 +++++++++++++++++ src/params.cpp | 72 ++++++++++++++++++++++++++++++++ test/sql/duckdb/duckdb_copy.test | 29 +++++++++++++ 4 files changed, 192 insertions(+), 10 deletions(-) diff --git a/src/functions/odbc_copy.cpp b/src/functions/odbc_copy.cpp index 723574d..aabe049 100644 --- a/src/functions/odbc_copy.cpp +++ b/src/functions/odbc_copy.cpp @@ -310,6 +310,12 @@ struct LocalInitData { uint32_t last_prepared_batch_size = 0; SQLUINTEGER orig_transaction_mode = SQL_AUTOCOMMIT_DEFAULT; uint64_t inserted_in_transaction = 0; + // single_row_buf is held on the LocalInitData so each ScannerValue slot keeps + // a stable address across CopyInTransaction calls — that is what makes it + // safe for single_row_bind_cache to remember a SQLBindParameter pointer from + // a previous execute. + std::vector single_row_buf; + BindCache single_row_bind_cache; LocalInitData() { } @@ -1044,8 +1050,14 @@ static void CopyInTransaction(duckdb_function_info info, duckdb_data_chunk outpu row.resize(reader.columns.size()); std::vector flat_batch; flat_batch.resize(ldata.last_prepared_batch_size * row.size()); - std::vector single_row_buf; - single_row_buf.resize(row.size()); + if (ldata.single_row_buf.size() != row.size()) { + // Column-count transition (only relevant on the first call per reader) — + // reinit the buffer and drop any stale bind cache. + ldata.single_row_buf.clear(); + ldata.single_row_buf.resize(row.size()); + ldata.single_row_bind_cache.Reset(); + } + std::vector &single_row_buf = ldata.single_row_buf; ldata.chunk_start_moment = CurrentTimeMillis(); for (;;) { @@ -1083,27 +1095,45 @@ static void CopyInTransaction(duckdb_function_info info, duckdb_data_chunk outpu ldata.last_prepared_batch_size = batch_size; tail_insert_prepared = true; use_batch_insert = batch_size > 1; + // The prepared statement has been replaced — any cached bindings + // point at the previous statement state and must be dropped. + ldata.single_row_bind_cache.Reset(); } + // Track whether the bind was reused so we know to close any implicit + // cursor left behind by the previous SQLExecute before the next one. + // Without an explicit SQLFreeStmt(SQL_CLOSE), some drivers (DuckDB ODBC + // in particular) leave the statement in a state that makes a later + // SQLEndTran fail with SQLSTATE HY010 "Function sequence error". + // Rebinding implicitly clears that state; reusing bindings does not. + bool reused_bindings = false; if (use_batch_insert) { Params::SetExpectedTypes(ctx, ldata.param_types, flat_batch); Params::BindToOdbc(ctx, flat_batch); + // Batch binding wipes per-param ODBC state via SQL_RESET_PARAMS, so + // any cached single-row shape would no longer be valid. + ldata.single_row_bind_cache.Reset(); } else { for (size_t i = 0; i < row.size(); i++) { single_row_buf[i] = std::move(flat_batch.at(flat_batch_idx + i)); } flat_batch_idx += row.size(); Params::SetExpectedTypes(ctx, ldata.param_types, single_row_buf); - Params::BindToOdbc(ctx, single_row_buf); + // When the per-row shape is stable (same type_id / expected_type / + // is_null across rows and all slots are fixed-width), this is a + // no-op after the first iteration, collapsing N per-row rebinds + // into a single bind. The rebind-per-row shape is what amplified + // the Firebird silent-corruption bug into row loss. + bool did_bind = Params::BindToOdbcIfShapeChanged(ctx, single_row_buf, ldata.single_row_bind_cache); + reused_bindings = !did_bind; } - if (ctx.quirks.reset_stmt_before_execute) { + if (ctx.quirks.reset_stmt_before_execute || reused_bindings) { SQLRETURN ret = SQLFreeStmt(ctx.hstmt(), SQL_CLOSE); if (!SQL_SUCCEEDED(ret)) { std::string diag = Diagnostics::Read(ctx.hstmt(), SQL_HANDLE_STMT); - throw ScannerException("'SQLFreeStmt' with SQL_CLOSE (reset_stmt_before_execute) failed, query: '" + - ctx.query + "', return: " + std::to_string(ret) + ", diagnostics: '" + diag + - "'"); + throw ScannerException("'SQLFreeStmt' with SQL_CLOSE failed, query: '" + ctx.query + + "', return: " + std::to_string(ret) + ", diagnostics: '" + diag + "'"); } } @@ -1177,11 +1207,23 @@ static void Copy(duckdb_function_info info, duckdb_data_chunk output) { try { CopyInTransaction(info, output); } catch (const std::exception &e) { + std::string primary = e.what(); if (bdata.insert_options.copy_in_transaction) { - Rollback(conn); - SetTransactionMode(conn, ldata.orig_transaction_mode); + // Keep the primary CopyInTransaction error even when Rollback / restoring + // the auto-commit mode throws on top. Without this, a Rollback-time + // SQLSTATE HY010 could hide the actual root cause inside CopyInTransaction. + try { + Rollback(conn); + } catch (const std::exception &re) { + throw ScannerException(primary + " (also, Rollback failed: " + re.what() + ")"); + } + try { + SetTransactionMode(conn, ldata.orig_transaction_mode); + } catch (const std::exception &se) { + throw ScannerException(primary + " (also, SetTransactionMode failed: " + se.what() + ")"); + } } - throw ScannerException(e.what()); + throw ScannerException(primary); } } diff --git a/src/include/params.hpp b/src/include/params.hpp index f45d0fd..3b38a34 100644 --- a/src/include/params.hpp +++ b/src/include/params.hpp @@ -15,6 +15,37 @@ namespace odbcscanner { +// Signature of a bound parameter set, used to detect stable shapes across +// consecutive SQLExecute calls. When every slot matches the previous bind, the +// full SQLFreeStmt(SQL_RESET_PARAMS) + per-param SQLBindParameter cycle can be +// skipped — the driver reads the new values from the already-bound addresses +// on the next SQLExecute. Rebinding every row is the execute shape that turned +// the Firebird ODBC driver's numeric-write bug (duckdb/odbc-scanner#161 / +// FirebirdSQL/firebird-odbc-driver#292) into catastrophic row loss, and it is +// also the least efficient of the three common execute shapes. +struct BindSlotShape { + param_type type_id = DUCKDB_TYPE_INVALID; + SQLSMALLINT expected_type = SQL_PARAM_TYPE_UNKNOWN; + bool is_null = false; + + bool operator==(const BindSlotShape &other) const { + return type_id == other.type_id && expected_type == other.expected_type && is_null == other.is_null; + } + bool operator!=(const BindSlotShape &other) const { + return !(*this == other); + } +}; + +struct BindCache { + std::vector shape; + bool initialized = false; + + void Reset() { + shape.clear(); + initialized = false; + } +}; + struct Params { static const param_type TYPE_DECIMAL_AS_CHARS = DUCKDB_TYPE_DECIMAL + 1000; static const param_type TYPE_TIME_WITH_NANOS = DUCKDB_TYPE_TIME + 1000; @@ -32,6 +63,14 @@ struct Params { std::vector &actual); static void BindToOdbc(QueryContext &ctx, std::vector ¶ms); + + // Binds only when the current shape differs from the cached one. Returns + // true if SQLBindParameter was actually issued. Variable-length slots + // (VARCHAR / TYPE_DECIMAL_AS_CHARS / BLOB / UUID) force a rebind because + // their backing buffers are not guaranteed to keep a stable address when + // the value changes — for those, the caller still benefits from the normal + // BindToOdbc path but pays the bind cost every row. + static bool BindToOdbcIfShapeChanged(QueryContext &ctx, std::vector ¶ms, BindCache &cache); }; } // namespace odbcscanner diff --git a/src/params.cpp b/src/params.cpp index 9819d77..331c5e3 100644 --- a/src/params.cpp +++ b/src/params.cpp @@ -196,4 +196,76 @@ void Params::BindToOdbc(QueryContext &ctx, std::vector ¶ms) { } } +// Slots whose backing buffer lives in a dynamically sized container (std::vector +// inside DecimalChars / WideString / ScannerBlob, etc.) can reallocate as the +// value changes; the previously-bound pointer would then be invalid. Only +// fixed-width slots are safe to reuse across executes without rebinding. +static bool IsFixedWidthShape(param_type type_id) { + switch (type_id) { + case DUCKDB_TYPE_SQLNULL: + case DUCKDB_TYPE_BOOLEAN: + case DUCKDB_TYPE_TINYINT: + case DUCKDB_TYPE_UTINYINT: + case DUCKDB_TYPE_SMALLINT: + case DUCKDB_TYPE_USMALLINT: + case DUCKDB_TYPE_INTEGER: + case DUCKDB_TYPE_UINTEGER: + case DUCKDB_TYPE_BIGINT: + case DUCKDB_TYPE_UBIGINT: + case DUCKDB_TYPE_FLOAT: + case DUCKDB_TYPE_DOUBLE: + case DUCKDB_TYPE_DECIMAL: + case DUCKDB_TYPE_DATE: + case DUCKDB_TYPE_TIME: + case DUCKDB_TYPE_TIMESTAMP: + case DUCKDB_TYPE_TIMESTAMP_TZ: + case Params::TYPE_TIME_WITH_NANOS: + case Params::TYPE_SQL_BIT: + case Params::TYPE_SS_TIMESTAMPOFFSET: + return true; + default: + return false; + } +} + +bool Params::BindToOdbcIfShapeChanged(QueryContext &ctx, std::vector ¶ms, BindCache &cache) { + if (params.size() == 0) { + return false; + } + + std::vector shape; + shape.reserve(params.size()); + bool all_fixed_width = true; + for (size_t i = 0; i < params.size(); i++) { + ScannerValue &p = params.at(i); + BindSlotShape s; + s.type_id = p.ParamType(); + s.expected_type = p.ExpectedType(); + s.is_null = (s.type_id == DUCKDB_TYPE_SQLNULL); + shape.push_back(s); + if (!IsFixedWidthShape(s.type_id)) { + all_fixed_width = false; + } + } + + bool can_skip = cache.initialized && all_fixed_width && cache.shape.size() == shape.size(); + if (can_skip) { + for (size_t i = 0; i < shape.size(); i++) { + if (cache.shape.at(i) != shape.at(i)) { + can_skip = false; + break; + } + } + } + + if (can_skip) { + return false; + } + + BindToOdbc(ctx, params); + cache.shape = std::move(shape); + cache.initialized = true; + return true; +} + } // namespace odbcscanner diff --git a/test/sql/duckdb/duckdb_copy.test b/test/sql/duckdb/duckdb_copy.test index ffa5dc3..a7493cf 100644 --- a/test/sql/duckdb/duckdb_copy.test +++ b/test/sql/duckdb/duckdb_copy.test @@ -122,5 +122,34 @@ SELECT * FROM odbc_query(getvariable('conn'), statement ok SELECT * FROM odbc_query(getvariable('conn'), 'DROP TABLE duckdb_int_to_varchar_pk') +# bind-once-execute-many for the single-row path (duckdb/odbc-scanner#163, item 2). +# batch_size=1 forces the scanner to take the single-row branch of the inner +# execute loop; with a stable row shape the parameter bindings are now reused +# across SQLExecute calls instead of being rebuilt every row. The correctness +# check is that every source row ends up in the destination table with the +# right value — the min/max/sum triple catches both row loss and value +# corruption in a single shot. + +statement ok +SELECT * FROM odbc_query(getvariable('conn'), + 'CREATE TABLE duckdb_bind_once (col1 INTEGER NOT NULL)') + +query II +SELECT completed, rows_processed FROM odbc_copy(getvariable('conn'), + dest_table='duckdb_bind_once', + batch_size=1, + source_query='SELECT i::INTEGER AS col1 FROM range(1, 51) t(i)') +---- +1 50 + +query III +SELECT * FROM odbc_query(getvariable('conn'), + 'SELECT min(col1), max(col1), sum(col1) FROM duckdb_bind_once') +---- +1 50 1275 + +statement ok +SELECT * FROM odbc_query(getvariable('conn'), 'DROP TABLE duckdb_bind_once') + statement ok SELECT odbc_close(getvariable('conn')) From 977ce4e986835077b8810400a1a0434a435b7838 Mon Sep 17 00:00:00 2001 From: "F.D.Castel" Date: Fri, 24 Apr 2026 18:19:50 -0300 Subject: [PATCH 2/5] tests(duckdb_copy): cast sum(col1) to BIGINT (scanner does not map HUGEINT) --- test/sql/duckdb/duckdb_copy.test | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/sql/duckdb/duckdb_copy.test b/test/sql/duckdb/duckdb_copy.test index a7493cf..f410162 100644 --- a/test/sql/duckdb/duckdb_copy.test +++ b/test/sql/duckdb/duckdb_copy.test @@ -142,9 +142,11 @@ SELECT completed, rows_processed FROM odbc_copy(getvariable('conn'), ---- 1 50 +# sum() over INTEGER returns HUGEINT in DuckDB, which the scanner cannot map +# directly; cast to BIGINT so the three columns all come back as integers. query III SELECT * FROM odbc_query(getvariable('conn'), - 'SELECT min(col1), max(col1), sum(col1) FROM duckdb_bind_once') + 'SELECT min(col1), max(col1), sum(col1)::BIGINT FROM duckdb_bind_once') ---- 1 50 1275 From 8f161a4d37f01f62ef119ed89535bfbda506365b Mon Sep 17 00:00:00 2001 From: "F.D.Castel" Date: Sat, 25 Apr 2026 15:08:04 -0300 Subject: [PATCH 3/5] odbc_copy: tighten BindCache + fold #164 fallback into the comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- src/functions/odbc_copy.cpp | 13 ++++++------- src/include/params.hpp | 19 +++++++++++-------- src/params.cpp | 16 ++++++++-------- test/sql/duckdb/duckdb_copy.test | 9 +++++---- 4 files changed, 30 insertions(+), 27 deletions(-) diff --git a/src/functions/odbc_copy.cpp b/src/functions/odbc_copy.cpp index aabe049..c6fcb29 100644 --- a/src/functions/odbc_copy.cpp +++ b/src/functions/odbc_copy.cpp @@ -1119,13 +1119,12 @@ static void CopyInTransaction(duckdb_function_info info, duckdb_data_chunk outpu } flat_batch_idx += row.size(); Params::SetExpectedTypes(ctx, ldata.param_types, single_row_buf); - // When the per-row shape is stable (same type_id / expected_type / - // is_null across rows and all slots are fixed-width), this is a - // no-op after the first iteration, collapsing N per-row rebinds - // into a single bind. The rebind-per-row shape is what amplified - // the Firebird silent-corruption bug into row loss. - bool did_bind = Params::BindToOdbcIfShapeChanged(ctx, single_row_buf, ldata.single_row_bind_cache); - reused_bindings = !did_bind; + // When the per-row shape is stable (same type_id / expected_type + // across rows and all slots are fixed-width), this is a no-op + // after the first iteration, collapsing N per-row rebinds into a + // single bind. The rebind-per-row shape is what amplified the + // Firebird silent-corruption bug into row loss. + reused_bindings = Params::BindToOdbcIfShapeUnchanged(ctx, single_row_buf, ldata.single_row_bind_cache); } if (ctx.quirks.reset_stmt_before_execute || reused_bindings) { diff --git a/src/include/params.hpp b/src/include/params.hpp index 3b38a34..b12b240 100644 --- a/src/include/params.hpp +++ b/src/include/params.hpp @@ -26,10 +26,9 @@ namespace odbcscanner { struct BindSlotShape { param_type type_id = DUCKDB_TYPE_INVALID; SQLSMALLINT expected_type = SQL_PARAM_TYPE_UNKNOWN; - bool is_null = false; bool operator==(const BindSlotShape &other) const { - return type_id == other.type_id && expected_type == other.expected_type && is_null == other.is_null; + return type_id == other.type_id && expected_type == other.expected_type; } bool operator!=(const BindSlotShape &other) const { return !(*this == other); @@ -65,12 +64,16 @@ struct Params { static void BindToOdbc(QueryContext &ctx, std::vector ¶ms); // Binds only when the current shape differs from the cached one. Returns - // true if SQLBindParameter was actually issued. Variable-length slots - // (VARCHAR / TYPE_DECIMAL_AS_CHARS / BLOB / UUID) force a rebind because - // their backing buffers are not guaranteed to keep a stable address when - // the value changes — for those, the caller still benefits from the normal - // BindToOdbc path but pays the bind cost every row. - static bool BindToOdbcIfShapeChanged(QueryContext &ctx, std::vector ¶ms, BindCache &cache); + // true when the cached bindings were reused (no SQLBindParameter issued), + // false when a full rebind happened. Variable-length slots (VARCHAR / + // TYPE_DECIMAL_AS_CHARS / BLOB / UUID) force a rebind because their backing + // buffers are not guaranteed to keep a stable address when the value changes + // — for those, the caller still benefits from the normal BindToOdbc path but + // pays the bind cost every row. This includes integer / float parameters + // whose target column is CHAR/VARCHAR/WCHAR family: SetExpectedTypes + // coalesces those into TYPE_DECIMAL_AS_CHARS, so they take the same + // rebind-per-row fallback. + static bool BindToOdbcIfShapeUnchanged(QueryContext &ctx, std::vector ¶ms, BindCache &cache); }; } // namespace odbcscanner diff --git a/src/params.cpp b/src/params.cpp index 331c5e3..4d935d0 100644 --- a/src/params.cpp +++ b/src/params.cpp @@ -221,6 +221,7 @@ static bool IsFixedWidthShape(param_type type_id) { case DUCKDB_TYPE_TIMESTAMP_TZ: case Params::TYPE_TIME_WITH_NANOS: case Params::TYPE_SQL_BIT: + case Params::TYPE_SQL_GUID: case Params::TYPE_SS_TIMESTAMPOFFSET: return true; default: @@ -228,7 +229,7 @@ static bool IsFixedWidthShape(param_type type_id) { } } -bool Params::BindToOdbcIfShapeChanged(QueryContext &ctx, std::vector ¶ms, BindCache &cache) { +bool Params::BindToOdbcIfShapeUnchanged(QueryContext &ctx, std::vector ¶ms, BindCache &cache) { if (params.size() == 0) { return false; } @@ -241,31 +242,30 @@ bool Params::BindToOdbcIfShapeChanged(QueryContext &ctx, std::vector Date: Mon, 27 Apr 2026 12:31:11 -0300 Subject: [PATCH 4/5] odbc_copy: per-slot bind cache + extend it to the batch path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the four review items on PR #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. --- src/functions/odbc_copy.cpp | 60 +++++++++++++++++++++---------------- src/include/params.hpp | 53 ++++++++++++++++++++------------ src/params.cpp | 56 ++++++++++++++-------------------- 3 files changed, 91 insertions(+), 78 deletions(-) diff --git a/src/functions/odbc_copy.cpp b/src/functions/odbc_copy.cpp index c6fcb29..9415f1c 100644 --- a/src/functions/odbc_copy.cpp +++ b/src/functions/odbc_copy.cpp @@ -310,12 +310,14 @@ struct LocalInitData { uint32_t last_prepared_batch_size = 0; SQLUINTEGER orig_transaction_mode = SQL_AUTOCOMMIT_DEFAULT; uint64_t inserted_in_transaction = 0; - // single_row_buf is held on the LocalInitData so each ScannerValue slot keeps - // a stable address across CopyInTransaction calls — that is what makes it - // safe for single_row_bind_cache to remember a SQLBindParameter pointer from - // a previous execute. + // single_row_buf and flat_batch are held on the LocalInitData so each + // ScannerValue slot keeps a stable address across CopyInTransaction calls — + // that is what makes it safe for the bind caches to remember a + // SQLBindParameter pointer from a previous execute. std::vector single_row_buf; + std::vector flat_batch; BindCache single_row_bind_cache; + BindCache batch_bind_cache; LocalInitData() { } @@ -1048,8 +1050,14 @@ static void CopyInTransaction(duckdb_function_info info, duckdb_data_chunk outpu std::vector row; row.resize(reader.columns.size()); - std::vector flat_batch; - flat_batch.resize(ldata.last_prepared_batch_size * row.size()); + size_t needed_flat_batch_size = ldata.last_prepared_batch_size * row.size(); + if (ldata.flat_batch.size() != needed_flat_batch_size) { + // First call, batch-size transition, or column-count transition. Vector + // may reallocate; any cached batch bindings now point at stale storage. + ldata.flat_batch.clear(); + ldata.flat_batch.resize(needed_flat_batch_size); + ldata.batch_bind_cache.Reset(); + } if (ldata.single_row_buf.size() != row.size()) { // Column-count transition (only relevant on the first call per reader) — // reinit the buffer and drop any stale bind cache. @@ -1058,6 +1066,7 @@ static void CopyInTransaction(duckdb_function_info info, duckdb_data_chunk outpu ldata.single_row_bind_cache.Reset(); } std::vector &single_row_buf = ldata.single_row_buf; + std::vector &flat_batch = ldata.flat_batch; ldata.chunk_start_moment = CurrentTimeMillis(); for (;;) { @@ -1095,39 +1104,34 @@ static void CopyInTransaction(duckdb_function_info info, duckdb_data_chunk outpu ldata.last_prepared_batch_size = batch_size; tail_insert_prepared = true; use_batch_insert = batch_size > 1; - // The prepared statement has been replaced — any cached bindings - // point at the previous statement state and must be dropped. + // The prepared statement has been replaced — SQLPrepare invalidates + // any existing bindings, so both caches must be dropped. ldata.single_row_bind_cache.Reset(); + ldata.batch_bind_cache.Reset(); } - // Track whether the bind was reused so we know to close any implicit - // cursor left behind by the previous SQLExecute before the next one. - // Without an explicit SQLFreeStmt(SQL_CLOSE), some drivers (DuckDB ODBC - // in particular) leave the statement in a state that makes a later - // SQLEndTran fail with SQLSTATE HY010 "Function sequence error". - // Rebinding implicitly clears that state; reusing bindings does not. - bool reused_bindings = false; if (use_batch_insert) { Params::SetExpectedTypes(ctx, ldata.param_types, flat_batch); - Params::BindToOdbc(ctx, flat_batch); - // Batch binding wipes per-param ODBC state via SQL_RESET_PARAMS, so - // any cached single-row shape would no longer be valid. - ldata.single_row_bind_cache.Reset(); + Params::BindToOdbcWithCache(ctx, flat_batch, ldata.batch_bind_cache); } else { for (size_t i = 0; i < row.size(); i++) { single_row_buf[i] = std::move(flat_batch.at(flat_batch_idx + i)); } flat_batch_idx += row.size(); Params::SetExpectedTypes(ctx, ldata.param_types, single_row_buf); - // When the per-row shape is stable (same type_id / expected_type - // across rows and all slots are fixed-width), this is a no-op - // after the first iteration, collapsing N per-row rebinds into a - // single bind. The rebind-per-row shape is what amplified the - // Firebird silent-corruption bug into row loss. - reused_bindings = Params::BindToOdbcIfShapeUnchanged(ctx, single_row_buf, ldata.single_row_bind_cache); + Params::BindToOdbcWithCache(ctx, single_row_buf, ldata.single_row_bind_cache); } - if (ctx.quirks.reset_stmt_before_execute || reused_bindings) { + // Always close any cursor / result-set state left by the previous + // SQLExecute before the next one. Without this, some drivers (DuckDB + // ODBC in particular) leave the statement in a state that makes a + // later SQLEndTran fail with SQLSTATE HY010 "Function sequence + // error". The previous implementation only did this on the + // reset_stmt_before_execute quirk path or when cached bindings were + // reused; doing it unconditionally simplifies the loop and matches + // the spec — SQL_CLOSE on a statement with no open cursor is a + // no-op. + { SQLRETURN ret = SQLFreeStmt(ctx.hstmt(), SQL_CLOSE); if (!SQL_SUCCEEDED(ret)) { std::string diag = Diagnostics::Read(ctx.hstmt(), SQL_HANDLE_STMT); @@ -1179,6 +1183,10 @@ static void CopyInTransaction(duckdb_function_info info, duckdb_data_chunk outpu ctx.query = PrepareInsert(ctx.hstmt(), bdata, reader.columns, batch_size); ldata.param_types = Params::CollectTypes(ctx); ldata.last_prepared_batch_size = batch_size; + // SQLPrepare invalidated the existing bindings on the statement + // handle; both caches must be dropped before the next bind. + ldata.single_row_bind_cache.Reset(); + ldata.batch_bind_cache.Reset(); } } } diff --git a/src/include/params.hpp b/src/include/params.hpp index b12b240..1fe75f5 100644 --- a/src/include/params.hpp +++ b/src/include/params.hpp @@ -15,18 +15,23 @@ namespace odbcscanner { -// Signature of a bound parameter set, used to detect stable shapes across -// consecutive SQLExecute calls. When every slot matches the previous bind, the -// full SQLFreeStmt(SQL_RESET_PARAMS) + per-param SQLBindParameter cycle can be -// skipped — the driver reads the new values from the already-bound addresses -// on the next SQLExecute. Rebinding every row is the execute shape that turned -// the Firebird ODBC driver's numeric-write bug (duckdb/odbc-scanner#161 / +// Per-slot signature captured the last time a parameter index was bound. The +// pair (type_id, expected_type) is everything BindOdbcParam looks at; if both +// match the current value, the slot's previous SQLBindParameter is still valid +// (modulo buffer-pointer stability — see IsFixedWidthShape in params.cpp). +// Rebinding every row is the execute shape that turned the Firebird ODBC +// driver's numeric-write bug (duckdb/odbc-scanner#161 / // FirebirdSQL/firebird-odbc-driver#292) into catastrophic row loss, and it is // also the least efficient of the three common execute shapes. struct BindSlotShape { param_type type_id = DUCKDB_TYPE_INVALID; SQLSMALLINT expected_type = SQL_PARAM_TYPE_UNKNOWN; + BindSlotShape() = default; + BindSlotShape(param_type type_id_in, SQLSMALLINT expected_type_in) + : type_id(type_id_in), expected_type(expected_type_in) { + } + bool operator==(const BindSlotShape &other) const { return type_id == other.type_id && expected_type == other.expected_type; } @@ -35,13 +40,15 @@ struct BindSlotShape { } }; +// Per-prepared-statement bind state. `shape[i]` is the last-bound shape for +// param index i+1; an empty vector means "nothing bound yet". Caller MUST call +// Reset() whenever the prepared statement changes (SQLPrepare invalidates all +// existing bindings). struct BindCache { std::vector shape; - bool initialized = false; void Reset() { shape.clear(); - initialized = false; } }; @@ -63,17 +70,25 @@ struct Params { static void BindToOdbc(QueryContext &ctx, std::vector ¶ms); - // Binds only when the current shape differs from the cached one. Returns - // true when the cached bindings were reused (no SQLBindParameter issued), - // false when a full rebind happened. Variable-length slots (VARCHAR / - // TYPE_DECIMAL_AS_CHARS / BLOB / UUID) force a rebind because their backing - // buffers are not guaranteed to keep a stable address when the value changes - // — for those, the caller still benefits from the normal BindToOdbc path but - // pays the bind cost every row. This includes integer / float parameters - // whose target column is CHAR/VARCHAR/WCHAR family: SetExpectedTypes - // coalesces those into TYPE_DECIMAL_AS_CHARS, so they take the same - // rebind-per-row fallback. - static bool BindToOdbcIfShapeUnchanged(QueryContext &ctx, std::vector ¶ms, BindCache &cache); + // Per-slot incremental bind. 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 value reassignment, so the previously + // bound pointer would dangle). Fixed-width slots whose shape is unchanged + // keep their existing SQLBindParameter — the driver reads the new value + // from the same address on the next SQLExecute. + // + // This includes integer / float parameters whose target column is the + // CHAR/VARCHAR/WCHAR family: SetExpectedTypes coalesces those into + // TYPE_DECIMAL_AS_CHARS, so each such row still rebinds that slot but + // fixed-width siblings on the same row stay cached. + // + // Does NOT call SQLFreeStmt(SQL_RESET_PARAMS); per-slot SQLBindParameter + // overwrites the previous binding for that index. Caller must Reset(cache) + // whenever the prepared statement changes (SQLPrepare wipes bindings) or + // the params vector's storage may have moved (different `std::vector` + // instance, capacity-changing resize, etc.). + static void BindToOdbcWithCache(QueryContext &ctx, std::vector ¶ms, BindCache &cache); }; } // namespace odbcscanner diff --git a/src/params.cpp b/src/params.cpp index 4d935d0..ae2563e 100644 --- a/src/params.cpp +++ b/src/params.cpp @@ -197,8 +197,8 @@ void Params::BindToOdbc(QueryContext &ctx, std::vector ¶ms) { } // Slots whose backing buffer lives in a dynamically sized container (std::vector -// inside DecimalChars / WideString / ScannerBlob, etc.) can reallocate as the -// value changes; the previously-bound pointer would then be invalid. Only +// inside DecimalChars / WideString / ScannerBlob / ScannerUuid) can move as the +// value is reassigned — the previously bound pointer would then dangle. Only // fixed-width slots are safe to reuse across executes without rebinding. static bool IsFixedWidthShape(param_type type_id) { switch (type_id) { @@ -229,43 +229,33 @@ static bool IsFixedWidthShape(param_type type_id) { } } -bool Params::BindToOdbcIfShapeUnchanged(QueryContext &ctx, std::vector ¶ms, BindCache &cache) { - if (params.size() == 0) { - return false; +void Params::BindToOdbcWithCache(QueryContext &ctx, std::vector ¶ms, BindCache &cache) { + if (params.empty()) { + return; } - std::vector shape; - shape.reserve(params.size()); - bool all_fixed_width = true; - for (size_t i = 0; i < params.size(); i++) { - ScannerValue &p = params.at(i); - BindSlotShape s; - s.type_id = p.ParamType(); - s.expected_type = p.ExpectedType(); - shape.push_back(s); - if (!IsFixedWidthShape(s.type_id)) { - all_fixed_width = false; - } + // Size mismatch (first call after Reset, or column-count change) — clear the + // cache so every slot gets bound fresh. + if (cache.shape.size() != params.size()) { + cache.shape.assign(params.size(), BindSlotShape()); } - bool can_reuse = cache.initialized && all_fixed_width && cache.shape.size() == shape.size(); - if (can_reuse) { - for (size_t i = 0; i < shape.size(); i++) { - if (cache.shape.at(i) != shape.at(i)) { - can_reuse = false; - break; - } + for (size_t i = 0; i < params.size(); i++) { + ScannerValue &p = params.at(i); + BindSlotShape current(p.ParamType(), p.ExpectedType()); + // Rebind when the slot's logical shape changed, OR when the slot is + // variable-width (its buffer pointer may have moved even with the same + // shape). Fixed-width + same-shape slots are intentionally left bound to + // their previous address — the value at that address has been updated + // in-place by the caller and the next SQLExecute will pick it up. + bool needs_rebind = (cache.shape.at(i) != current) || !IsFixedWidthShape(current.type_id); + if (!needs_rebind) { + continue; } + SQLSMALLINT idx = static_cast(i + 1); + Types::BindOdbcParam(ctx, p, idx); + cache.shape.at(i) = current; } - - if (can_reuse) { - return true; - } - - BindToOdbc(ctx, params); - cache.shape = std::move(shape); - cache.initialized = true; - return false; } } // namespace odbcscanner From b887398a5c14bcb0a2ceb97ff769a5bb1755f187 Mon Sep 17 00:00:00 2001 From: "F.D.Castel" Date: Mon, 27 Apr 2026 12:58:24 -0300 Subject: [PATCH 5/5] odbc_copy: SQL_RESET_PARAMS on cache-empty path in BindToOdbcWithCache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/include/params.hpp | 16 ++++++++++------ src/params.cpp | 13 ++++++++++++- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/include/params.hpp b/src/include/params.hpp index 1fe75f5..03f9a18 100644 --- a/src/include/params.hpp +++ b/src/include/params.hpp @@ -42,8 +42,9 @@ struct BindSlotShape { // Per-prepared-statement bind state. `shape[i]` is the last-bound shape for // param index i+1; an empty vector means "nothing bound yet". Caller MUST call -// Reset() whenever the prepared statement changes (SQLPrepare invalidates all -// existing bindings). +// Reset() whenever the prepared statement changes — SQLPrepare does NOT clear +// parameter bindings on the hstmt, so BindToOdbcWithCache will issue +// SQL_RESET_PARAMS itself the next time it sees an empty cache. struct BindCache { std::vector shape; @@ -83,10 +84,13 @@ struct Params { // TYPE_DECIMAL_AS_CHARS, so each such row still rebinds that slot but // fixed-width siblings on the same row stay cached. // - // Does NOT call SQLFreeStmt(SQL_RESET_PARAMS); per-slot SQLBindParameter - // overwrites the previous binding for that index. Caller must Reset(cache) - // whenever the prepared statement changes (SQLPrepare wipes bindings) or - // the params vector's storage may have moved (different `std::vector` + // In the steady state (cache populated, same `params.size()`), per-slot + // SQLBindParameter overwrites the previous binding for that index — no + // SQL_RESET_PARAMS needed. On the first call after Reset() (or when + // `params.size()` changes), SQL_RESET_PARAMS is issued first to drop any + // leftover bindings from a previous prepared statement on the same hstmt. + // Caller must Reset(cache) whenever the prepared statement changes or the + // params vector's storage may have moved (different `std::vector` // instance, capacity-changing resize, etc.). static void BindToOdbcWithCache(QueryContext &ctx, std::vector ¶ms, BindCache &cache); }; diff --git a/src/params.cpp b/src/params.cpp index ae2563e..523db24 100644 --- a/src/params.cpp +++ b/src/params.cpp @@ -235,8 +235,19 @@ void Params::BindToOdbcWithCache(QueryContext &ctx, std::vector &p } // Size mismatch (first call after Reset, or column-count change) — clear the - // cache so every slot gets bound fresh. + // cache so every slot gets bound fresh, and call SQL_RESET_PARAMS first to + // drop any leftover bindings from a previous prepared statement on the same + // hstmt. SQLPrepare does NOT release parameter bindings — only + // SQL_RESET_PARAMS (or rebinding the same index) does — so without this a + // 16-row batch statement transitioning to a 1-row tail statement leaves 15 + // stale bindings pointing at addresses the new statement has no parameter + // indices for, which segfaults at least the DuckDB ODBC driver. if (cache.shape.size() != params.size()) { + SQLRETURN ret = SQLFreeStmt(ctx.hstmt(), SQL_RESET_PARAMS); + if (!SQL_SUCCEEDED(ret)) { + std::string diag = Diagnostics::Read(ctx.hstmt(), SQL_HANDLE_STMT); + throw ScannerException("'SQLFreeStmt' SQL_RESET_PARAMS failed, diagnostics: '" + diag + "'"); + } cache.shape.assign(params.size(), BindSlotShape()); }