diff --git a/src/functions/odbc_copy.cpp b/src/functions/odbc_copy.cpp index 723574d..9415f1c 100644 --- a/src/functions/odbc_copy.cpp +++ b/src/functions/odbc_copy.cpp @@ -310,6 +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 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() { } @@ -1042,10 +1050,23 @@ 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()); - std::vector single_row_buf; - single_row_buf.resize(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. + 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; + std::vector &flat_batch = ldata.flat_batch; ldata.chunk_start_moment = CurrentTimeMillis(); for (;;) { @@ -1083,27 +1104,39 @@ 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 — SQLPrepare invalidates + // any existing bindings, so both caches must be dropped. + ldata.single_row_bind_cache.Reset(); + ldata.batch_bind_cache.Reset(); } if (use_batch_insert) { Params::SetExpectedTypes(ctx, ldata.param_types, flat_batch); - Params::BindToOdbc(ctx, flat_batch); + 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); - Params::BindToOdbc(ctx, single_row_buf); + Params::BindToOdbcWithCache(ctx, single_row_buf, ldata.single_row_bind_cache); } - if (ctx.quirks.reset_stmt_before_execute) { + // 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); - 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 + "'"); } } @@ -1150,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(); } } } @@ -1177,11 +1214,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..03f9a18 100644 --- a/src/include/params.hpp +++ b/src/include/params.hpp @@ -15,6 +15,44 @@ namespace odbcscanner { +// 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; + } + bool operator!=(const BindSlotShape &other) const { + return !(*this == other); + } +}; + +// 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 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; + + void Reset() { + shape.clear(); + } +}; + 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 +70,29 @@ struct Params { std::vector &actual); static void BindToOdbc(QueryContext &ctx, std::vector ¶ms); + + // 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. + // + // 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); }; } // namespace odbcscanner diff --git a/src/params.cpp b/src/params.cpp index 9819d77..523db24 100644 --- a/src/params.cpp +++ b/src/params.cpp @@ -196,4 +196,77 @@ void Params::BindToOdbc(QueryContext &ctx, std::vector ¶ms) { } } +// Slots whose backing buffer lives in a dynamically sized container (std::vector +// 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) { + 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_SQL_GUID: + case Params::TYPE_SS_TIMESTAMPOFFSET: + return true; + default: + return false; + } +} + +void Params::BindToOdbcWithCache(QueryContext &ctx, std::vector ¶ms, BindCache &cache) { + if (params.empty()) { + return; + } + + // Size mismatch (first call after Reset, or column-count change) — clear the + // 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()); + } + + 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; + } +} + } // namespace odbcscanner diff --git a/test/sql/duckdb/duckdb_copy.test b/test/sql/duckdb/duckdb_copy.test index ffa5dc3..b0712d2 100644 --- a/test/sql/duckdb/duckdb_copy.test +++ b/test/sql/duckdb/duckdb_copy.test @@ -122,5 +122,37 @@ 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. 500 rows is enough for the bind cache to fire +# the reuse path many times across multiple internal data chunks. + +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, 501) t(i)') +---- +1 500 + +# 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)::BIGINT FROM duckdb_bind_once') +---- +1 500 125250 + +statement ok +SELECT * FROM odbc_query(getvariable('conn'), 'DROP TABLE duckdb_bind_once') + statement ok SELECT odbc_close(getvariable('conn'))