search

php / php-src

The PHP Interpreter
https://www.php.net
Other
38.08k stars 7.74k forks source link

odbc_fetch_*() fails to fetch SQL Server text fields with SQL_CUR_USE_ODBC #15219

Open danielmarschall opened 2 months ago

danielmarschall commented 2 months ago

Description

The following code:

$conn = odbc_connect('DRIVER={SQL Server Native Client 11.0};SERVER=LOCALHOST\SQLSERVER;DATABASE=OIDPLUS;charset=UTF-8;Client_CSet=UTF-8;Server_CSet=UTF-8', 'sa', '123456', SQL_CUR_USE_ODBC);
$res = odbc_exec($conn, "select description from my_objects");
while ($row = odbc_fetch_object($res)) {
    var_dump($row);
}
echo odbc_close($conn);

Resulted in this output:

(Nothing; odbc_fetch_object is false)

But I expected this output instead:

Printed array with descriptions

The bug only happens if odbc_connect() has SQL_CUR_USE_ODBC and it ONLY fails if you fetch exactly one column with the text data type. If you fetch a text row and another row, then it works!

System:

PHP Version

PHP 8.3.6 (also tested with PHP 8.4 Alpha 4)

Operating System

Windows 10 x64

danielmarschall commented 2 months ago

The SQL Server table can be created with this query:

CREATE TABLE [dbo].[my_objects](
    [id] [varchar](255) NOT NULL,
    [description] [ntext] NULL
 CONSTRAINT [PK_my_objects] PRIMARY KEY CLUSTERED 
(
    [id] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON, OPTIMIZE_FOR_SEQUENTIAL_KEY = OFF) ON [PRIMARY]
) ON [PRIMARY] TEXTIMAGE_ON [PRIMARY];

insert into my_objects (id, description) values ('foo', 'bar');
insert into my_objects (id, description) values ('foo2', 'bar');
insert into my_objects (id, description) values ('foo3', 'bar');
cmb69 commented 2 months ago

I get the expected result with the ODBC Driver 17 for SQL Server. Note that the Installing SQL Server Native Client documentation states that this driver isn't recommended for new application development, and that user should use the ODBC Driver for SQL Server instead. Can you at least test whether this driver would solve the issue for you?

Also, consider to switch to PECL/sqlsrv instead of ext/odbc, since the former is likely way better supported for SQL Server.

danielmarschall commented 2 months ago

@cmb69 Thank you very much for the fast reply!

I have now tested "SQL Server" (Generation 1), "SQL Server Native Client 11.0" (Generation 2), and "ODBC Driver 17 for SQL Server" (Generation 3), all have the same behavior.

I could reproduce it on two computers (I will test a 3rd computer tomorrow):

<?php

echo PHP_VERSION."\n";
echo (PHP_INT_SIZE == 4 ? '32' : '64')." Bit\n";

$conn = odbc_connect('DRIVER={ODBC Driver 17 for SQL Server};SERVER=ELY2\TEST;DATABASE=TEST', 'sa', '.......', SQL_CUR_USE_ODBC);

$res = odbc_exec($conn, "select id, description from my_objects");
if (odbc_fetch_object($res) === false) {
    echo "NOT GOOD\n";
} else {
    echo "GOOD\n";
}

$res = odbc_exec($conn, "select description from my_objects");
if (odbc_fetch_object($res) === false) {
    echo "NOT GOOD\n";
} else {
    echo "GOOD\n";
}

odbc_close($conn);

returns:

8.3.6
64 Bit
GOOD
NOT GOOD

Is there anything I can do to help finding the issue?

