Bug #81688 [Com]: PDO_ODBC doesn't handle fixed-length character columns with character conversio

Edit report at https://bugs.php.net/bug.php?id=81688&edit=1 ID: 81688 Comment by: calvin at cmpct dot info Reported by: calvin at cmpct dot info Summary: PDO_ODBC doesn't handle fixed-length character columns with character conversio Status: Feedback Type: Bug Package: PDO ODBC Operating System: IBM i 7.2 PHP Version: 8.0.13 Assigned To: cmb Block user comment: N Private report: N New Comment: I think I'm OK with compile-time for now. We mostly support users running on i directly (with our packages, so we can easily include this patch), though some do use the same driver on Linux/Windows with official PHP/distro binaries. FWIW, most of our users are also running PDO_ODBC, mostly because that supports in/out parameters with stored procedures (which, I haven't checked if that's impacted...). I skimmed the source to see what it'd take to do the same, since I think it's a little more dependent on binding for fetching data. I'm a little concerned about perf since you mentioned the possibilities about other scenarios and maybe causing a regression for realistic workloads, but I'll try the microbenchmark again with a larger count of rows. Previous Comments: ------------------------------------------------------------------------ [2022-01-05 22:28:50] cmb@php.net Thanks for checking and the good news! Regarding the performance, I'm surprised, but an MSDN article about fetching result data[1] possibly clarifies: | If a result set contains only a few rows, using SQLGetData | instead of SQLBindCol is faster; otherwise, SQLBindCol gives the | best performance. And: | When using server cursors, the SQL Server Native Client ODBC | driver is optimized to not transmit the data for unbound text, | ntext, or image columns at the time the row is fetched. The text, | ntext, or image data is not actually retrieved from the server | until the application issues SQLGetData for the column. So probably the performance of SQLBindCol() vs. SQLGetData() likely depends very much on the driver. If the latter requires additional requests to the server, performance might drastically degrade. It might make sense to do more extensive testing (although, we cannot hope to cover nearly all cases), but at least wrt. fixing a bug (i.e. changes to PHP-8.0), we need to be conservative anyway. Would it work for you/your client to do the builds with a compile time constant (maybe configurable)? If so, I could do a pull request based on the latest patch (plus a similar solution for PDO_ODBC), and we can go from there. More aggressive changes might be doable for the master branch (PHP 8.2) afterwards. At least the code in ext/odbc looks like it needs some overhaul anyway. [1] <https://docs.microsoft.com/en-us/sql/relational-databases/native-client-odbc-results/fetching-result-data?view=sql-server-ver15> ------------------------------------------------------------------------ [2022-01-05 18:23:04] calvin at cmpct dot info I tried the patch and not only does it work (as below): ``` Record string(180) "éééééééééééééééééééééééééééééé " Record string(165) "ééééééééééééééé " Record string(155) "ééééé " ``` ...it's actually faster. I took the sample from earlier, wrapped the prepare-execute-fetch-display in a 10000 iteration loop, ran the script 10 times (to average out outliers - it's not super scientific, but to make it fair), and unmodified PHP returning garbage takes 10 seconds, and your fix takes 7 seconds. Now I'm wondering if SQLGetData might be more efficient for other scenarios... ------------------------------------------------------------------------ [2022-01-05 16:15:37] cmb@php.net I made a quick patch which disables any column binding for ext/odbc[1]. That is, all data are now retrieved by SQLGetData(). This is obviously less efficient, but I like to know whether that would work with the IBM driver/DB. Could you please check it out? Note that all ext/odbc tests work for me like without the patch (or undefining ODBC_DONT_BIND), except for bug44618.phpt which now is able to retrieve the third column. [1] <https://gist.github.com/cmb69/839d5e04395936ad64fd049d2cb7fd55> ------------------------------------------------------------------------ [2021-12-30 23:06:56] cmb@php.net Oh yeah, sorry, zeroing the memory can't work, since the length is only properly set when fetching, so we cannot do that upfront. Sorry for the delay, once again. I shall dig deeper ASAP. ------------------------------------------------------------------------ [2021-12-28 16:58:01] calvin at cmpct dot info Was going to write a sample w/ ODBC in C, but decided to poke the existing code path to determine if it's just junk left over before, or just created after. Gist b/c it's too long to post here: https://gist.githubusercontent.com/NattyNarwhal/3864224066dda8ed4be991c06e67a670/raw/3e3aad9efbc6472f7dde1e8e1167e2bc9ad06288/gistfile1.txt (I rebuilt PHP w/ --enable-debug for my own sanity in GDB. Also on Linux too.) Note that the garbage on 0x7ffff767e3a8 that appears in the string comes from before the memset, so I think this is just leftover in memory and not coming from the driver. (You can also see my connection string... thankfully not my password.) ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=81688 -- Edit this bug report at https://bugs.php.net/bug.php?id=81688&edit=1