Most BCP improvements from NCBI, as of May 2024 #558

ucko · 2024-05-30T14:35:20Z

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 · 2024-06-04T14:43:09Z

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

ucko · 2024-06-06T21:28:33Z

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 · 2024-06-12T16:02:58Z

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 · 2024-06-12T16:04:47Z

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

ucko · 2024-06-18T20:03:46Z

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 · 2024-06-20T19:58:47Z

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 · 2024-07-01T21:19:16Z

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 · 2024-08-28T17:46:49Z

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 · 2025-01-22T11:58:24Z

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

freddy77 · 2025-01-28T15:32:10Z

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

_cs_convert is getting a lot of different usages and parameters. The name starts to not be appropriate any more;
_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;
bcp_column_data is kind of weird. It stored TDS representation in the column. So why not usind column_data and other fields instead?
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;
wondering how proprietary bcp tools export Unicode data. It looks like Sybase writes data as binary (so 'A' -> 4100), not sure about Microsoft;
should FreeTDS store format file (freebcp utility) with neutral data types (like UNITEXT for both MS and Sybase) ?

freddy77 · 2025-01-28T15:35:23Z

src/tds/bulk.c

+	if (s[n] == '\0') {
+		return s; /* already all uppercase */
+	} else {
+		char * result = malloc(n + 1);


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

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

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

freddy77 · 2025-01-28T15:39:12Z

src/tds/config.c

-const char STD_DATETIME_FMT[] = "%b %e %Y %I:%M%p";
+const char STD_DATETIME_FMT[] = "%Y-%m-%d %H:%M:%S.%z";


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

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

* Read defaults, send them back explicitly as needed, and clean up their storage afterwards. * tds5_bcp_add_variable_columns: ** Send spaces in lieu of fully empty SYB(VAR)CHAR data. ** Send correct textpos values in all cases. ** When writing the adjustment table, avoid a potential Sybase "row format error" by eliding the (effective) column count when it would have repeated the largest adjustment table entry. * tds_deinit_bcpinfo: Free and reset current_row for transfers in. Avoid potentially trying to interpret fragments of outgoing TDS data as BLOB pointers. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

Under these protocol versions, MS SQL Server describes these columns as having NVARCHAR(n) types rather than traditional DATETIME types, due to DATETIME's range limitations. (Likewise for TIME, which however still needs to come in as a preconverted UTF-16 string to compensate for combined API and protocol limitations.) - ctlib/blk.c: Perform character conversion on strings converted from fixed-width types such as CS_DATETIME_TYPE when a comparison of column-size limits appears to indicate an encoding difference. - tds/config.c: Adjust STD_DATETIME_FMT to match MS's (ISO-ish) tastes; in particular, ensure that the entire date portion fits within the first 10 characters. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

* Extend _cs_convert to take an additional handle argument (allowed to be NULL) to provide for the possibility of replacing the caller's original output buffer with the one tdsconvert allocated itself (as it always does for blobs), rather than copying the blob data. * blk.c (_blk_get_col_data): Provide a non-NULL handle, both for efficiency's sake and because tds_alloc_bcp_column_data imposes a 4 KiB cap with the expectation that callers will take this approach. (dblib already does.) Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

To that end, factor a static _blk_clean_desc function out of blk_done's CS_BLK_ALL case. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

* tds.h (struct tds_bcpinfo): Add more fields to enable the necessary bookkeeping. * ctlib/blk.c: Implement blk_textfer; adjust other functions accordingly. * tds/{bulk,data}.c: Allow for deferred, and possibly piecemeal, blob submission; in particular, make tds_bcp_send_record resumable. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

Adjust existing failure modes' boundaries and introduce the possibility of proceeding with truncated input with or without a message (with the latter occurring in some cases that require no conversion). As for messages, arrange to emit them via _ctclient_msg rather than _csclient_msg with the same (English) wording and context as Sybase yields, modulo formal punctuation differences and the usual discrepancies in layer and origin numbering. To that end: * Add internal BLK_CONV_* constants that serve both as (new) client message numbers and as possible return values for _cs_convert. * Cover those numbers in _ct_get_user_api_layer_error. * Have _cs_convert take the presence of a non-NULL handle (which only _blk_get_col_data supplies) to indicate that the caller wants bulk-insertion semantics, including in particular the possible use of those return values. * Keep a running count of rows sent for reporting purposes (determining the column number to cite by a linear scan of the bindinfo's columns array). Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

