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: Assigned
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:
The user having the issue wanted to try the fix with their existing code, but was using PDO. So,
I've ported the logic over for PDO_ODBC and I have the patch here: https://gist.github.com/NattyNarwhal/077dbd184f08ac030f8b6088b3d39ed4
- I'll try it out with them soon and see how it goes.
It's quite simple and like the procedural ODBC one, works by basically skipping functionality
and using existing things. There's almost certainly a better solution, as this patch does all
sorts of bad things (i.e. coercing to string, have to get assembled in a 256 byte buffer piecemeal),
but it does fix the primary issue here.
(Also, unrelated observation and happens with/without any of the patches: PDO_ODBC is *noticeably*
faster at least on Linux. I can actually see the results stream out w/ procedural, but they're
instant with PDO, and that's without fetchAll. Quite interesting...)
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