[fdcastel]

Summary

Binding a numeric, date or time C type to a Firebird text parameter and executing the statement many times silently corrupted or dropped rows, with SQL_SUCCESS at every step. Found through duckdb/odbc-scanner#161: an odbc_copy_from of 500 rows into a Firebird stored procedure committed 11 of them.

Root cause

Two defects, plus a third found while testing:

1. Wide converter on the Firebird side. With CHARSET=UTF8 the driver maps a UTF8 text parameter to SQL_C_WCHAR, so getAdressFunction picked conv<Type>ToStringW, which writes UTF-16 code units into the Firebird buffer. Firebird stored them as bytes, with embedded NULs. Under CHARSET=NONE the slot maps to SQL_C_CHAR and the byte converter was used, which is why the bug did not reproduce there.
2. Precision read from the mutated sqlvar. After the first execute setSqlLen(actual_length) shrinks the sqlvar, and a rebind re-described the parameter from it, so record->length collapsed to the length of the first value and later values were truncated to one character. This is the rebind-per-row shape odbc-scanner uses.
3. Null flag never cleared. ODBCCONVERT_CHECKNULL sets the Firebird-side null flag on the NULL path and never clears it, so a value bound after a NULL on the same parameter was sent as NULL.

Fix

Three commits, each with the tests that fail without it:

• Routing and null flag. selectToStringConv: for a numeric / date / time C type bound to SQL_C_WCHAR, Firebird-side targets go through the byte variant. These converters emit ASCII only, so the bytes are correct in every charset; application-side targets keep the wide variant. ODBCCONVERT_CHECKNULL and ODBCCONVERT_CHECKNULLW now clear the Firebird-side null flag on the not-null path, as the two sibling macros already did.
• Precision. Sqlda::getPrecision reads INPUT parameters from the prepare-time snapshot (orgSqlProperties), as getColumnDisplaySize already does. OUTPUT and SQL_ARRAY keep the live sqlvar.
• Sibling coverage. Loop tests for Short, Bigint, Float and Double to VARCHAR, previously tests: cover sibling numeric→VARCHAR conversions #293.

An earlier version of this PR rewrote the converters to write a VARYING length prefix into the buffer. That rested on a wrong theory: setHeadSqlVar retypes every Firebird-side text parameter to SQL_TEXT at bind time, so no VARYING buffer ever reaches a converter. That code is gone.

Tests

• ParamConversionsTest.Issue161_*: stored procedure, DML, direct and rebind shapes, plus the CHAR and CHARACTER SET NONE corners of the rebind shape.
• ParamConversionsTest.NullThen*ValueOn*Param: a value bound after a NULL, for SLONG, SSHORT, SBIGINT and DOUBLE on VARCHAR and CHAR.
• NumericToVarcharTest: the four sibling loop tests.

All of them skip on the FB6 snapshot, which aborts parameterized statements server-side (the limitation guarded elsewhere in the suite), and run on FB 3, 4 and 5.

#302 (describe INPUT parameters from the prepared type) is independent of this PR and can merge first.

[fdcastel]

Two follow-ups after the initial push failed the FB-master matrix jobs:

• fb356d6 — rewrote the fix to stop mutating sqltype/sqllen on the Firebird side. The previous setTypeText() + setSqlLen(1) produced SQL_TEXT + sqllen=1 + UTF-8 charset metadata that FB 6 rejects at execute time (Stack overflow on Windows, SEGFAULT on Linux). New approach: write the VARYING [uint16 length][data] layout directly in the pre-allocated buffer and leave the prepared metadata untouched. Added a tiny HeadSqlVar::isSqlVarying() getter so the conversion helpers can discriminate VARYING from TEXT at run-time.
• 3e267bf — guarded the new test with SKIP_ON_FIREBIRD6(), matching the existing convention for the FB 6 parameterized-query regression. The driver fix is still verified by the FB 3 / 4 / 5 matrix jobs and the DuckDB odbc_copy_from reproduction.

All 10 matrix jobs green on the latest push.

[irodushka]

Does this issue only occur when executing a stored procedure? Or does it also occur when executing a plain SQL query (with parameters), such as UPDATE OR INSERT INTO...?

[fdcastel]

Does this issue only occur when executing a stored procedure? Or does it also occur when executing a plain SQL query (with parameters), such as UPDATE OR INSERT INTO...?

Good catch. I ran a few quick tests here, and it does happen in that case as well.

I’m expanding the test coverage now.

[fdcastel]

Follow-up after a question about test coverage — two findings worth recording:

1. The bug is not SP-specific. Empirical check against the old v3.0.1.21 driver:

Dest statement	Old driver (no fix)	This PR
EXECUTE PROCEDURE sp(?, ?)	500 sent, 11 stored ✗	500 stored ✓
UPDATE OR INSERT ... VALUES (?, ?) MATCHING (ID)	500 sent, 11 stored ✗	500 stored ✓

