Bug #81688 [Fbk->Opn]: 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 Updated by: cmb@php.net Reported by: calvin at cmpct dot info Summary: PDO_ODBC doesn't handle fixed-length character columns with character conversio -Status: Feedback +Status: Open 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 Previous Comments: ------------------------------------------------------------------------ [2022-01-06 18:00:20] calvin at cmpct dot info OK, expanding it to 26 rows (not quite "big" but less trivial than a single row, albeit still a single column and printing the output) is ~43s for unmodified and ~30s for your patch. FWIW, I also tested a version that doesn't bother with the bug and just the differences in approach; basically the same test against a more "realistic" table (on i, QIWS.QCUSTCDT is basically a multirow, multi-column table with a variety of data that most people use for tests like this) and not printing any rows (to avoid measuring the costs writing the data out). The result is the unmodified PHP is a bit faster (14.5496666666667) versus your patch (14.9203333333333) but it's very small and perhaps within the margin of error. ------------------------------------------------------------------------ [2022-01-06 16:46:53] calvin at cmpct dot info 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. ------------------------------------------------------------------------ [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> ------------------------------------------------------------------------ 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