pdo_odbc: Don't fetch 256 byte blocks for long columns #10809

NattyNarwhal · 2023-03-08T19:39:00Z

Fetching 256 byte blocks can confuse some drivers with conversion routines. That, and it seems to me the round trips to and from a database could be a major performance impact.

Instead, we try to fetch all at once, and continue fetching if a driver somehow has more for us.

This has been tested with a problematic case with the Db2i driver with stateful MBCS encodings.

See GH-10733 for discussion about this and issues it can resolve.

NattyNarwhal · 2023-03-08T19:42:27Z

I'm marking this as draft because it is a different approach, though one that might be less troublesome. The reason why PDO_ODBC fetched 256 byte blocks seems a little unclear, so I went digging and found 43c1a1b, which references #33533 - that does seem to imply this could create problems instead for drivers that instead declare something like 2 GB available, though that seem like it'd be a problem regardless?

NattyNarwhal · 2023-03-08T19:43:40Z

The other angle is why isn't it just binding all the time; procedural ODBC seems to do this, or at least a lot less conservatively.

NattyNarwhal · 2023-03-17T18:51:45Z

FWIW a user reported to me that they tried this PR and it resolved the original issue for them.

SakiTakamachi · 2023-10-15T11:51:44Z

I was looking around on Sunday night to see if there were any issues that I might be able to help with, and I found this PR.

Is it correct to understand that the fact that it has not been merged yet means that it is unresolved?

If you need a test run or review, I'm ready to help.

NattyNarwhal · 2023-10-16T15:27:42Z

When I and a client tested it, it did seem to resolve the issue (see issue linked in description), but I'm a bit worried about edge cases and how well this behaves with other drivers outside of Db2i.

SakiTakamachi · 2023-10-19T15:48:31Z

First, I tried it with SQL Server.

Test code

<?php
$dsn = 'Driver=FreeTDS;Server=sql-server;Port=1433;Database=test;uid=SA;pwd={p@ssw0rd};'; // my env
$user = 'SA';
$password = 'p@ssw0rd';

$attrOpions = [
    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
    PDO::ATTR_PERSISTENT => false,
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
];
$db = new PDO("odbc:$dsn", $user, $password, $attrOpions);

$db->exec("CREATE TABLE TEST (TEST_VAL NVARCHAR(2000))");

$db->exec(<<<SQL
INSERT INTO TEST VALUES (N'こんにちは')
SQL);

$db->exec(<<<SQL
INSERT INTO TEST VALUES (N'☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘ABCDEFGHIJKLMOPR                                                                                                                            ')
SQL);

$stmt = $db->prepare("SELECT * FROM TEST");
$stmt->execute();
while (($row = $stmt->fetch())) {
    var_dump($row);
}

$db->exec("DROP TABLE TEST");

Results

SQL Server + FreeTDS

master branch

array(1) {
  ["TEST_VAL"]=>
  string(15) "こんにちは"
}
array(1) {
  ["TEST_VAL"]=>
  string(1463) "☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘ABCDEFGHIJKLMOPR                                                                                                                            "
}

Natty's branch

array(1) {
  ["TEST_VAL"]=>
  string(15) "こんにちは"
}

Fatal error: Possible integer overflow in memory allocation (0 + 32) in <path to file> on line 25

SakiTakamachi · 2023-10-19T15:57:10Z

FYI

This test code works fine on either branch:

<?php
$dsn = 'Driver=FreeTDS;Server=sql-server;Port=1433;Database=test;uid=SA;pwd={p@ssw0rd};'; // my env
$user = 'SA';
$password = 'p@ssw0rd';

$db = odbc_connect("$dsn", $user, $password);
odbc_exec($db, "CREATE TABLE TEST (TEST_VAL NVARCHAR(2000))");

odbc_exec($db, <<<SQL
INSERT INTO TEST VALUES (N'こんにちは')
SQL);

odbc_exec($db, <<<SQL
INSERT INTO TEST VALUES (N'☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘☆株★■▲┐┘ABCDEFGHIJKLMOPR                                                                                                                            ')
SQL);

$stmt = odbc_prepare($db, "SELECT * FROM TEST");
odbc_execute($stmt);
while (($row = odbc_fetch_array($stmt))) {
    var_dump($row);
}

odbc_exec($db, "DROP TABLE TEST");

NattyNarwhal · 2023-10-19T16:11:51Z

