Most BCP improvements from NCBI, as of May 2024

[ucko]

This PR roughly corresponds to the primarily BCP-related commits from #555, adding 02aa63c per dependency considerations and omitting a couple of commits that can receive individual PRs, which I'll open shortly.

[ucko]

I've force-pushed a corrected version superseding #570.

[ucko]

Meanwhile, I'm looking to extend this series with BCP-specific tuneups superseding #573. To that end, I wanted to compare behavior for three cases: normal BCP in, BCP into an alternate version of the table where column types are all VARCHAR(64) (to confirm that conversion to VARCHAR(n) is OK when truncation definitely won't be needed), and BCP into an alternate version where they're all VARCHAR(1) (to see what happens when truncation definitely will be needed), using ctlib/unittests/blk_in.c and dblib/unittests/bcp.c as starting points.

On the DBLIB front, I found that all three cases fully succeeded when built against this branch, but VARCHAR(1) failed for at least one column (details TBD) when using proprietary libraries, so more strictness may actually be in order. On the CTLIB front, something appears to be off altogether, as even the initial run with the usual column types fails with

Open Client Message: 0x608010 0x6259b0
number 0x1010104 layer 1 origin 1 severity 1 number 4
msgstring: blk_describe(): blk layer: user error: Parameter colnum has an illegal value of 1
osstring: (null)
blk_describe(1) failed
Open Client Message: 0x608010 0x6259b0
number 0x1010104 layer 1 origin 1 severity 1 number 4
msgstring: blk_describe(): blk layer: user error: Parameter colnum has an illegal value of 2
osstring: (null)
blk_describe(2) failed
[...]

Any idea what I might be missing? I took care to predefine SYB_LP64 and verified that only FreeTDS-specific headers come from FreeTDS.

UPDATE: Those errors turned out to stem from the use of CS_VERSION_100 and BLK_VERSION_100; advancing the requested version helped.

[ucko]

Here are my summarized comparison results (comparing against version 16.0 of Sybase's client libraries), where *B is (VAR)BINARY; *C is (VAR)CHAR; B, C, VB, and VC are respectively BINARY, CHAR, VARBINARY, and VARCHAR; *DT is (SMALL)DATETIME; *$ is (SMALL)MONEY; and # is any numerical type -- BIT, any variant of INT, REAL, FLOAT, DECIMAL, or NUMERIC:

CTLIB BCP truncation differences w/CS layer messages ignored (see below for corrected version):

FROM\TO	*B(1)	*B(64)	*C(1)	*C(64)
C(10)	a	a	b	OK
VC(10)	a	a	c	OK
B(10)	b	OK	c/d	OK
VB(10)	e	OK	e	OK
*DT	f/g	g	c	OK
*$/#	c/h	OK	e/h	OK

• a)
  • Sybase: clearly expects hex digits; behavior depends on what it sees first:
    • non-hex char among first 2n: rowxfer fails w/syntax error
    • excess char when length is implicit [C(n)]: xfer noisily truncates but OK(!)
    • excess char when length is given [VC(n)]: rowxfer fails w/overflow error
  • FreeTDS: rowxfer silently fails in all those cases
• b) silently truncates under Sybase (but OK), fails under FreeTDS
• c) noisily truncates in rowxfer under Sybase (but OK), fails under FreeTDS
• d) B(n) -> *C(m): silently truncates under Sybase when 4 <= m < 2n; the cutoff of 4 suggests a leading 0x, but VB(n) -> *C(2n) is OK
• e) rowxfer fails under both, (overflow) error only under Sybase
• f) S: bind fails (type mismatch), xfer OK (!); F: xfer fails; (g) if NULL
• g) rowxfer always OK, but Sybase bind fails (type mismatch)
• h) BIT is always OK, as is TINYINT -> *BINARY(1)

DBLIB BCP truncation differences (also corrected below):