(PS: Regarding alternatives to ext/odbc, I am building a OpenSource app which should connect to as many different database technologies as possible, so I implemented ODBC, PDO, ADO, MySQLi, PgSQL, SQLite3, ... so that end-users hopefully can keep their system and don't need to reconfigure/install something)

danielmarschall commented 2 months ago

For computer 2, the SQL Server Profiler shows that the query has correctly arrived at the server. Though I need to admit, I have very limited knowledge with the profiler.

image

danielmarschall commented 2 months ago

I tested it on a 3rd computer. Also the same result. This time, I had a 32-bit PHP instead of 64-bit.

8.3.6
32 Bit
GOOD
NOT GOOD

Computer 3

cmb69 commented 2 months ago

Actually, I can reproduce the reported behavior now (I might have made a mistake while adjusting the script to my environment).

A quick look at the ODBC trace reveals:

DIAG [SL009] [Microsoft][ODBC Cursor Library] Vor dem Aufrufen von SQLFetchScroll/SQLExtendedFetch waren keine Spalten gebunden (0)

I'll have a closer look.

cmb69 commented 2 months ago

D'oh! Apparently yet another (?) manifestation of using SQLBindCol() vs. SQLGetData(), and it looks like having no SQLBindCol() causes SQL_CUR_USE_ODBC queries to fail. Now on to the fun part, i.e. reading the specs.

cmb69 commented 2 months ago

Now on to the fun part, i.e. reading the specs.

That wasn't that bad, after realizing that this already caused when calling SQLExtendedFetch(). From https://learn.microsoft.com/en-us/sql/odbc/reference/appendixes/odbc-cursor-library-error-codes?view=sql-server-ver16:

SL009 No columns were bound prior to calling SQLFetch or SQLFetchScroll.

And if we had proper error handling

patch ````diff ext/odbc/php_odbc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ext/odbc/php_odbc.c b/ext/odbc/php_odbc.c index 579b5e989b..0eacbc848c 100644 --- a/ext/odbc/php_odbc.c +++ b/ext/odbc/php_odbc.c @@ -1326,6 +1326,7 @@ static void php_odbc_fetch_hash(INTERNAL_FUNCTION_PARAMETERS, int result_type) rc = SQLFetch(result->stmt); if (rc != SQL_SUCCESS && rc != SQL_SUCCESS_WITH_INFO) { + odbc_sql_error(result->conn_ptr, result->stmt, "SQLFetch"); RETURN_FALSE; } ````

We would have seen something like this:

Warning: odbc_fetch_object(): SQL error: [Microsoft][ODBC Cursor Library] Vor dem Aufrufen von SQLFetchScroll/SQLExtendedFetch waren keine Spalten gebunden, SQL state SL009 in SQLFetch in %s on line %d

That would have at least saved some time.

Now the question is what to do about this. Maybe @NattyNarwhal has an idea.

cmb69 commented 2 months ago

For what it's worth, pdo_odbc has the same issue (but at least would show the error):

$pdo = new PDO("odbc:$dsn", $user, $pass, [PDO::ODBC_ATTR_USE_CURSOR_LIBRARY => PDO::ODBC_SQL_USE_ODBC]);
$stmt = $pdo->query("select description from my_objects");
var_dump($stmt->fetchAll());

outputs:

Fatal error: Uncaught PDOException: SQLSTATE[SL009]: <<Unknown error>>: [Microsoft][ODBC Cursor Library] Vor dem Aufrufen von SQLFetchScroll/SQLExtendedFetch waren keine Spalten gebunden (SQLFetchScroll[0] at ext\pdo_odbc\odbc_stmt.c:556) in %s:%d
danielmarschall commented 2 months ago

A small thing I have tested:

alter table my_objects add ansi1 [text] NULL;
alter table my_objects add ansi2 [text] NULL;
alter table my_objects add ansi3 [text] NULL;

Issue also happens for Ansi [text] types, not only Unicode [ntext]. => So Unicode is not the reason why it failed

This also fails: select ansi1, ansi2, ansi3 from my_objects => So the problem is the field type and not "one column"

My theory is that the amount of columns is counted, and if it is 0, then error SL009 is thrown. Maybe the [text] and [ntext] types are excluded from the counting because they cannot be used for binding? (I remember that there are a lot of things you cannot do with text/memo columns). It's still weird because I just want to execute a SQL statement and not prepare/bind a statement.

cmb69 commented 2 months ago

Not quite. The problem is that some column types are bound (i.e. fetched into a variable), and some other types are not bound (their value is retrieved by an extra call); the former cause no issues, but the latter do, if only columns of the latter type are requested. If there was a description2 column of the same type as description, a SELECT description, description2 would also fail.

So the problem is clear; the solution isn't, because not all column types can be bound (well, they could, but the length of the required buffer is hard to know; that's what I meant with "yet another (?) manifestation of using SQLBindCol() vs. SQLGetData()"). A hypothetical solution would be to alter the given query (e.g. instead of SELECT description FROM, send SELECT description, 1 AS dummy FROM), and then bind the dummy (but ignore the result), but I don't think we want to do that. A more practical solution would be to make that a documentation issue (i.e. don't fix anything, but document the behavior). I'm hoping there is a practical solution in between, but I don't see it (yet).

NattyNarwhal commented 2 months ago

Make a PR for that error reporting, it's definitely useful.

I'm not very familiar with SQL Server. Is the problem the same with ntext (which MS seems to say is deprecated) vs. nvarchar(max)? I would think that SQL Server would treat both as normal SQL_CHAR though (or a lob)...

danielmarschall commented 2 months ago

@NattyNarwhal I have just checked that text , ntext , varchar(max) , and nvarchar(max) have the problem

cmb69 commented 2 months ago

Make a PR for that error reporting, it's definitely useful.

See PR #15256.

Is the problem the same with ntext (which MS seems to say is deprecated) vs. nvarchar(max)? I would think that SQL Server would treat both as normal SQL_CHAR though (or a lob)...

Not SQLServer specific, but probably a good reference: https://learn.microsoft.com/en-us/sql/odbc/reference/appendixes/sql-data-types?view=sql-server-ver16

cmb69 commented 2 months ago