I don't expect the issue this patch was written to affect MSSQL (it was a driver bug w/ character conversion that PHP behaviour exacerbated), but the regression there is worrying. Do you know which allocation failed, and how? I'm suspecting it's the string realloc, with somehow a zero length being returned?

SakiTakamachi · 2023-10-19T16:27:34Z

673                     if (rc == SQL_SUCCESS_WITH_INFO || rc == SQL_SUCCESS) {
(gdb) 
688                             SQLLEN to_fetch_len = orig_fetched_len + 1; /* for 0 at end */
(gdb) 
689                             char *buf2 = emalloc(to_fetch_len);
(gdb) p to_fetch_len
$1 = -3
(gdb) p orig_fetched_len
$2 = -4
(gdb) n

Fatal error: Possible integer overflow in memory allocation (0 + 32) in <path to file> on line 25
0x00005555559fb071 in php_execute_script (primary_file=0x7fffffffd960) at /php-src/main/main.c:2432
2432            zend_try {
(gdb)

It looks like passing a negative value to L689 emalloc, causing an underflow. C->fetched_len looks like something is broken.

(edit)

For "こんにちは", the value of orig_fetched_len is 15.
It might be a bug in FreeTDS, so I'm thinking of trying it out a little more.

NattyNarwhal · 2023-10-19T16:37:42Z

I'm guessing a negative value is one of those sentinel values that ODBC allows it to return return:

calvin@anika php-src % grep -- -4 /opt/local/include/sql*
/opt/local/include/sqlext.h:#define SQL_IS_POINTER							(-4)
/opt/local/include/sqlext.h:#define SQL_LONGVARBINARY                       (-4)
/opt/local/include/sqlext.h:#define SQL_NO_TOTAL                    (-4)
calvin@anika php-src % grep -- -3 /opt/local/include/sql*
/opt/local/include/sql.h:#define SQL_NTS                   (-3)
/opt/local/include/sql.h:#define SQL_NTSL                  (-3L)
/opt/local/include/sqlext.h:#define SQL_VARBINARY                           (-3)
/opt/local/include/sqlext.h:#define SQL_SCROLL_STATIC               (-3L) /*-SQL_CURSOR_STATIC */
/opt/local/include/sqlite3.h:#define SQLITE_UTF8           1    /* IMP: R-37514-35566 */
/opt/local/include/sqlite3.h:#define SQLITE_UTF16LE        2    /* IMP: R-03371-37637 */
/opt/local/include/sqlite3.h:#define SQLITE_UTF16BE        3    /* IMP: R-51971-34154 */

SQL_NTS is possible if it's null-terminated. Usually, that gets set as a length that the API user passes, but maybe drivers can return it? (if it does, then tbh, handling it means that we're given a free null terminated string and can think less).

NattyNarwhal · 2023-10-19T17:22:23Z

Per MS:

StrLen_or_IndPtr
[Output] Pointer to the buffer in which to return the length or indicator value. If this is a null pointer, no length or indicator value is returned. This returns an error when the data being fetched is NULL.
SQLGetData can return the following values in the length/indicator buffer:
The length of the data available to return
SQL_NO_TOTAL
SQL_NULL_DATA

Probably SQL_NO_TOTAL then. IME drivers usually return this if they don't want to estimate how long a string is after conversion. The Db2i driver has an option to force it to calculate instead to make PHP's job easier, but this is something not all drivers would support, so we'll have to figure out a way to deal with it, even if it's just giving up and throwing.

SakiTakamachi · 2023-10-19T17:30:54Z

I was just reading the same page.
How about calculating the length on the php side when a sentinel value is returned, that is, when a negative value is returned?

(gdb) p (*((pdo_odbc_column *) 0x7ffff4676460))->data
$21 = 0x7ffff4688140 "\342\230\206\346\240\252\342\230\205\342\226\240\342\226\262\342\224\220\342\224\230\342\230\206\346\240\252\342\230\205\342\226\240\342\226\262\342\224\220\342\224\230\342\230\206\346\240\252\342\230\205\342\226\240\342\226\262\342\224\220\342\224\230\342\230\206\346\240\252\342\230\205\342\226\240\342\226\262\342\224\220\342\224\230\342\230\206\346\240\252\342\230\205\342\226\240\342\226\262\342\224\220\342\224\230\342\230\206\346\240\252\342\230\205\342\226\240\342\226\262\342\224\220\342\224\230\342\230\206\346\240\252\342\230\205\342\226\240\342\226\262\342\224\220\342\224\230\342\230\206\346\240\252\342\230\205\342\226\240\342\226\262\342\224\220\342\224\230\342\230\206\346\240\252\342\230\205\342\226\240\342\226\262\342\224\220\342\224\230\342\230\206\346\240\252\342\230\205\342\226"...

It seems like all the data is already included, so I feel like I can count the number of bytes.

NattyNarwhal · 2023-10-19T17:35:11Z

Yeah, I think it would be possible to do an estimation on our side. Keep in mind we can just try again with whatever buffer size we like, and we can just stitch it together at the end, because the final call will always return the right length:

Places the length of the data in *StrLen_or_IndPtr. If StrLen_or_IndPtr was a null pointer, SQLGetData does not return the length.
For character or binary data, this is the length of the data after conversion and before truncation due to BufferLength. If the driver cannot determine the length of the data after conversion, as is sometimes the case with long data, it returns SQL_SUCCESS_WITH_INFO and sets the length to SQL_NO_TOTAL. (The last call to SQLGetData must always return the length of the data, not zero or SQL_NO_TOTAL.) If data was truncated due to the SQL_ATTR_MAX_LENGTH statement attribute, the value of this attribute - as opposed to the actual length - is placed in *StrLen_or_IndPtr. This is because this attribute is designed to truncate data on the server before conversion, so the driver has no way of figuring out what the actual length is. When SQLGetData is called multiple times in succession for the same column, this is the length of the data available at the start of the current call; that is, the length decreases with each subsequent call.
For all other data types, this is the length of the data after conversion; that is, it is the size of the type to which the data was converted.

SakiTakamachi · 2023-10-19T17:35:32Z

No, my plan is no good because it only takes 256 bytes.

Counting length in php looks good. However, I overlooked the fact that not all strings could be retrieved the first time.

NattyNarwhal · 2023-10-19T17:37:14Z

Maybe if we get NO_TOTAL, we can fall back to using a fixed size, i.e. the 256 from before?

SakiTakamachi · 2023-10-19T17:40:24Z

Yes, I think it's possible. Indeed, SQL Server did not seem to have the same problems as db2 for i, so it seems that there will be no problem with fallback.

SakiTakamachi · 2023-10-19T18:00:35Z

But maybe we should wait a little. The procedural version of odbc does not have any problems under the same conditions. I'd like to take a closer look at how they are implemented.

NattyNarwhal · 2023-10-19T18:42:12Z

But maybe we should wait a little. The procedural version of odbc does not have any problems under the same conditions. I'd like to take a closer look at how they are implemented.

Both bound fetches and direct results (w/ odbc_result) are stringy like PDO_ODBC is, and direct fetches have similar-ish logic. The loop there is pretty similar; though they do handle the NULL_DATA and NO_TOTAL cases, but errors out on the latter (which is better than crashing, at least).

SakiTakamachi · 2023-10-20T14:46:36Z

@NattyNarwhal
This should probably work as expected. #12487
However, I feel that S->cols[colno].data = emalloc(colsize + 1); is a waste of memory, so I'm wondering if there is a better way.

ext odbc seems to reserve memory for the maximum value of the column as same as this.

SakiTakamachi · 2023-10-20T15:33:31Z

take3
NattyNarwhal#1

When C->fetched_len == SQL_NO_TOTAL, I tried to make it work as before.

SakiTakamachi · 2023-10-20T15:39:25Z

Take2 has failed the test. I think take3 is smarter.

SakiTakamachi · 2023-10-20T16:47:36Z

By the way, no matter how I try, I cannot insert multibyte characters into db2 of PUB400.COM from php.
Can you insert from php? If you don't mind, could you give me an example of an insert statement and the column type?

NattyNarwhal · 2023-10-20T20:18:01Z

I agree take3 looks better; erroring on NO_TOTAL is annoying for any conversion cases (so, pretty much any case with drivers converting encodings from DB to client representation).

By the way, no matter how I try, I cannot insert multibyte characters into db2 of PUB400.COM from php.
Can you insert from php? If you don't mind, could you give me an example of an insert statement and the column type?

How are you inserting/what's your table specification? You might want to specify the CCSID (encoding) of the column, to force it into a. I often annotate CHAR(n) CCSID 1208 to force UTF-8 storage. I believe for EBCDIC DBCS Japanese, 5035 is a good choice (there are a ton of CCSIDs for Japanese, many awful SBCS ones - IBM isn't clear with this), and it'll make the original issue happen, because the driver will convert to and from UTF-8 (and return NO_TOTAL unless you set that AlwaysCalculateResultLength option)