Same failure signature in both cases — 9 single-digit rows + 1 NUL-corrupted "1\0" = 11 total. The bug lives entirely in the conv*ToString path and doesn't care about the statement kind. The odbc-scanner issue body table that showed plain DML as unaffected looks to have been based on a different test run — it's not what actually happens.

Added Issue161_SLongToVarcharViaDml as a DML analog of the existing SP test to prove this explicitly.

2. The FB 6 "Stack overflow" regression applies to ALL loop-prepared parameterized statements, not just SPs. My first attempt at the DML test failed CI on FB 6 master with the same Stack overflow (Windows) / SEGFAULT (Linux) signature on the very first SQLExecute. So SKIP_ON_FIREBIRD6() has to guard the DML test too — same as the SP variant and the existing Char* tests. Locally an older FB 6 snapshot (6.0.0.1910) accepts the pattern, but the fresher snapshot the CI runners download crashes; don't trust green-on-my-machine for FB 6 parameterized work.

3. Sibling conversion coverage. The conv*ToString family this PR fixed has several paths the suite never exercised: conv{Short,Bigint,Float,Double}ToString. A regression in any of those would slip through CI today. Opened #293 with one compact loop-based test per helper (using plain DML; same SKIP_ON_FIREBIRD6 convention). Date / Timestamp → VARCHAR deliberately left out — they expose a separate pre-existing routing bug (driver pipes SQL_{DATE,TIMESTAMP}_STRUCT through helpers that expect Firebird's internal ISC_* encoding) that is unrelated to Issue #161.

All 10 matrix jobs green on both PRs.

[irodushka]

Does this issue only occur when executing a stored procedure? Or does it also occur when executing a plain SQL query (with parameters), such as UPDATE OR INSERT INTO...?

Good catch. I ran a few quick tests here, and it does happen in that case as well.

I’m expanding the test coverage now.

Does this issue only occur when executing a stored procedure? Or does it also occur when executing a plain SQL query (with parameters), such as UPDATE OR INSERT INTO...?

Good catch. I ran a few quick tests here, and it does happen in that case as well.

I’m expanding the test coverage now.

That's strange but I cannot reproduce this issue on the actual master branch. What FB version you're using? I've tried 3, 4 and 5. No data loss, 500 records are inserted and commited, no data corruption.

SELECT COUNT(*) FROM test_table_292 WHERE POSITION(_OCTETS x'00' IN ID) > 0;
0

SELECT COUNT(*), MIN(CAST(ID AS INTEGER)), MAX(CAST(ID AS INTEGER)) FROM test_table_292;
500 | 1 | 500

[fdcastel]

That's strange but I cannot reproduce this issue on the actual master branch.

I built upstream master (eb7072b) locally and the bug does still repro there, but only when the application binds a numeric C-type (SQL_C_SLONG, SQL_C_SBIGINT, …) to a SQL_VARCHAR target.

If your test harness stringifies the integer and binds SQL_C_CHAR, you see all 500 rows with no NUL — that path goes through transferString*ToAllowedType, which has always handled the VARYING length prefix correctly. The broken path is conv<Numeric>ToString.

I put together a minimal standalone reproducer (one .cpp, 4 shapes — direct+char, direct+slong, prepare-once+slong, rebind+slong): repro_variants.cpp. Against upstream eb7072b / FB 5.0.3 / CHARSET=UTF8 it prints:

direct_char      expected=500 count=500 nul_rows=0    OK
direct_slong     expected=500 count=500 nul_rows=500  FAIL
preponce_slong   expected=500 count=500 nul_rows=500  FAIL
rebind_slong     expected=500 count=11  nul_rows=1    FAIL

With this PR all four shapes are OK. Could you share the snippet you used (or point me at the C-type you bind)? If it's SQL_C_CHAR the no-repro outcome is expected and both of us are right.

[fdcastel]

Also worth noting while I was at it: my gtest cases here are in fact SKIPPED on every matrix job because the driver's SQL_DBMS_VER string starts with "06.03…" on Firebird 5 too (it reports the implementation / protocol number, not the product major), so SKIP_ON_FIREBIRD6 fires for FB 5 as well.

That's a pre-existing issue — I'll open a small follow-up fixing GetServerMajorVersion to query rdb$get_context('ENGINE_VERSION') so the coverage actually runs.

[irodushka]

@fdcastel
I look into your sample & send you mine, but not today... I would like to postpone discussing this story for now, for I need to investigate the issue. And frankly, at first glance you fix looks not very optimal/suitable for me (was it created by AI?:)) maybe we should do it in more solid and clean way.

[fdcastel]

Sure, take your time!

That was just a first pass (yep, AI-generated 😄) to show it works. Now we can improve it.
(Remembering the 3 rules) 😆

I just came across this -- while working on a totally a different project.

[fdcastel]

Hi @irodushka — took your feedback and pushed a cleaner v2 on top of the PR: e996b2b (the refactor) + 538a993 (a small cosmetic). Net −70 lines against the earlier fix commits.