The destination view may well have an INSTEAD OF INSERT trigger that makes meaningful use of the supplied values. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

ucko · 2025-02-14T20:35:52Z

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.

_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?

_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.

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.

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?

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.

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.

This was referenced May 30, 2024

NCBI changes as of May 2024 #555

Closed

tds_generic_put: Correct BCP 7.x new blob type handling #570

Closed

ucko force-pushed the ncbi-2024-05-bcp-improvements branch from e6facc4 to 7b71bdb Compare June 4, 2024 14:41

ucko mentioned this pull request Jun 18, 2024

cs_convert, dbconvert: Truncate data as needed. #573

Closed

ucko force-pushed the ncbi-2024-05-bcp-improvements branch from 7b71bdb to 468812f Compare July 3, 2024 19:27

ucko force-pushed the ncbi-2024-05-bcp-improvements branch from 468812f to 8739305 Compare August 28, 2024 17:42

freddy77 reviewed Jan 28, 2025

View reviewed changes

ucko added 9 commits February 12, 2025 17:59

blk.c: Handle CS_BLK_CANCEL in blk_done.

3a2509c

To that end, factor a static _blk_clean_desc function out of blk_done's CS_BLK_ALL case. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

_blk_get_col_data: Bail (successfully) on null columns.

af06c45

Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

BCP into computed columns again when allowing triggers.

3f51545

The destination view may well have an INSTEAD OF INSERT trigger that makes meaningful use of the supplied values. Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

Declare and implement blk_sethints.

1c45955

Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>

ucko force-pushed the ncbi-2024-05-bcp-improvements branch from 8739305 to 1c45955 Compare February 14, 2025 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Most BCP improvements from NCBI, as of May 2024 #558

Most BCP improvements from NCBI, as of May 2024 #558

ucko commented May 30, 2024

ucko commented Jun 4, 2024

ucko commented Jun 6, 2024 •

edited

Loading

ucko commented Jun 12, 2024 •

edited

Loading

ucko commented Jun 12, 2024

ucko commented Jun 18, 2024 •

edited

Loading

ucko commented Jun 20, 2024 •

edited

Loading

ucko commented Jul 1, 2024

ucko commented Aug 28, 2024

freddy77 commented Jan 22, 2025

freddy77 commented Jan 28, 2025

freddy77 Jan 28, 2025

ucko Feb 14, 2025 •

edited

Loading

ucko Feb 14, 2025

freddy77 Jan 28, 2025

ucko Feb 14, 2025

ucko commented Feb 14, 2025

		const char STD_DATETIME_FMT[] = "%b %e %Y %I:%M%p";
		const char STD_DATETIME_FMT[] = "%Y-%m-%d %H:%M:%S.%z";

Most BCP improvements from NCBI, as of May 2024 #558

Are you sure you want to change the base?

Most BCP improvements from NCBI, as of May 2024 #558

Conversation

ucko commented May 30, 2024

ucko commented Jun 4, 2024

ucko commented Jun 6, 2024 • edited Loading

ucko commented Jun 12, 2024 • edited Loading

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

DBLIB BCP truncation differences (also corrected below):

ucko commented Jun 12, 2024

ucko commented Jun 18, 2024 • edited Loading

ucko commented Jun 20, 2024 • edited Loading

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

General notes

ucko commented Jul 1, 2024

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

ucko commented Aug 28, 2024

freddy77 commented Jan 22, 2025

freddy77 commented Jan 28, 2025

freddy77 Jan 28, 2025

Choose a reason for hiding this comment

ucko Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

ucko Feb 14, 2025

Choose a reason for hiding this comment

freddy77 Jan 28, 2025

Choose a reason for hiding this comment

ucko Feb 14, 2025

Choose a reason for hiding this comment

ucko commented Feb 14, 2025

ucko commented Jun 6, 2024 •

edited

Loading

ucko commented Jun 12, 2024 •

edited

Loading

ucko commented Jun 18, 2024 •

edited

Loading

ucko commented Jun 20, 2024 •

edited

Loading

ucko Feb 14, 2025 •

edited

Loading