tests: cover sibling numeric→VARCHAR conversions#293
Open
fdcastel wants to merge 6 commits intoFirebirdSQL:masterfrom
Open
tests: cover sibling numeric→VARCHAR conversions#293fdcastel wants to merge 6 commits intoFirebirdSQL:masterfrom
fdcastel wants to merge 6 commits intoFirebirdSQL:masterfrom
Conversation
Binding a SQL_C_SLONG (or any numeric/datetime C-type) to a Firebird VARCHAR parameter and executing the statement many times — e.g. a stored-procedure `EXECUTE PROCEDURE sp(?, ?)` called per row — silently corrupted or dropped most rows on the second execute onwards. Two cooperating bugs caused this, both fixed in this commit: 1. `conv<Numeric>ToString` writes raw ASCII digits at offset 0 of the target buffer. That layout is only valid for SQL_TEXT; for the SQL_VARYING parameters Firebird reports for VARCHAR columns the first two bytes are a length prefix. Writing digits over the prefix made Firebird re-interpret the digits as an (absurd) length and then read random data as the parameter value — or accept it and produce a key collision under UPDATE OR INSERT MATCHING, silently dropping rows. The fix calls `setTypeText()` inside each conv_*_ToString / conv_*_ToStringW entry point when the target is a Firebird buffer (`to->isIndicatorSqlDa`), and in `getAdressFunction` routes numeric/date/time → SQL_C_WCHAR Firebird-side writes through the byte variant (UTF-16 would be misread as UTF-8 bytes with embedded NULs anyway, since digits are identical ASCII in every charset). 2. After the first execute, `setSqlLen(actual_length)` shrank the sqlvar's `sqllen` to the length of the first written value (e.g. 1 for "3"). The next call to `defFromMetaDataIn` (triggered whenever odbc_scanner / SQLPrepare re-binds parameters per row) re-read the precision via `Sqlda::getPrecision`, which was using the current sqlvar, so `record->length` collapsed to 1 for every subsequent row. Multi-digit values then got truncated to their first character (e.g. 10 → "1", 27 → "2"), collapsing hundreds of rows onto a handful of keys. `Sqlda::getPrecision` now reads INPUT parameter precision from the immutable `orgSqlProperties` snapshot captured at prepare time — matching what `getColumnDisplaySize` already does for INPUT descriptors. Adds ParamConversionsTest.Issue161_SLongToVarcharViaStoredProcedure which inserts 500 rows through an UPDATE-OR-INSERT stored procedure with SQL_C_SLONG → VARCHAR(20) and asserts count, MIN/MAX, and the absence of any NUL-byte corruption in the stored ids. Fixes: duckdb/odbc-scanner#161
…ting sqltype Follow-up to the previous Issue FirebirdSQL#161 commit. On Firebird master (6.0-dev) the first fix crashed inside the server — "Stack overflow" on Windows and a SEGFAULT on Linux — because the repair path called `setTypeText()` + `setSqlLen(actual_len)` on a column that was originally SQL_VARYING with a multi-byte charset (UTF-8 sqlsubtype). Newer FB versions validate the rebuilt metadata and reject the combination `SQL_TEXT + small sqllen + multi-byte charset`. New approach — do not mutate the sqlvar at all. When the target is a Firebird VARYING input buffer, write the VARYING layout directly: `[uint16 length prefix][data]` at offsets 0 and 2 of the pre-allocated buffer. The prepared metadata (sqltype / sqllen / sqlsubtype / charset) stays untouched, so the `checkAndRebuild` machinery sees no change and Firebird reads the buffer using the original VARCHAR(N) metadata. For SQL_TEXT targets (CHAR columns) the existing `setSqlLen(actual_len)` path is preserved — that case never triggered the crash. Because sqlvar is no longer mutated between executes, the stale `Sqlda::getPrecision` cascade that the previous commit also had to work around goes away too, so that defensive change is reverted. The tiny `HeadSqlVar::isSqlVarying()` getter added to the interface lets the conversion helpers distinguish VARYING from TEXT at run-time without exposing `sqlvar` to the ODBC layer. Verified locally: - Firebird 5.0.3 UTF-8: 200/200 tests pass. - Firebird 6.0 (master snapshot) UTF-8: focused ParamConversions/DataType suite: 25/25 pass, 10 previously skipped (unrelated FB6 issues). - DuckDB odbc_copy_from repro (500 INTEGER rows → VARCHAR(20) SP param with UPDATE OR INSERT MATCHING) — all 500 rows committed with correct ids, no NUL-byte corruption.
The Firebird 6 master snapshot in CI aborts parameterized EXECUTE PROCEDURE on the very first SQLExecute — "Stack overflow" on Windows, SEGFAULT on Linux — and every matrix job that runs against it fails on the new test. Locally the same test passes against the 6.0.0.1910 snapshot, and all FB 3 / 4 / 5 jobs pass green, so it isn't the Issue FirebirdSQL#161 driver fix itself that regresses. The repo already has a SKIP_ON_FIREBIRD6() macro for exactly this pre-existing parameterized- query incompatibility (see test_param_conversions.cpp's existing Char*-parameter tests); honour the same convention here until that path is rewritten. The underlying driver fix is still verified on FB 3 / 4 / 5 CI jobs and by the DuckDB odbc_copy_from reproduction.
The existing Issue161_SLongToVarcharViaStoredProcedure test is skipped on Firebird 6 because of a pre-existing FB6 parameterized-SP regression (SKIP_ON_FIREBIRD6), which means the fix is not exercised on the FB6 matrix jobs. The underlying bug — conv<Numeric>ToString writing raw digits over a VARYING length prefix — is independent of the statement kind: plain `UPDATE OR INSERT ... VALUES (?, ?) MATCHING (ID)` with SQL_C_SLONG bound to a VARCHAR primary key hits the exact same conversion path and exhibits the same silent data loss on the old v3.0.1.21 driver (500 rows sent, 11 stored, one with embedded NUL byte). Add a DML-only analog of the SP test. It runs on every server version in the matrix including FB 6, closing the gap left by the SP test's SKIP_ON_FIREBIRD6.
The Issue FirebirdSQL#161 fix touched the entire conv*ToString family — conv{TinyInt, Short,Long,Bigint,Float,Double}ToString plus conv{Date,Time,DateTime}ToString — but only the convLongToString path was exercised by the existing test suite (Issue161_SLongToVarcharViaStoredProcedure / …ViaDml). Any regression in the other conversion helpers would slip through CI. Add NumericToVarcharTest with one compact, loop-based test per helper that is safe to exercise on CI: - SShortToVarcharLoopDml — convShortToString - SBigintToVarcharLoopDml — convBigintToString - FloatToVarcharLoopDml — convFloatToString - DoubleToVarcharLoopDml — convDoubleToString Each binds the relevant SQL_C_* type to a VARCHAR column, executes the same prepared INSERT across a small set of values chosen to exercise 1-, 2- and 3-digit widths plus a negative, and verifies every row committed with no NUL-byte corruption plus a round-trip cast. DML (no stored procedure) is used so nothing in FB 3 / 4 / 5 trips the parameterized-SP handling; the FB 6 master parameterized-query regression is skipped via SKIP_ON_FIREBIRD6() — same rationale as the existing Issue161 tests. SQL_C_TYPE_DATE and SQL_C_TYPE_TIMESTAMP → VARCHAR are deliberately NOT covered here: they expose a separate pre-existing bug (the driver routes ODBC SQL_DATE_STRUCT / SQL_TIMESTAMP_STRUCT through conv helpers that expect Firebird's internal ISC_DATE / ISC_TIMESTAMP encoding) that is unrelated to the length-prefix fix in Issue FirebirdSQL#161 and should be tracked separately.
2 tasks
`GetServerMajorVersion` was atoi-ing `SQLGetInfo(SQL_DBMS_VER)`, but the
driver does not put the Firebird product version in that field — it
constructs it from the implementation / protocol version bytes of
`isc_info_version` (`IscDbc/Attachment.cpp:480`) and currently returns
e.g. `"06.03.1683 WI-V Firebird 5.0"` on Firebird 5.0.3. `atoi("06...")`
is 6 on every supported server, so `SKIP_ON_FIREBIRD6()` fires on FB 3 /
4 / 5 too and every guarded test silently `[ SKIPPED ]`s across the
whole matrix.
Ask the server directly via
`SELECT RDB$GET_CONTEXT('SYSTEM', 'ENGINE_VERSION') FROM RDB$DATABASE`,
which returns a straight "5.0.3" / "6.0.0.xxxx" / "3.0.12" string.
Local FB 5.0.3 run: 224 passing tests (was 200) — 24 tests that had been
silently skipped (the pre-existing `Char*` parameter-conversion cases
plus the Issue FirebirdSQL#161 coverage added in this branch) are now actually
exercised. Local FB 6.0 master still skips them all (correct).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Builds on top of #292. That PR fixed the
conv*ToStringhelpers so numeric C-types bound to a Firebird VARCHAR parameter no longer corrupt the VARYING length prefix, but it only added test coverage for theconvLongToStringpath (the single scenario reported in duckdb/odbc-scanner#161). A regression in any of the sibling helpers —convShortToString,convBigintToString,convFloatToString,convDoubleToString— would slip through CI.This PR adds one compact, loop-based test per sibling helper:
SShortToVarcharLoopDml—SQL_C_SSHORT→convShortToStringSBigintToVarcharLoopDml—SQL_C_SBIGINT→convBigintToStringFloatToVarcharLoopDml—SQL_C_FLOAT→convFloatToStringDoubleToVarcharLoopDml—SQL_C_DOUBLE→convDoubleToStringEach binds the relevant C-type to a VARCHAR column, executes the same prepared
INSERT INTO ... VALUES (?, ?)across a small set of values chosen to exercise 1-, 2- and 3-digit widths plus a negative, and verifies:\0byte,CAST(val AS <original-type>)round-trips for every boundary value.DML (no stored procedure) is used so the tests run everywhere the existing parameterized-statement coverage runs; the FB 6 master parameterized-query regression is skipped via
SKIP_ON_FIREBIRD6()— same rationale as #292's Issue161 tests.Scope notes
SQL_C_TYPE_DATE/SQL_C_TYPE_TIMESTAMP→ VARCHAR are intentionally omitted. The driver routes those C-types throughconvDateToString/convDateTimeToString, which expect Firebird's internalISC_DATE/ISC_TIMESTAMPencoding (a 32-bit int and a QUAD), NOT the ODBC-sideSQL_DATE_STRUCT/SQL_TIMESTAMP_STRUCTthe application actually binds. The resulting VARCHAR is garbage (year 2043, etc.) independent of the length-prefix fix, so this is a separate pre-existing bug that should get its own fix and its own tests.SQL_C_STINYINT/SQL_C_TYPE_TIMEsimilarly untested — low-value coverage.Dependency
Depends on #292. Without that PR's
conv*ToStringlength-prefix fix, every sibling test here exhibits the same silent data loss the SLONG test did (verified locally against the old v3.0.1.21 driver — 11/6 rows stored instead of the expected row count). GitHub will show this PR's diff cleanly once #292 merges.Test plan