Three named helpers replace the scattered inlining:

1. HeadSqlVar::writeStringData(src, len) — new inline method on the IscDbc interface, beside setSqlLen / setSqlData. Owns the Firebird buffer-layout rule: [uint16 len][data] for SQL_VARYING (no sqlvar mutation), bare bytes + effective sqllen for SQL_TEXT. Replaces the peek-only isSqlVarying() getter — behavior, not a peek.

2. writeConvertedString(pointer, indicatorTo, to, src, len) — file-scope adapter in OdbcConvert.cpp. Every numeric / date / time → string converter now produces a local ASCII buffer and calls this one helper; no more *(unsigned short*)pointer = len; sprinkled across 18 sites.

3. chooseNumericToStringConv(to, byteFn, wideFn) — tiny inline that collapses the 8 identical SQL_C_WCHAR dispatch blocks in getAdressFunction to one line each. The rationale comment lives once, at the helper.

Net effect: the ODBC conversion layer no longer writes VARYING length prefixes or reads sqltype == SQL_VARYING — that is IscDbc's concern now.

I also rewrote the Sqlda::getPrecision comment to frame its orgVarSqlProperties read for INPUT parameters as a symmetry with the pre-existing getColumnDisplaySize pattern (prepared-parameter precision is immutable), not a workaround. Code behavior unchanged.

One small cosmetic follow-up — 538a993 drops a stray Issue #161: prefix from a test section banner that I had carried over without thinking. duckdb/odbc-scanner#161 is in a different repo and long closed, so citing it here just added noise to git log. My bad.

All 10 matrix jobs green on both commits. Happy to split, rearrange, or dig deeper into any piece if something still looks off.

[irodushka]

Aha... you gonna blow my brains)

So the only failing scenario is to prepare the query, then on each iteration - reset parameters, bind them every time & exеcute?.. I mean "rebind_slong". All the others - direct_char, direct_slong and preponce_slong - are OK.

And "preponce_slong" is the only sane scenario. All the rest are strange and extremely suboptimal.

Okay, at least I got it and I see the misbehaviour (although, despite this, I would not recommend anyone to use this approach when working with sql queries 😃).

[irodushka]

@fdcastel

Can you please check a bit simplified fix...

Change the line OdbcConvert.cpp:1306
int len = to->length;
to
int len = to->octetLength;

and nothing more. I mean - instead of all your changes in this PR. Well, and the line 1398 for the Wide version.
Will it blend help?..

[fdcastel]

Unfortunately it's not so simple.

I built your one-liner against master and measured all four shapes. The short version: it recovers rebind_slong (row count 11 → 500) but leaves direct_slong and preponce_slong silently storing 500 NUL-corrupted PKs, same as pure master. Row counts stay at 500 there; the corruption is in the stored bytes.

Shape	master (no fix)	master + your one-liner	this PR
direct_char	count=500 nul=0 OK	count=500 nul=0 OK	count=500 nul=0 OK
direct_slong	count=500 nul=500 FAIL	count=500 nul=500 FAIL	count=500 nul=0 OK
preponce_slong	count=500 nul=500 FAIL	count=500 nul=500 FAIL	count=500 nul=0 OK
rebind_slong	count=11 nul=1 FAIL	count=500 nul=1 FAIL	count=500 nul=0 OK

(FB 5.0.3, CHARSET=UTF8, 500 rows, nul_rows = POSITION(_OCTETS x'00' IN ID) > 0.)

Which lines up with there being two separate bugs the PR addresses:

• Bug A — conv<Numeric>ToString writes ASCII digits at offset 0 of a SQL_VARYING target buffer (where FB expects a uint16 length prefix) and then calls setSqlLen(actual_len). FB master reads the digits as the prefix ('1','\0' = 49,
  '1','0' = 12337, …) and ends up storing bytes from past the copy range — the embedded NULs. Your one-liner doesn't touch either the offset-0 write or the setSqlLen, so this bug survives, and it fires on every shape that routes through conv*ToString, not just the rebind one.

• Bug B — Sqlda::getPrecision was reading the mutated sqlvar, so record->length collapsed to 1 after the first execute and multi-digit IDs got cut to their first character in the rebind path. Your octetLength switch sidesteps this, because octetLength is populated from getColumnDisplaySize, which already reads from orgVarSqlProperties (the immutable prepare-time snapshot). This is exactly why rebind_slong goes 11 → 500 with your change. Nice spot.

The PR addresses both: the VARYING-vs-TEXT write rule now lives in HeadSqlVar::writeStringData (offset 0 = uint16 prefix, data at offset 2, sqltype/sqllen untouched — and as a side-effect Sqlda::checkAndRebuild() no longer fires for these parameters at all), and Sqlda::getPrecision reads from orgVarSqlProperties for INPUT so the rebind path is immune too. Full patch is the e996b2b commit on top of fb356d6.

