Skip to content

Commit f9dcbfc

Browse files
committed
REVIEW: Address gargsaumya feedback on GH-610 cache
1. Update thread-safety comment to reference ODBC spec — statement handles are not thread-safe by spec, same assumption as rest of the driver. No additional synchronization needed. 2. Add test_gh610_cache_no_mix_across_different_sql — proves cache is cleared when switching SQL on same cursor (INSERT INTO t1 then INSERT INTO t2 with different param counts). 3. Add test_gh610_cache_null_then_nonnull_same_statement — proves cached NULL type does not interfere with non-NULL execution (non-NULL params use concrete C types, never enter SQL_C_DEFAULT case where cache is consulted). 529 tests pass, 0 regressions.
1 parent 2aea8ce commit f9dcbfc

2 files changed

Lines changed: 48 additions & 1 deletion

File tree

‎mssql_python/pybind/ddbc_bindings.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,8 @@ class SqlHandle {
276276
// GH-610: Per-handle SQLDescribeParam result cache.
277277
// Keyed by 0-based parameter index. Populated on first NULL param
278278
// execution, cleared on SQLPrepare (new SQL = new param types) and
279-
// on handle free. No mutex needed — each handle is used by one thread.
279+
// on handle free. No mutex needed — ODBC statement handles are not
280+
// thread-safe by spec (same assumption as the rest of the driver).
280281
std::unordered_map<int, DescribedParamInfo> describeCache;
281282
void clearDescribeCache() { describeCache.clear(); }
282283

‎tests/test_004_cursor.py‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2305,6 +2305,52 @@ def test_gh610_setinputsizes_bypasses_cache(cursor, db_connection):
23052305
cursor.execute("DROP TABLE #gh610_cov7")
23062306

23072307

2308+
def test_gh610_cache_no_mix_across_different_sql(cursor, db_connection):
2309+
"""Switching SQL on same cursor clears cache — no cross-query type mixing.
2310+
2311+
Regression test requested by gargsaumya: execute INSERT INTO t1 VALUES (?)
2312+
then INSERT INTO t2 VALUES (?, ?) on the same cursor to ensure cache
2313+
doesn't mix parameter types across different prepared statements.
2314+
"""
2315+
cursor.execute("CREATE TABLE #gh610_mix1 (val INT)")
2316+
cursor.execute("CREATE TABLE #gh610_mix2 (a VARCHAR(50), b INT)")
2317+
# First query — cache populated for 1-param INSERT
2318+
cursor.execute("INSERT INTO #gh610_mix1 VALUES (?)", (None,))
2319+
# Different SQL (different param count) — cache must be cleared by re-prepare
2320+
cursor.execute("INSERT INTO #gh610_mix2 VALUES (?, ?)", (None, None))
2321+
# Back to first query — cache cleared again, fresh describe
2322+
cursor.execute("INSERT INTO #gh610_mix1 VALUES (?)", (None,))
2323+
db_connection.commit()
2324+
cursor.execute("SELECT COUNT(*) FROM #gh610_mix1")
2325+
assert cursor.fetchone()[0] == 2
2326+
cursor.execute("SELECT COUNT(*) FROM #gh610_mix2")
2327+
assert cursor.fetchone()[0] == 1
2328+
cursor.execute("DROP TABLE #gh610_mix1")
2329+
cursor.execute("DROP TABLE #gh610_mix2")
2330+
2331+
2332+
def test_gh610_cache_null_then_nonnull_same_statement(cursor, db_connection):
2333+
"""Cached NULL type does not affect subsequent non-NULL execution.
2334+
2335+
Regression test requested by gargsaumya: execute with NULL first (caches
2336+
fallback type), then execute with non-NULL on same statement. The cache
2337+
should not interfere because non-NULL params take a different code path
2338+
(concrete C type, not SQL_C_DEFAULT).
2339+
"""
2340+
cursor.execute("CREATE TABLE #gh610_nullnon (val INT)")
2341+
# First call: param is None → SQL_UNKNOWN_TYPE → cache populated
2342+
cursor.execute("INSERT INTO #gh610_nullnon VALUES (?)", (None,))
2343+
# Second call: param is 42 (int) → SQL_C_TINYINT, never hits cache
2344+
cursor.execute("INSERT INTO #gh610_nullnon VALUES (?)", (42,))
2345+
# Third call: param is None again → cache hit
2346+
cursor.execute("INSERT INTO #gh610_nullnon VALUES (?)", (None,))
2347+
db_connection.commit()
2348+
cursor.execute("SELECT val FROM #gh610_nullnon ORDER BY val")
2349+
rows = [r[0] for r in cursor.fetchall()]
2350+
assert rows == [None, None, 42]
2351+
cursor.execute("DROP TABLE #gh610_nullnon")
2352+
2353+
23082354
@pytest.mark.skip(reason="Skipping due to commit reliability issues with executemany")
23092355
def test_executemany_concurrent_null_parameters(db_connection):
23102356
"""Test executemany with NULL parameters across multiple sequential operations."""

0 commit comments

Comments
 (0)