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: 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: Nitpick I have found that probably doesn't matter: odbc_result_all doesn't work with the patch, it seems. It's deprecated, so I assume it's not a big deal, but it's something I noticed debugging some unrelated code. Previous Comments: ------------------------------------------------------------------------ [2022-01-20 19:11:37] calvin at cmpct dot info Brief update w/ the PDO_ODBC patch from last week: The user does report the patch works for them without any (noticeable) performance regression. Probably a bit rough still, but good to know so far. ------------------------------------------------------------------------ [2022-01-14 16:49:39] cmb@php.net > So, I've ported the logic over for PDO_ODBC and I have the patch > here: […] Ah, cool! It might be possible to simplify further by just setting S->going_long = 1 early (but that doesn't matter for now). > coercing to string, have to get assembled in a 256 byte buffer > piecemeal I think that code is indeed rather suboptimal. I still wonder why fetching in small chunks is claimed to be faster than reading a single chunk[1]. > PDO_ODBC is *noticeably* faster at least on Linux. That's interesting! Might be worth investigating. [1] <https://github.com/php/php-src/pull/6716#issuecomment-783461335> ------------------------------------------------------------------------ [2022-01-14 16:31:56] calvin at cmpct dot info 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...) ------------------------------------------------------------------------ [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. ------------------------------------------------------------------------ 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