FROM\TO	*B(1)	*B(64)	*C(1)	*C(64)
*C(10)	a	a	b	OK
B(10)	c	OK	b/d	OK
VB(10)	b	OK	b/d	OK
*DT/*$	b	OK	b	OK
#	b/e	OK/f	b/e	OK/f

• a)
  • Sybase: syntax error if non-hex char among first 2n, otherwise overflow error as appropriate
  • FreeTDS: syntax error if non-hex char present at all, otherwise silently truncates
• b) conversion overflow error (20049) from Sybase, silent truncation by FreeTDS
• c) oversized row error (20074) from Sybase, silent truncation by FreeTDS
• d) *B(n) -> *C(m): silently truncates under Sybase when 4 <= m < 2n
• e) BIT is always OK, as is TINYINT -> *BINARY(1)
• f) Sybase crashes with heap corruption for DECIMAL and NUMERIC

[ucko]

I'll turn back to other PRs for now, but do still plan to add a proper replacement for #573 at some point.

[ucko]

Ah, blk_in (like most ctlib unit tests) doesn't register a handler for messages from the CS layer, just from the CT layer and the server; it looks like resurveying with one in place will make a difference, at least when running against FreeTDS.

[ucko]

CTLIB BCP truncation differences w/CS layer messages printed, and more corner cases examined

General notes

• This behavior is consistent between 15.7 and 16.0 libraries when using {CS,BLK}_VERSION_125 and up.
  • blk_describe fails altogether under {CS,BLK}_VERSION_120 and below.
  • blk_rowxfer started allowing noisy truncation [(1, 2, 1, 42)] sometime between 12.5 (which issued that same error but then failed) and 15.7; I don't have any installations with intermediate versions, but even 16.0 is a decade old. (At any rate, all other cases, including those that encounter silent truncation, behave the same across versions, modulo 12.5's more limited data type support.)
• LC and LB are LONGCHAR and LONGBINARY, which act like base versions; L?C and L?B are short for C/LC and B/LB respectively; *DT now additionally covers DATE, TIME, and BIG(DATE)TIME; L and USH are LONG and USHORT, which do not act like *INT.
• UNICHAR is out of scope per broader differences -- Sybase expects even-length input, which it inserts into binary columns as is modulo possible truncation (but does attempt to convert when inserting into string columns), whereas FreeTDS treats it like (LONG)CHAR.
• DECIMAL and NUMERIC also work differently, but only in the context of binary columns -- Sybase stores just the main body (and drops trailing NUL bytes as usual), whereas FreeTDS stores the whole thing (space permitting), complete with trailing NUL bytes per its usual practice.

FROM\TO	*B(1)	*B(64)	*C(1)	*C(64)
L?C(10)	a	a	b	OK
VC(10)	a	a	c	OK
L?B(10)	b	OK	c	OK
VB(10)	d	OK	d	OK
*DT	e/f	f	c	g
*$/#	c/h	OK	d/h	i
L/USH	j	j	j	j

• a) Both expect hex digits with optional leading 0x (and effectively prepend a 0 to the body if the length is odd), but their behavior when something is off otherwise varies.
  • Sybase: clearly expects hex digits; behavior depends on what it sees first:
    • non-hex char among first 2n(+2): rowxfer fails w/syntax error
      (1, 2, 1, 26) - blk_rowxfer(): blk layer: internal BLK-Library error: Failed in conversion routine - syntax error. col = # row = #.
    • excess char w/CS_NULLTERM [C(n)/LC(n)]: xfer noisily truncates but OK(!); if the accompanying full length is odd, effectively prepends a zero and discards the last digit even if the initial run of hex digits has even length.
      (1, 2, 1, 42) - blk_rowxfer(): blk layer: internal BLK-Library error: Data truncated while doing local character set conversion. col = # row = #.
    • excess char when length is restated [VC(n)]: rowxfer fails w/overflow error
      (1, 2, 1, 25) - blk_rowxfer(): blk layer: internal BLK-Library error: Failed in conversion routine - condition overflow. col = # row = #.
  • FreeTDS:
    • any other syntax anywhere: rowxfer fails w/syntax error
      (2, 4, 1, 24) - cs_convert: cslib user api layer: common library error: The conversion/operation was stopped due to a syntax error in the source field.
    • excess char otherwise: rowxfer fails w/overflow error
      (2, 4, 1, 36) - cs_convert: cslib user api layer: common library error: The result is truncated because the conversion/operation resulted in overflow.
• b) silently truncates under Sybase (but OK), fails under FreeTDS -- (2, 4, 1, 36) as above
• c) noisily truncates in rowxfer under Sybase (but OK): (1, 2, 1, 42) as above; NB: truncates to even length when starting with binary
  fails under FreeTDS: (2, 4, 1, 36) as above
• d) rowxfer overflow error - (1, 2, 1, 25) under Sybase, (2, 4, 1, 36) under FTDS
• e)
  • Sybase: bind fails (type mismatch), blk_rowxfer OK (!), storing raw content (quite possibly truncated)
    (1, 1, 1, 10) - blk_bind(): blk layer: user error: a table column of type # cannot be bound to a program variable of type #.
  • FreeTDS: blk_rowxfer fails [(2, 4, 1, 36) as above]; (f) if NULL
• f) rowxfer always OK, but Sybase bind fails (type mismatch as above)
• g) OK, but hour padding differs (space with Sybase, 0 with FreeTDS) and FreeTDS prepends Jan 01 1900 for pure (BIG)TIME values and appends 12:00AM for pure DATE values
• h) BIT is always OK, as is (U)TINYINT -> *BINARY(1)
• i) OK, but only Sybase trims trailing zeroes from fractional MONEY(4) values; OTOH, only Sybase formats CS_REAL (float) values with excess precision.
• j) Same as (e) [length 1, not NULL]/(f) for Sybase; under FreeTDS, rowxfer fails with a type mismatch (unless NULL)
  (2, 1, 1, 16) - cs_convert: cslib user api layer: external error: Conversion between # and # datatypes is not supported.

Notably, all errors under Sybase officially come from the CT/BLK layer, whereas FreeTDS only ever issues CS-layer errors that offer less context and may be likelier to slip through the cracks. That should be possible to remedy by letting _cs_convert take an optional pointer to a BCP-error variable and populate it in lieu of reporting an error directly; this pointer's presence would also result in behavior tweaks.

[ucko]

Corrected DBLIB truncation differences (modulo formatting details along the above lines and FreeTDS's support for additional client types):

FROM\TO	*B(1)	*B(64)	*C(1)	*C(64)
*C(10)	a	a	b	OK
*B(10)	b	OK	c	OK
*DT/*$	c	OK	c	OK
#	c/d	OK/e	c/d	OK/e

• a) Both expect hex digits with optional leading 0x (and effectively prepend a 0 to the body if the length is odd), but their behavior when something is off otherwise varies.
  • Sybase: syntax error (20050) if non-hex char among first 2n(+2), otherwise overflow error (20049) as appropriate
  • FreeTDS: syntax error (20050) if non-hex char present at all, otherwise silently truncates
• b) oversized row error (20074) from Sybase, silent truncation by FreeTDS
• c) overflow error (20049) from Sybase, silent truncation by FreeTDS
• d) BIT is always OK, as is TINYINT -> *BINARY(1)
• e) Sybase crashes with heap corruption for DECIMAL and NUMERIC

[ucko]

My latest version of this series adds a commit to address a corner case I recently encountered by (re)allowing BCP into computed columns in the context of a FIRE_TRIGGERS hint, since an INSTEAD OF INSERT trigger may well be in play.

[freddy77]

Picking up a commit to replace one work I was doing, specifically ae519f8.

[freddy77]

Another commit merged!

I'm rebasing the initial merge on master to get an updated version. See https://github.com/freddy77/freetds/tree/pr/558/ncbi-2024-05-bcp-improvements.

Some considerations

1. _cs_convert is getting a lot of different usages and parameters. The name starts to not be appropriate any more;
2. _cs_convert is returning a CS_RETCODE and also a BLK_CONV_STATUS which mostly looks like tds_convert result converted to a new enumeration. Wondering if there should be just a single return;
3. bcp_column_data is kind of weird. It stored TDS representation in the column. So why not usind column_data and other fields instead?
4. bcp_column_data character representation for Unicode data is always TDS raw data, so utf-16. For ctlib and dblib data is usually converted to client charset by libTDS, but not this data. Wondering if this inconsistency should be removed;
5. wondering how proprietary bcp tools export Unicode data. It looks like Sybase writes data as binary (so 'A' -> 4100), not sure about Microsoft;
6. should FreeTDS store format file (freebcp utility) with neutral data types (like UNITEXT for both MS and Sybase) ?

[freddy77]

Are you sure about this ? Isn't this function converting ASCII to upper case but stopping at first lower case character ?

[ucko]

Oops, yeah, I needed to reset n to the length before proceeding here.

[ucko]

Alternatively, the calling code could use strcasestr, but that's not portable, so we'd need to add an implementation to replacements.

[freddy77]

Sure useful for BCP but I suppose it breaks a lot of tests

[ucko]

Don't worry, I checked for regressions and didn't find any.

[mmcnabb-vms]

I found from profiling on VMS that a significant amount of time was spent in strftime for bcp out -c operations (actually more than 50% of the entire runtime), so I think there are noticeable improvements to be had by a hand-tuned date/time format for bcp operations .

The ASE bcp client appears to use locale for bcp out -c date formatting but I don't think that's a requirement we need to follow

[freddy77]

I suppose it depends on how many date/time fields you have in your table. tds_strftime allocates a temporary buffer on the heap, maybe that's the issue?
Or maybe it would be worth to code directly instead of calling strtfime at the end?
Surely not something I care in the close future, there are quite a long list I want to do before.

[ucko]

Another commit merged!

I'm rebasing the initial merge on master to get an updated version.

Thanks for all that and for taking another look in general, and sorry for not getting back to you sooner -- I work for a US federal agency, and the new administration imposed a broad communications ban until recently.

1. _cs_convert is getting a lot of different usages and parameters. The name starts to not be appropriate any more;

Is there some other name you'd prefer? _cs_transform, perhaps?

2. _cs_convert is returning a CS_RETCODE and also a BLK_CONV_STATUS which mostly looks like tds_convert result converted to a new enumeration. Wondering if there should be just a single return;

Fair point. I was also using the presence of a non-NULL bulk-conversion-status pointer as an indication that BCP semantics are in order, but I see that checking for a non-NULL handle should suffice; my latest version of this commit, which I've just force-pushed, adds a comment noting that only _blk_get_col_data is expected to supply one. At any rate, I do see that CS_FAIL always accompanies both BLK_CONV_OVERFLOW and ..._SYNTAX_ERROR and that CS_SUCCEED always accompanies BLK_CONV_TRUNCATION, so I went ahead with a single return value and adjusted _blk_get_col_data to treat ..._TRUNCATION as an alternative successful return value. This approach won't scale as well as what I previously had, but I suppose it doesn't need to.

3. bcp_column_data is kind of weird. It stored TDS representation in the column. So why not usind column_data and other fields instead?

Good question. I was preserving the status quo here, but perhaps some simplification is possible.

4. bcp_column_data character representation for Unicode data is always TDS raw data, so utf-16. For ctlib and dblib data is usually converted to client charset by libTDS, but not this data. Wondering if this inconsistency should be removed;

Wouldn't that be a subtle API break?

5. wondering how proprietary bcp tools export Unicode data. It looks like Sybase writes data as binary (so 'A' -> 4100), not sure about Microsoft;

By my reading of https://learn.microsoft.com/en-us/sql/tools/bcp-utility?view=sql-server-ver16, it defaults to a legacy 8-bit encoding (e.g. CP-1252), but the Windows version can be directed to use UTF-16 instead. I'm not sure we have any installations here I can use for empirical testing. At any rate, I'm specifically concerned with bulk insertion.

6. should FreeTDS store format file (freebcp utility) with neutral data types (like UNITEXT for both MS and Sybase) ?

Perhaps, but that's also out of scope here.

[freddy77]

Back to this series, or at least c3f9ec7 commit. Yes, this type is currently broken with very old protocols.
The condition is kind of complex and weird (destfmt.maxlength != bindcol->column_size && destfmt.datatype == CS_CHAR_TYPE && is_fixed_type(_ct_get_server_type(tds, srctype))) but at the end the behavior is converting the string encoding.
I tried to extend some test to capture the issue and the issue is present but the commit does not fix it, not even including all previous commits.
So I had a look at the code and found these lines (src/tds/data.c, tds_generic_put function):

        /* convert string if needed */
        if (!bcp7 && curcol->char_conv && curcol->char_conv->flags != TDS_ENCODING_MEMCPY && colsize) {
                size_t output_size;
...

What bcp7 flag has to do with conversion is not clear to me. Looking at the history I wrote that code but I cannot understand the meaning beside some weird workaround. The need for bcp7 is due to the fact that the wire encoding of data is different when sending bulk data and when sending bound parameter. This is clear. Why it's used in relation to conversion is less clear. I can imagine that at the time of writing that code the only or main usage of bulk transfer was for dblib and specifically freebcp utility (both ODBC and ctlib usages came much later). Probably to support Unicode columns (like NVARCHAR for MSSQL) code converts directly into this encoding doing bulk.
That makes code not that easy to understand due to the incoherence. char_conv is used to understand how to convert server/client encoding. All libraries beside ODBC convert the the "client encoding" while getting data. I though code also did the opposite, that is converting from client to server (putting data), but this is true if bcp7 is false, that is mainly for Sybase! For Microsoft bcp7 is true and there's no conversion!
Another oddness of the actual code is that use_iconv flag is a bit misleading as this is used only while getting data. Specifically use_iconv is used by ODBC code to support multiple client encoding (ODBC support wide and no wide encoding).
I tried to change the above code to

        /* convert string if needed */
        if (USE_ICONV && curcol->char_conv && curcol->char_conv->flags != TDS_ENCODING_MEMCPY && colsize) {
                size_t output_size;
...

(use_iconv directions coherence) and tests (mostly ODBC ones) complained quite a lot. More or less the assumption that use_iconv affects only getting data is true.
I tried to change the code to

        /* convert string if needed */
        if (curcol->char_conv && curcol->char_conv->flags != TDS_ENCODING_MEMCPY && colsize) {
                size_t output_size;
...

(always conversion coherence) and just some bulk tests is failing.
I was thinking about:

1. rename use_iconv to use_iconv_in to make the direction understandable
2. add a column flag to specify if converting or not.

Those changes should make code more consistent and would allow to easily fix the initial conversion issue.
One of the missing tests, I think, that prevented these mistake to be visible is testing client bound column insertion on unicode columns (like src/odbc/unittests/bcp.c, src/dblib/unittests/bcp.c and src/dblib/unittests/bcp2.c).

[mmcnabb-vms]

@freddy77 I have found that all of the other commits from this PR can be applied in order , with a few trivial conflict resolutions, and the tests all pass against both MS (tdsver 7.4) and Sybase. See mm_pr558 for reference. I haven't done much testing outside of the FreeTDS test suite though (e.g. the trigger application and blob input).

This was just before your cherry-pick of "BCP into computed columns again..."

Are you planning to pick all of these (just so I can rebase my #685 changes to the right place)

[freddy77]

@mmcnabb-vms added a commit to replace entirely the "ctlib: Optimize blob conversion for bulk transfers in." commit in this series. _cs_convert was getting too messy and complicated. That's introduce quite some conflicts.

The series is surely nice but it lacks entirely tests. Not clear if it's doing what is supposed to do or not. For instance the "Fix DATE and DATETIME2 bulk insertion via TDS 7.0 and 7.1." is not fixing the issue.

[freddy77]

Slowly getting this in. More than merging is a kind of rewriting. Now the behavior of char -> binary in ctlib is correct. Added tests for this.