On "preponce_slong is the only sane scenario" — fair call, but direct_slong is just SQLExecDirect with a numeric parameter and rebind_slong is what odbc_scanner's odbc_copy_from (and a fair number of high-level wrappers) do under the hood. All three are legal ODBC and all three currently corrupt data on master; I'd rather not pick which ones we're willing to leave broken.

[fdcastel]

Pushed e40dfa8 — two additional tests in test_param_conversions.cpp so the PR's regression suite covers both bugs, not just one.

The original two tests (Issue161_SLongToVarcharViaStoredProcedure, Issue161_SLongToVarcharViaDml) both bind once and execute 500 times. That exercises Bug A — conv<Numeric>ToString writing over the SQL_VARYING length prefix — but not Bug B (Sqlda::getPrecision reading from the mutated sqlvar), because the preponce shape never re-enters defFromMetaDataIn and so never re-reads getPrecision after the first execute. A regression that reintroduced the pre-PR record->length = getPrecision(mutated_sqlvar) read would have slipped past both tests.

The two new tests

Test	Shape	Exercises
Issue161_SLongToVarcharViaDmlRebind	rebind_slong	Bug A + Bug B — re-binds per row
Issue161_SLongToVarcharViaDmlDirect	direct_slong	Bug A via SQLExecDirect (no prepare cache)

• Rebind variant: SQLPrepare once, then per row SQLFreeStmt(SQL_RESET_PARAMS) + SQLBindParameter(×2) +
  SQLExecute. This is what odbc-scanner's odbc_copy_from does internally, and the only shape where Bug B actually fires (a per-row SQLBindParameter forces defFromMetaDataIn to re-read getPrecision, which pre-PR returned the shrunken length from the mutated sqlvar).
• Direct variant: SQLExecDirect per row with SQL_C_SLONG, no prepared-statement context carried between iterations. Guards against any state-leak regression where the per-execute reset path diverges from the per-prepare path.

[fdcastel]

P.S.: I attempted to rename the (very poorly named) branch for this PR, but GitHub ended up closing the PR incorrectly.

So I’m leaving it as is for now.

[irodushka]

Oh my Lord... That's not addressed to you, @fdcastel, just an idle talk.

Finally I got it. The misbehavior occures when I set "CHARACTER SET UTF8" for varchar fields. And it works ok (with the 1-line fix of cause) when I use NONE charset.

The reason is - if we use the UTF8 charset, the WIDE int->string conv function is called, and there's an extremely bad place in it:

		{																						\
			char tempBuf [256];																	\
			strcpy( tempBuf, string );															\
			from->MbsToWcs( (wchar_t *)string, tempBuf, len );									\
			((wchar_t *)string)[len] = L'\0';													\
			len *=2;																			\
		}	

Such a familiar code, isn't it?)
It writes fixed 2-byte characters on Windows and 4-byte - on Linux. The lengths if hardcoded-multiplied by 2. All this logic is invalid in the both cases because UTF8 is not the widechar encoding, but the multi-byte, 1-4 bytes for one character, and for we use the pure ASCII, it must be 1 byte for character here.

Your fix works because you've added the gating logic, I mean chooseNumericToStringConv, and it always use the single-byte conv function when converting from the client var to the FB parameter. So the WIDE conv function is never called, and the toxic code above never works)
BUT I doubt this approach is correct... you're copying data as plain bytes from the client side buffer to the UTF8 FB buffer. It should be ok when the client-side is ASCII (as in the test is) - each ASCII byte corresponds exactly to one UTF8 byte. But what if it's not ASCII?

And the last - it does not relate to VARCHAR at all) Please try to create the test table as

RECREATE TABLE ISSUE161_T(
ID CHAR(20) CHARACTER SET UTF8 NOT NULL PRIMARY KEY,
NAME CHAR(100) CHARACTER SET UTF8
)

and run your tests...

You can also create table with CHARACTER SET NONE and check. I ran the test on Linux, not Windows, and I got 4/4 OK with NONE charset (CHAR or VARCHAR) and 3/4 FAIL with UTF8 (CHAR or VARCHAR). I expect you'll get similar results on Windows.

[irodushka]

you can:

1. make the 1-line fix in 2 places (for char & for widechar conv, length->octetLength)
2. add the condition if(from->isIndicatorSqlDa) before the bad code block in the widechar conv (with from->MbsToWcs...)

As for me, this solves the problem completely. Linux.

P.S. And yes, it is true that when converting from any data type (except string) to string, we cannot get anything but ASCII characters... so your control logic (chooseNumericToStringConv) in these cases looks correct... Just fix the naming - it's not always Numeric. Maybe selectToStringConv?..

[fdcastel]

Almost 😅

TL;DR Your minimal fix (3 lines) works for the write direction: my v3 theory was wrong. But unfortunately it breaks 4 pre-existing tests in the read direction.

---

Tested both of your proposals empirically:

CHAR target and CHARACTER SET NONE

