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

From: Date: Wed, 05 Jan 2022 22:28:50 +0000
Subject: Bug #81688 [Fbk]: PDO_ODBC doesn't handle fixed-length character columns with character conversio
References: 1  Groups: php.bugs 
Request: Send a blank email to php-bugs+get-238785@lists.php.net to get a copy of this message
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
 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:

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>


Previous Comments:
------------------------------------------------------------------------
[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.)

------------------------------------------------------------------------
[2021-12-27 22:10:27] cmb@php.net

> […], just chuffed at the issue tracker that it got auto-closed.

Yeah, that is a bit unfortunate.  If the status is set to
"feedback", the bug is automatically closed on the next but one
Sunday, even if feedback has been provided.  You, as bug reporter,
should be able to change the status back to "open", though
("re-opened" doesn't work, IIRC).

> but I don't notice any different behaviour

Hmm, does the *driver* fill the buffers with some garbage at the
end, in this case?  I'll have a closer look tomorrow; it seems
using SQLGetData() always (for testing purposes) might be a way to
proceed.

------------------------------------------------------------------------


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


Thread (37 messages)

« previous php.bugs (#238785) next »