NattyNarwhal · 2023-10-20T21:42:22Z

Did take the time to test - NO_TOTAL cases are handled so that it doesn't fail. (In the case of the original bug, NO_TOTAL triggers it because the 256-byte buffer fallback isn't enough, but it means for most cases, the driver option to suppress NO_TOTAL is unneeded.)

Another option is instead of using 256 as the fixed size, get the size of the column instead for the initial/no total cases. I bet in most cases, there's a good chance the described column length is sufficient for a single round.

While you could call SQLGetColAttribute or SQLDescribeCol beforehand, I think the describer is called beforehand, so you can just do:

diff --git a/ext/pdo_odbc/odbc_stmt.c b/ext/pdo_odbc/odbc_stmt.c
index e2bdc95ef3..3e491ebc16 100644
--- a/ext/pdo_odbc/odbc_stmt.c
+++ b/ext/pdo_odbc/odbc_stmt.c
@@ -651,6 +651,8 @@ static int odbc_stmt_get_col(pdo_stmt_t *stmt, int colno, zval *result, enum pdo
 	pdo_odbc_stmt *S = (pdo_odbc_stmt*)stmt->driver_data;
 	pdo_odbc_column *C = &S->cols[colno];
 
+fprintf(stderr, "col #%d (%s) size??? %zd\n", colno, ZSTR_VAL(stmt->columns[colno].name), stmt->columns[colno].maxlen);
+
 	/* if it is a column containing "long" data, perform late binding now */
 	if (C->is_long) {
 		SQLLEN orig_fetched_len = SQL_NULL_DATA;

and it'll output:

col #0 (CODE) size??? 7
col #1 (KANA) size??? 180
col #2 (HANKAN) size??? 180
col #3 (NAME) size??? 180
col #4 (VENDC) size??? 7
col #5 (VENDN) size??? 180
col #6 (KIND) size??? 60
col #7 (PACK) size??? 96
col #8 (PRICE) size??? 11
col #9 (SQTY) size??? 11
col #10 (PQTY) size??? 11
col #11 (LQTY) size??? 11
col #12 (DESC) size??? 3072
col #13 (WH) size??? 30
col #14 (INVDT) size??? 10
col #15 (INVTM) size??? 8
col #16 (DEL) size??? 3

Changing it to not use an initial 256-byte buffer and using a dynamic one instead though requires more thought though:

the describer is the one that allocates the 256-byte buffer, it and get_col need to adapt if the initial buffer is larger (you could then perhaps get rid of the whole "long column" thing at all, simplify the logic)
any columns like hugebinary(2GB) and similar are definitely not fitting in on a 32-bit platform
larger buffers w/ huge column sizes/number of columns could become problematic, blow out the PHP memory limit

It might be worth bumping the fallback buffer size to a larger one - something in the kilobytes or megabytes.

SakiTakamachi · 2023-10-21T00:19:55Z

How are you inserting/what's your table specification? You might want to specify the CCSID (encoding) of the column, to force it into a. I often annotate CHAR(n) CCSID 1208 to force UTF-8 storage. I believe for EBCDIC DBCS Japanese, 5035 is a good choice (there are a ton of CCSIDs for Japanese, many awful SBCS ones - IBM isn't clear with this), and it'll make the original issue happen, because the driver will convert to and from UTF-8 (and return NO_TOTAL unless you set that AlwaysCalculateResultLength option)

Thank you. I simply inserted to var_char(2000). All multi-byte characters are deleted and inserted. I'll try your advice.

Another option is instead of using 256 as the fixed size, get the size of the column instead for the initial/no total cases. I bet in most cases, there's a good chance the described column length is sufficient for a single round.

I see. Am I correct in understanding that in most use cases, if the block size is larger, it can be fetched in one round? That makes sense.

Btw, I was thinking of another method. In the first place, I was wondering why db2 characters become garbled when separated by 256 bytes, and I wondered where on earth they were encoded.

This is completely a random idea and I haven't tried it yet, but I thought that using zend_string_realloc to expand memory might be the cause, and I thought what would happen if I replaced it with realloc in C language.
(So when I tried to test it with db2, I got stuck on insert.)

aszenz · 2024-02-14T14:59:28Z

@aszenz

What database are you using?

I'm using duckdb-odbc, see https://duckdb.org/docs/installation/?version=latest&environment=odbc&installer=binary&platform=linux

NattyNarwhal · 2024-02-14T16:15:47Z

I'm not sure if you'd be encountering the same issue I had that drove me to write the PR. That said, if you could try the PR and see if it works for you (it should rebase onto current master fine), that'd be helpful. If it solves your issue, I'd have some confidence this is the right approach in general.

SakiTakamachi · 2024-02-15T15:39:21Z

I installed duckdb-odbc and debugged it. That is very close to this PR problem, but slightly different.

(This is probably something only Natty would know. ↓)

First, like db2, orig_fetched_len does not contain the total number of characters. However, unlike db2, it does not return NO_TOTAL. It contains 255 which was a split fetch.

pdo_odbc retrieves additional data if rc is SQL_SUCCESS_WITH_INFO.

Suppose we repeat the loop and pdo_odbc finishes fetching all the data. However, pdo_odbc loops one last time, checks that there is no more data, and breaks the loop.

This last loop conflicts with the duckdb specification. If duckdb ran out of data to fetch and then fetched it again, surprisingly it would start over again.

This means that the fetch loop starts again and never ends.

I think there is a problem with duckdb's SQLGetData behavior, but some workaround may be possible on the php side.
The idea is that if rc==SQL_SUCCESS in the loop, it will break at that point instead of looping again at the end.

@NattyNarwhal
I would love to hear your opinion.

SakiTakamachi · 2024-02-15T16:10:30Z

@NattyNarwhal

By the way, unrelated to the above topic, I suddenly thought of something, how about making it possible to set LONG_COLUMN_BUFFER_SIZE as an attribute value?

If you specify -1, all will be acquired at once.

It may be a good option for users who only handle small amounts of data that do not require buffers.

NattyNarwhal · 2024-02-15T17:35:04Z

Your explanation as to why is fails makes sense; DuckDB shouldn't be returning more data if we hit theoretical end of buffer and try to read more. It's been a while since I looked at this, but maybe we need to explicitly break the loop when we get less data. I think the logic as to why it did this before was it'd hit the else block when it got SQL_NO_DATA for another attempt (which might help if the driver was misleading about there actually being stuff at the end for encoding conversion cases?), but if this driver doesn't do that...

The other proposal I'm a bit mixed on. I see the purpose, but it's another knob to have to worry about as a user (what does it do, does changing it help me, if I should change it, shouldn't it be at a better default?), and PHP tends to shy away from that recently (i.e. less emphasis on INI options, it makes testing more complex). However, if we can't find a reasonable default (be it fixed buffer or based on column size, whatever), there might not be a better option.

SakiTakamachi · 2024-02-15T21:21:45Z

Thanks for checking.

I think the logic as to why it did this before was it'd hit the else block when it got SQL_NO_DATA for another attempt (which might help if the driver was misleading about there actually being stuff at the end for encoding conversion cases?), but if this driver doesn't do that...

There is such a case... odbc bugs are really troublesome. If that's the case, maybe duck db should fix this problem.

I mean PDO::setAttribute like PDO::MYSQL_ATTR_MAX_BUFFER_SIZE.

NattyNarwhal · 2024-02-15T21:46:28Z

Yeah, if DuckDB doesn't fix this upstream (and we point users to update their driver if they report this), we'll have to work around this.

Point, I didn't know pdo_mysql did this. If there's precedent, it's probably fine to do this (and maybe borrow the buffer size they use by default?).

SakiTakamachi · 2024-02-15T23:34:40Z

Agree, I think it would be a good idea to first hope that the bug will be fixed upstream, and if it isn't fixed, then think of a workaround again.

Thanks, borrowing the default value is a good idea.

In the case of MySQL, a different attribute value (PDO::MYSQL_ATTR_USE_BUFFERED_QUERY) is used to turn the buffer mode on and off, but I don't want the number of attributes to increase unnecessarily, so I think it's better to turn it off with -1. How do you think?

NattyNarwhal · 2024-02-16T06:57:58Z

That makes sense to me; sentinel value doesn't matter per se. Figuring out an ideal default is the trickiest part, but it sounds like pdo_mysql's ~1 MB default might be reasonable since it does something similar. My guess is most columns are smaller than that, and the ones larger than that are probably going to be things like clob(4GB) that wouldn't fit into the memory limit (or brush up against other limits).

SakiTakamachi · 2024-02-16T14:37:33Z

Yeah, your view is probably correct. Do you have time to add these changes? It took some time, but I think we were able to find specifications that we were fully satisfied with.

aszenz · 2024-02-16T14:50:37Z

Yeah, if DuckDB doesn't fix this upstream (and we point users to update their driver if they report this), we'll have to work around this.

DuckDB folks are normally very eager to fix valid issues, I would recommend filing an issue at:

https://github.com/duckdb/duckdb/tree/main/tools/odbc

SakiTakamachi · 2024-02-17T07:21:31Z

I've got it. I'll report it later.

SakiTakamachi · 2024-02-17T07:25:22Z

@NattyNarwhal
Sorry, I had misunderstood how PDO::MYSQL_ATTR_MAX_BUFFER_SIZE worked. This is quite different from what I suggested.

I think it's still a good idea to borrow the default value, but it may not be necessary to allow the buffer size to be set by the user.

cmb69 · 2024-10-16T11:51:28Z

I'm afraid there is no general solution. If drivers don't properly report StrLen_or_IndPtr, the attempt from bccca0b can't work. But we cannot even use strlen() or some other attempt to detect what the driver had delivered, because we cannot make assumptions about the character encoding (general PHP issue). And we cannot even request character data as binary data, since in this case the driver likely ignores any configured character encoding conversions.

Increasing the buffer size appears to be a reasonable mitigation, and I don't really see the reason to even have a separate buffer. The relevant part of the zend_string could be used as buffer directly, what would save the additional memcpy(). Still, that doesn't really help if we need to call SQLGetData() more than once for a column …

cmb69 · 2024-11-25T11:15:36Z

I think we should pursue this, since it might actually improve performance, and at least would mitigate the issues with drivers not properly counting the available data length.

I would suggest to have a buffer size that just fits into a ZendMM page (4KiBi); if we're storing in a zend_str, that would be ZEND_MM_PAGE_SIZE- ZSTR_MAX_OVERHEAD, I think.

NattyNarwhal · 2024-11-25T14:46:51Z

Do you think a variable buffer size would be better? We were experimenting with that approach last time me and Saki touched the PR. I agree that the current size of 256 is too small, a bigger size alone would probably work around a lot of driver issues, so it's worth updating that initial buffer size at least. (There are other criteria like i.e. MTUs and DB protocol overhead over the network, but that seems really hard to predict when localhost is a very common case.)

SakiTakamachi · 2025-04-18T06:23:29Z

Well, I'm still interested in this too.

NattyNarwhal · 2025-06-10T17:59:17Z

@SakiTakamachi How do you feel about merging this for PHP 8.5? I'm still a little hesitant as previously mentioned, but it does seem to resolve issues (i.e. the one I originally diagnosed this for w/ DBCS, and fleadram's here), but those experiences are specific to Db2i (which I'm more familiar with).

SakiTakamachi · 2025-06-18T13:57:43Z

@NattyNarwhal

Since there is a mention of the buffer size, I personally think we can merge it into 8.5 once that part is fixed.

Fetching 256 byte blocks can confuse some drivers with conversion routines. That, and it seems to me the round trips to and from a database could be a major performance impact. Instead, we try to fetch all at once, and continue fetching if a driver somehow has more for us. This has been tested with a problematic case with the Db2i driver with stateful MBCS encodings. See phpGH-10733 for discussion about this and issues it can resolve.

change to separate by 256 bytes, when C->fetched_len == SQL_NO_TOTAL changed from 256 byte to 2048 byte buf block.

Could be configurable maybe, but best to avoid magic numbers even for a compile-time constant.

Change recommended by Christoph. Probably a little better performance wise I have to guess.

NattyNarwhal marked this pull request as draft March 8, 2023 19:39

github-actions bot added the Extension: pdo_odbc label Mar 8, 2023

NattyNarwhal marked this pull request as ready for review March 27, 2023 16:05

SakiTakamachi mentioned this pull request Oct 20, 2023

[pdo_odbc] Data is no longer separated by 256 bytes. #12487

Closed

SakiTakamachi mentioned this pull request Oct 20, 2023

change to separate by 256 bytes, when C->fetched_len == SQL_NO_TOTAL NattyNarwhal/php-src#1

Merged

NattyNarwhal requested a review from SakiTakamachi as a code owner April 15, 2024 15:43

cmb69 mentioned this pull request Jul 19, 2024

pdo_odbc: allocate sufficient space for retrieving unicode data #15008

Open

NattyNarwhal mentioned this pull request Sep 24, 2024

PDO_MySQL not properly quoting PDO_PARAM_LOB binary data #15949

Closed

NattyNarwhal mentioned this pull request Oct 16, 2024

PDO_ODBC can inject garbage into field values #16450

Closed

cmb69 mentioned this pull request Nov 25, 2024

PDO inserts NULL byte at position 254 on SQL Server ntext column #16901

Open

fleadram mentioned this pull request Apr 17, 2025

PDO_ODBC truncating CHAR field Db2i driver #18349

Open

NattyNarwhal and others added 4 commits June 18, 2025 12:51

change to separate by 256 bytes, when C->fetched_len == SQL_NO_TOTAL

c2ec656

change to separate by 256 bytes, when C->fetched_len == SQL_NO_TOTAL changed from 256 byte to 2048 byte buf block.

Make long column buffer size single define

d45858c

Could be configurable maybe, but best to avoid magic numbers even for a compile-time constant.

Use ZendMM page size minus zend_string overhead

5dc5975

Change recommended by Christoph. Probably a little better performance wise I have to guess.

NattyNarwhal force-pushed the dont-fetch-256-byte-blocks branch from a97752e to 5dc5975 Compare June 18, 2025 15:58

pdo_odbc: Don't fetch 256 byte blocks for long columns #10809

Are you sure you want to change the base?

pdo_odbc: Don't fetch 256 byte blocks for long columns #10809

Conversation

NattyNarwhal commented Mar 8, 2023

Uh oh!

NattyNarwhal commented Mar 8, 2023

Uh oh!

NattyNarwhal commented Mar 8, 2023

Uh oh!

NattyNarwhal commented Mar 17, 2023

Uh oh!

SakiTakamachi commented Oct 15, 2023

Uh oh!

NattyNarwhal commented Oct 16, 2023

Uh oh!

SakiTakamachi commented Oct 19, 2023

Test code

Results

SQL Server + FreeTDS

master branch

Natty's branch

Uh oh!

SakiTakamachi commented Oct 19, 2023

Uh oh!

NattyNarwhal commented Oct 19, 2023

Uh oh!

SakiTakamachi commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NattyNarwhal commented Oct 19, 2023

Uh oh!

NattyNarwhal commented Oct 19, 2023

Uh oh!

SakiTakamachi commented Oct 19, 2023

Uh oh!

NattyNarwhal commented Oct 19, 2023

Uh oh!

SakiTakamachi commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NattyNarwhal commented Oct 19, 2023

Uh oh!

SakiTakamachi commented Oct 19, 2023

Uh oh!

SakiTakamachi commented Oct 19, 2023

Uh oh!

NattyNarwhal commented Oct 19, 2023

Uh oh!

SakiTakamachi commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Oct 20, 2023

Uh oh!

SakiTakamachi commented Oct 20, 2023

Uh oh!

SakiTakamachi commented Oct 20, 2023

Uh oh!

NattyNarwhal commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NattyNarwhal commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Oct 21, 2023

Uh oh!

aszenz commented Feb 14, 2024

Uh oh!

NattyNarwhal commented Feb 14, 2024

Uh oh!

SakiTakamachi commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NattyNarwhal commented Feb 15, 2024

Uh oh!

SakiTakamachi commented Feb 15, 2024

Uh oh!

SakiTakamachi commented Oct 19, 2023 •

edited

Loading

SakiTakamachi commented Oct 19, 2023 •

edited

Loading

SakiTakamachi commented Oct 20, 2023 •

edited

Loading

NattyNarwhal commented Oct 20, 2023 •

edited

Loading

NattyNarwhal commented Oct 20, 2023 •

edited

Loading

SakiTakamachi commented Feb 15, 2024 •

edited

Loading

SakiTakamachi commented Feb 15, 2024 •

edited

Loading