• pushed 220eb3c and 9a1a432.
• Three new gtest cases covering CHAR(20) + UTF8, VARCHAR(20) + NONE, CHAR(20) + NONE in the rebind shape;
  • a cherry-picked rdb$get_context fix for GetServerMajorVersion so SKIP_ON_FIREBIRD6 stops firing on FB 5.
• Combined effect on run 24850170760:
  • 7/7 Issue161 tests
  • Passed on ubuntu-22.04 / 5.0.3 AND on windows-2022 / 5.0.3, correctly Skipped on FB 6 master
  • so the coverage is real on both OSes now, not silent skips.

Your 3-line minimal fix (line 1306 + 1387 + from->isIndicatorSqlDagate)

• I applied it on top of clean master, pushed to a scratch branch, and ran the full CI matrix (run 24851385888, branch since deleted).
• On the write-direction matrix it passes exactly as you described on Linux: all 16 (column type × charset × shape) combinations OK, including CHAR + UTF8.
• My earlier claim that the VARYING prefix stomp would persist under your minimal fix was wrong: the OO-API path via checkAndRebuild() handles the sqllen mutation fine. Apologies for the sloppy theoretical argument.

But

• the minimal fix regressed 4 pre-existing read-direction tests on every matrix job: DataTypeTest.NumericPrecision, ResultConversionsTest.{IntegerToChar, NumericToChar, BooleanToChar}.
• The reason is that s/to->length/to->octetLength/ at line 1306 affects both directions
  • and on the read direction (FB column → client SQL_C_CHAR buffer) to->length and to->octetLength are not the same:
  • to->length is the declared capacity (SQL_DESC_LENGTH) while to->octetLength can come from the source column's display size and be smaller.
• Using octetLength as the truncation cap on the read path writes only part of the formatted number and the test fails.

This PR's chooseNumericToStringConv avoids that: it routes only when to->isIndicatorSqlDa is true (write-to-FB), leaving the read path on the wide variant unchanged and to->length semantics untouched.

I can't see a tweak to your 3-line approach that keeps it 3 lines without introducing the same to->isIndicatorSqlDa branching we already have.

On chooseNumericToStringConv naming

Agreed, the helper covers date/time as well, not just numerics. I'll rename to selectToStringConv in a follow-up commit on this PR.

On "what if not ASCII"

• the specific converters this PR touches (conv<Numeric|Date|Time|DateTime>ToString{,W}) emit only ASCII: 0–9, ., -, :, , e/E, *.
• No path in those macros produces a byte > 0x7F, so routing the UTF8 / WVARCHAR target through the byte variant is bit-identical regardless of charset.
• The character-data family (conv<String>ToString*) is a different code path, unchanged by this PR, and continues to flow through transferString*ToAllowedType with charset awareness.

Next steps

I need TWO beers, now.

[irodushka]

No need to use the exact 3-lines patch, it was for illustration only. I think it's ok to separate logic for read&write directions. And yes, that legacy code is very, very, VERY entangled) чт, 23 апр. 2026 г., 21:43 F.D.Castel ***@***.***>:

…

