odbc_copy: bind once per stable single-row shape (re: #163 item 2)

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<ScannerValue> single_row_buf;
+	std::vector<ScannerValue> 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<ScannerValue> row;
 	row.resize(reader.columns.size());
-	std::vector<ScannerValue> flat_batch;
-	flat_batch.resize(ldata.last_prepared_batch_size * row.size());
-	std::vector<ScannerValue> 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<ScannerValue> &single_row_buf = ldata.single_row_buf;
+	std::vector<ScannerValue> &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);
 	}
 }
 

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<BindSlotShape> 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<ScannerValue> &actual);
 
 	static void BindToOdbc(QueryContext &ctx, std::vector<ScannerValue> &params);
+
+	// 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<ScannerValue> &params, BindCache &cache);
 };
 
 } // namespace odbcscanner

src/params.cpp

@@ -196,4 +196,77 @@ void Params::BindToOdbc(QueryContext &ctx, std::vector<ScannerValue> &params) {
 	}
 }
 
+// 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<ScannerValue> &params, 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<SQLSMALLINT>(i + 1);
+		Types::BindOdbcParam(ctx, p, idx);
+		cache.shape.at(i) = current;
+	}
+}
+
 } // namespace odbcscanner

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'))