The SQLFetch() documentation states:

If no columns are bound, SQLFetch returns no data but does move the block cursor forward. The data can still be retrieved by using SQLGetData.

ext/odbc relies on this behavior.

The SQLExtendedFetch() documentation (and this is what we are actually using here) states:

ODBC 3.x applications should not call SQLExtendedFetch; instead they should call SQLFetchScroll.

That is something we should consider doing anyway. However, I have not tried that, because the ODBC Cursor Library documentation states:

This feature will be removed in a future version of Windows. Avoid using this feature in new development work and plan to modify applications that currently use this feature. Microsoft recommends using the driver's cursor functionality.

So it seems to me that we should possibly deprecate SQL_CUR_USE_ODBC, or at least document that this should not be used (at least when working with SQLServer; or is it a general Windows ODBC issue?; the docs are a bit unclear about that). Currently, the odbc_connect() documentation states:

With some ODBC drivers, executing a complex stored procedure may fail with an error similar to: "Cannot open a cursor on a stored procedure that has anything other than a single select statement in it". Using SQL_CUR_USE_ODBC may avoid that error. Also, some drivers don't support the optional row_number parameter in odbc_fetch_row(). SQL_CUR_USE_ODBC might help in that case, too. And given that ext/odbc still appears to support ODBC versions < 3.0 (its specification has been released in 1995), this all seems overly complex and confusing.

Now reading the odbc_fetch_row() docs, the $row_number paramter is now called $row (minor issue that still needs to be fixed), but there is even no mention regarding SQL_EXTENDED_FETCH, i.e. that the $row argument is ignored if extended fetching is not supported. Now checking the implementation (php_odbc_includes.h), I see that HAVE_SQL_EXTENDED_FETCH is defined for all known ODBC_TYPEs except for Empress; while that DB still appears to be maintained, its ODBC documentation appears to be meager, and I'm not sure whether it still does not support extended fetching, or whether anybody is still using ext/odbc to connect to it, or if ext/odbc can still be build against it.

Anyhow, @danielmarschall, did you use SQL_CUR_USE_ODBC for a particular reason, or just because the documentation states it may help with some issues.

danielmarschall commented 2 months ago

Anyhow, @danielmarschall, did you use SQL_CUR_USE_ODBC for a particular reason, or just because the documentation states it may help with some issues.

The problem I am facing with the ODBC plugin of my software is that it is ultra slow, compared to the other plugins like PDO or ADO. If I iterate over a result set, the server is contacted for each fetch_*() command, so a single query+fetchAll requires approximately 5 seconds, while in PDO it is <0,1 seconds (since it supports FetchAll).

SQL_CUR_USE_ODBC is recommended a lot in internet forums, so I guess I give it a try. It reduces the 5 seconds to approximately 2 seconds (which is still bad). It is also recommended to solve rare problems with stored procedures.

So, everybody says SQL_CUR_USE_ODBC makes things better, but for me it seems like SQL_CUR_USE_ODBC makes things worse in many ways. Not only this bug; I also have another bug where SQL_CUR_USE_ODBC causes "zend_mm_heap corrupted" if a lot of queries (100+) are executed in my app, but I didn't report this bug, because I have no minimal reproducible example...

A naïve question...: if there is such a problem with ODBC and the "no columns are bound" problem - how are vendors other than PHP handling this? Do they also fail?

cmb69 commented 2 months ago

A naïve question...: if there is such a problem with ODBC and the "no columns are bound" problem - how are vendors other than PHP handling this? Do they also fail?

Frankly, I don't know. I guess that issue is related to the SQLFetch()/SQLGetData() issue. "Normal" usage is probably to bind all columns to buffers, and call SQLFetch() to fill these buffers with the required data. Now for some column types it is inherently hard to know in advance how large these buffers need to be (different drivers appear to report different values for SQL_COLUMN_DISPLAY_SIZE/SQL_DESC_OCTET_LENGTH regarding charset encoding), so for these column types the implementation falls back on SQLGetData(), which appears to work fine in most cases. This might cause a performance penalty (maybe it's even the sole reason for the performance issues you've mentioned), and obviously breaks down in this case if no columns are bound. Now, it would be possible to bind all columns, and check whether truncation occured, on only use SQLGetData() additionally in this case, but if I remember correctly, we also had issues with this approach. There are simply so many ODBC drivers which handle the details differently, and since we apparently even support so many different ODBC implementations (I count 12! known implementations in pdo_odbc_includes.h), which may even support only very old ODBC versions, it is hard to find solutions which won't break other existing code. And to my knowledge, on CI we only test ext/odbc and ext/pdo_odbc against SQL Server with a single driver (ODBC Driver 17 for SQL Server). And since the introduction of PDO, the focus of most developers likely shifted away from ODBC – different databases are different beasts anyway, and trying to put a compatibility layer in between might make things even worse (or at least harder to deal with at the implementation level).