*fdcastel* left a comment (FirebirdSQL/firebird-odbc-driver#292) <#292?email_source=notifications&email_token=AAKZQLWDIDVTKYZDM4QOMOT4XJP4JA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMZQGY4TENBRG4YKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4306924170> Almost 😅 *TL;DR* Your minimal fix (3 lines) works for the *write* direction: my v3 theory was wrong. But unfortunately it breaks 4 pre-existing tests in the *read* direction. ------------------------------ Tested both of your proposals empirically: CHAR target and CHARACTER SET NONE - pushed 220eb3c <fdcastel@220eb3c> and 9a1a432 <fdcastel@9a1a432>. - Three new gtest cases covering CHAR(20) + UTF8, VARCHAR(20) + NONE, CHAR(20) + NONE in the rebind shape; - a cherry-picked rdb$get_context fix for GetServerMajorVersion so SKIP_ON_FIREBIRD6 stops firing on FB 5. - Combined effect on run 24850170760 <https://github.com/fdcastel/firebird-odbc-driver/actions/runs/24850170760>: - 7/7 Issue161 tests - Passed on ubuntu-22.04 / 5.0.3 AND on windows-2022 / 5.0.3, correctly Skipped on FB 6 master - so the coverage is real on both OSes now, not silent skips. Your 3-line minimal fix (line 1306 + 1387 + from->isIndicatorSqlDagate) - I applied it on top of clean master, pushed to a scratch branch, and ran the full CI matrix (run 24851385888 <https://github.com/fdcastel/firebird-odbc-driver/actions/runs/24851385888>, branch since deleted). - On the write-direction matrix *it passes exactly as you described on Linux*: all 16 (column type × charset × shape) combinations OK, including CHAR + UTF8. - *My earlier claim that the VARYING prefix stomp would persist under your minimal fix was wrong*: the OO-API path via checkAndRebuild() handles the sqllen mutation fine. Apologies for the sloppy theoretical argument. But - the minimal fix regressed 4 pre-existing read-direction tests on every matrix job: DataTypeTest.NumericPrecision, ResultConversionsTest.{IntegerToChar, NumericToChar, BooleanToChar}. - The reason is that s/to->length/to->octetLength/ at line 1306 *affects both directions* - and on the read direction (FB column → client SQL_C_CHAR buffer) to->length and to->octetLength are not the same: - to->length is the declared capacity (SQL_DESC_LENGTH) while to->octetLength can come from the source column's display size and be smaller. - Using octetLength as the truncation cap on the read path writes only part of the formatted number and the test fails. This PR's chooseNumericToStringConv avoids that: it routes only when to->isIndicatorSqlDa is true (write-to-FB), leaving the read path on the wide variant unchanged and to->length semantics untouched. I can't see a tweak to your 3-line approach that keeps it 3 lines without introducing the same to->isIndicatorSqlDa branching we already have. On chooseNumericToStringConv naming Agreed, the helper covers date/time as well, not just numerics. I'll rename to selectToStringConv in a follow-up commit on this PR. On "what if not ASCII" - the specific converters this PR touches ( conv<Numeric|Date|Time|DateTime>ToString{,W}) emit only ASCII: 0–9, ., -, :, , e/E, *. - No path in those macros produces a byte > 0x7F, so routing the UTF8 / WVARCHAR target through the byte variant is bit-identical regardless of charset. - The character-data family (conv<String>ToString*) is a different code path, unchanged by this PR, and continues to flow through transferString*ToAllowedType with charset awareness. Next steps I need TWO beers, now. — Reply to this email directly, view it on GitHub <#292?email_source=notifications&email_token=AAKZQLWDIDVTKYZDM4QOMOT4XJP4JA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMZQGY4TENBRG4YKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJNLQOJPWG33NNVSW45C7N5YGK3S7MNWGSY3L#issuecomment-4306924170>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKZQLRR2JK3F6F2SIRPWN34XJP4JAVCNFSM6AAAAACYBN4RGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DGMBWHEZDIMJXGA> . Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/AAKZQLUBYJAGO37ENWG7Z2L4XJP4JA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMZQGY4TENBRG4YKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y> and Android <https://github.com/notifications/mobile/android/AAKZQLQDMW3MMGFIQ6EE7BL4XJP4JA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMZQGY4TENBRG4YKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>. Download it today! You are receiving this because you were mentioned.Message ID: ***@***.***>

[irodushka]

@fdcastel
so what should we do with this PR?..

[fdcastel]

Sorry for the silence — I'd drafted this reply weeks ago and then never actually posted it. Posting now so you have the empirical answers to both of your last two comments.

TL;DR

• chooseNumericToStringConv → selectToStringConv: done in b6ad723. You were right — the dispatcher covers date/time too, not just numerics.
• CHAR target and CHARACTER SET NONE coverage: added in 220eb3c — three new Issue161_* gtest cases for CHAR + UTF8, VARCHAR + NONE, CHAR + NONE via a RunIssue161RebindVariant(pkColumnDef, nameColumnDef) helper. All 7 Issue161 tests now actually run on Linux x64 and Windows x64 against FB 5.0.3 — Passed, not Skipped — and correctly Skipped on FB 6 master.
• Your 3-line minimal fix (line 1306 + 1387 + from->isIndicatorSqlDa gate): I built it and ran the full CI matrix. The 16-cell write-direction matrix is green exactly as you described. But it regresses 4 pre-existing read-direction tests on every matrix job — full details below.
• "Copying bytes... what if it's not ASCII?": legitimate concern in general, but for the specific converters this PR touches (conv<Numeric|Date|Time|DateTime>ToString{,W}) the output is ASCII by construction. No path emits a byte > 0x7F.

1. Column-type × charset matrix on Windows (FB 5.0.3 / CLIENT=UTF8)

Three driver states × four PK column definitions × four bind shapes, via tmp/repro_variants.cpp + the four tmp/setup_{varchar,char}_{utf8,none}.sql scripts.

master (no fix)

Target column	direct_char	direct_slong	preponce_slong	rebind_slong
VARCHAR(20) CHARACTER SET UTF8	500 / nul=0 OK	500 / nul=500 FAIL	500 / nul=500 FAIL	11 / nul=1 FAIL
CHAR(20) CHARACTER SET UTF8	500 / nul=0 OK	500 / nul=500 FAIL	500 / nul=500 FAIL	11 / nul=1 FAIL
VARCHAR(20) CHARACTER SET NONE	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK	10 / nul=0 FAIL
CHAR(20) CHARACTER SET NONE	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK	10 / nul=0 FAIL

master + your minimal fix (both line changes + from->isIndicatorSqlDa gate)

Target column	direct_char	direct_slong	preponce_slong	rebind_slong
VARCHAR(20) CHARACTER SET UTF8	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK
CHAR(20) CHARACTER SET UTF8	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK
VARCHAR(20) CHARACTER SET NONE	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK
CHAR(20) CHARACTER SET NONE	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK

This PR

Target column	direct_char	direct_slong	preponce_slong	rebind_slong
VARCHAR(20) CHARACTER SET UTF8	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK
CHAR(20) CHARACTER SET UTF8	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK
VARCHAR(20) CHARACTER SET NONE	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK
CHAR(20) CHARACTER SET NONE	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK	500 / nul=0 OK

The first two tables confirm your Linux observations on Windows as well. My v3 reply argued theoretically that your minimal fix would leave the VARYING-prefix stomp in place — that was wrong. FB's OO-API checkAndRebuild() path handles the sqllen mutation cleanly; offset 0..1 is not reinterpreted the way the legacy XSQLVAR code would. Sorry for the sloppy theoretical argument — should have measured first.

Stored values are correct under the minimal fix too:

SELECT ID, OCTET_LENGTH(ID), CHAR_LENGTH(ID) FROM ISSUE161_T WHERE ID IN ('1','10','100','500');
ID   OCTET_LENGTH  CHAR_LENGTH
1               1            1
10              2            2
100             3            3
500             3            3

So at the write-direction the minimal fix is functionally equivalent for this workload.

2. But the minimal fix regresses the read direction

When I pushed the minimal fix to a scratch branch and ran the full CI matrix (run 24851385888, branch since deleted), all 10 matrix jobs failed — and not on the new Issue161 cases. They failed on 4 unrelated, pre-existing tests in the read direction:

• DataTypeTest.NumericPrecision
• ResultConversionsTest.IntegerToChar
• ResultConversionsTest.NumericToChar
• ResultConversionsTest.BooleanToChar

Locally reproducible on Windows x64 / FB 5.0.3 with the minimal fix applied:

[ RUN      ] DataTypeTest.NumericPrecision
test_data_types.cpp(132): error: The difference between dval and
12345678901234.5678 is 12345678901234.568, which exceeds 0.001
[  FAILED  ] DataTypeTest.NumericPrecision
...
[  FAILED  ] ResultConversionsTest.IntegerToChar
[  FAILED  ] ResultConversionsTest.NumericToChar
[  FAILED  ] ResultConversionsTest.BooleanToChar

Why

These tests exercise the read direction — SELECT integer_column with SQLBindCol(SQL_C_CHAR, buf, 32), then SQLFetch. Path: conv<Long>ToString runs with from = FB column (isIndicatorSqlDa = true) and to = client SQL_C_CHAR buffer (isIndicatorSqlDa = false).

The minimal fix swaps int len = to->length; for int len = to->octetLength; at the top of the byte macro. For a FB-side to, the two are pragmatically equivalent (both represent the declared column length). For a client-side to, they are not: to->length is SQL_DESC_LENGTH (the application-visible capacity in characters), while to->octetLength is SQL_DESC_OCTET_LENGTH (the raw buffer size in bytes), and they can diverge — in particular SQL_C_CHAR bindings made with a 32-byte buffer for a NUMERIC(18,4) target end up with length = 32 but octetLength coming from the source column's display size, which is 21 for NUMERIC(18,4). Using octetLength as the truncation cap writes only part of the number and breaks the read test.

This is exactly why this PR keeps to->length in the byte macro as-is and instead routes the write-direction to the byte variant from getAdressFunction via selectToStringConv (was chooseNumericToStringConv before b6ad723). The path selection depends on to->isIndicatorSqlDa, not on a universal substitution — so the read direction is untouched.

I did not catch this divergence theoretically; I caught it by running the full test suite on CI.

3. The non-ASCII concern, specifically for these converters

For the conv<Numeric|Date|Time|DateTime>ToString{,W} family — everything this PR modifies — the source-side output is ASCII by construction:

• conv{TinyInt,Short,Long,Bigint}ToString: 0–9, ., -, *
• convFloatToString / convDoubleToString: 0–9, ., -, e, E
• convDateToString / convTimeToString / convDateTimeToString: 0–9, -, :, , .

No code path in these macros can produce a byte > 0x7F, so routing the UTF8 / WVARCHAR target through the byte variant is safe: ASCII is bit-identical under UTF8 / NONE / ISO8859_1 / WIN1252.

The non-ASCII concern would be legitimate for convString*ToString* — the conversion family for character data — but this PR does not touch those. They already go through transferString*ToAllowedType which honours the charset and is exercised by the WCharTest matrix.

4. Test coverage status

Pushed in 220eb3c and f412b68:

• Issue161_SLongToCharViaDmlRebind — CHAR(20) CHARACTER SET UTF8
• Issue161_SLongToVarcharViaDmlRebindCharsetNone — VARCHAR(20) CHARACTER SET NONE
• Issue161_SLongToCharViaDmlRebindCharsetNone — CHAR(20) CHARACTER SET NONE

…on top of the existing 4 (Issue161_SLongToVarcharViaStoredProcedure, Issue161_SLongToVarcharViaDml, Issue161_SLongToVarcharViaDmlRebind, Issue161_SLongToVarcharViaDmlDirect).

Plus the cherry-picked rdb$get_context test-helper fix — same content as fix/sql-dbms-ver (now merged in #294); kept here so the Issue161 tests actually execute on FB 3 / 4 / 5 against the driver-side SQL_DBMS_VER fix.

CI on the rebased + renamed tip (run 25896309764):

Job	Issue161 tests	Result
build-and-test (ubuntu-22.04, linux-x64)	7	7 Passed, 0 Skipped
build-and-test (windows-2022, windows-x64)	7	7 Passed, 0 Skipped
build-and-test (ubuntu-22.04, master / FB 6)	7	0 Passed, 7 Skipped (correct — SKIP_ON_FIREBIRD6 firing only on FB 6 as intended)

All 10 matrix jobs green.

Status & branch

Rebased on current master (b64c011, which now includes #294's SQL_DBMS_VER driver fix and #297's SQLCopyDesc fix). Branch tip: b6ad723 (the rename). No conflicts.

PR #293 (sibling test coverage for Short / Bigint / Float / Double) is also rebased on the new master; it'll reduce to a clean tests-only diff once this lands.

Happy to do whatever else makes review easier — split commits differently, drop the rdb$get_context cherry-pick now that #294 fixes the driver side, etc. Just say the word.

[fdcastel]

Added three commits: the fix for #300 and a second defect on the same code path that only becomes testable here.

Describe INPUT parameters from the prepared type, not the last bind — the fix for #300. A parameter's sqlvar is rewritten on every bind to match the C type being transferred (setHeadSqlVar, at the top of getAdressFunction), but the driver described the parameter from that same live sqlvar, which closes a loop: a character transfer sets SQL_TEXT, the next describe therefore reports CHAR, that picks a string converter again, and the parameter stays text for the life of the statement. Describing INPUT from orgSqlProperties — what getColumnDisplaySize and, in this PR, getPrecision already do — breaks it, and setHeadSqlVar then shapes the sqlvar back on its own. The two inline SqldaDir == SQLDA_INPUT conditions are now one describedSqlProperties() helper; getPrecision still has its own because of the SQL_ARRAY guard added here.

Clear a parameter's null flag when a converted string is not null — ODBCCONVERT_CHECKNULL, shared by the whole conv<Type>ToString family this PR rewrites, sets the target null indicator on the null path and never clears it on the other one, unlike ODBCCONVERT_CHECKNULL_COMMON and ODBCCONVERT_CHECKNULL_SQLDA. For a sqlda target that indicator is the parameter's own null flag, and it survives between executes — so a value bound after a NULL on the same parameter went to the server as NULL, with SQL_SUCCESS. No rebind and no SQL_C_DEFAULT needed: one binding whose indicator toggles is enough.

tests: cover a value bound after a NULL on a character parameter — six cases for that, plus the five that came with the #300 commit. All eleven fail on master and pass here.

Why the second one belongs in this PR rather than in #302: on master its payoff is invisible, because clearing the flag only exposes the truncation this PR fixes (master writes 6 for 60). Measured on Windows against Firebird 5.0.3 with the MS driver manager, one binding, NULL row then value row, VARCHAR(20) and CHAR(20) targets × SQL_C_SLONG / SSHORT / SBIGINT / DOUBLE:

build	result
master	8/8 rows silently NULL
this PR before these commits	8/8 rows silently NULL
+ null-flag fix only	8/8 truncated (6 for 60)
this PR now	8/8 correct

#302 carries the #300 commit on its own for a master-based merge; whichever of the two lands first makes that commit a no-op in the other.

Full suite green in all three charset configurations (141 passed, 10 pre-existing skips).

[fdcastel]

Sorry for the long silence; other professional tasks took my available time.

To answer your question: I rebuilt the branch from current master with only what you agreed with, and force-pushed with lease so this thread stays. Three commits:

1. selectToStringConv routing (renamed as you asked), plus clearing the Firebird-side null flag in ODBCCONVERT_CHECKNULL / CHECKNULLW. Tests: stored procedure, DML, direct, and NULL-then-value.
2. getPrecision from the prepare-time snapshot for INPUT parameters. Tests: the rebind shape, including the CHAR and CHARACTER SET NONE corners you asked for.
3. The four sibling-converter loop tests from tests: cover sibling numeric→VARCHAR conversions #293, which I am closing.

Gone: writeStringData, writeConvertedString and the VARYING-prefix write. setHeadSqlVar retypes every Firebird-side text parameter to SQL_TEXT at bind time, so no VARYING buffer ever reaches a converter; that part of the earlier fix addressed a case that cannot happen. The #300 commit is out of here too; it stays in #302, which stands alone and can merge first.

The tests declare CHARACTER SET UTF8 on their columns so they reproduce on any database. Verified on FB 5.0.3 with CHARSET=UTF8 and NONE: all 17 new tests fail on current master and pass here; full suite 240 passed, 0 failed passed, 0 failed.