Bug #80458 PDOStatement::fetchAll() throws for upsert queries #6482
Conversation
ext/pdo_mysql/tests/bug80458.phpt
Outdated
| [0]=> | ||
| string(1) "6" | ||
| } | ||
| } No newline at end of file |
|
I'm not sure this fix is right. When linking against libmysqlclient and running the added test case, I get:
This shows that libmysqlclient agrees with that this is an error. As such it needs to stay an error at least at the level of mysqlnd. If we want to treat this as not an error in PDO, then I believe the fix needs to be on the side of PDO, not mysqlnd. |
|
@nikic Ok, I agree. I reverted the change back and added a check in PDO to see if the statement produces a result or not. I have also added a more comprehensive test case. |
ext/pdo_mysql/mysql_statement.c
Outdated
|
|
||
| if(S->stmt) { | ||
| MYSQL_RES *prepare_meta_result; | ||
| if(!(prepare_meta_result = mysql_stmt_result_metadata(S->stmt))) { |
There was a problem hiding this comment.
I don't think this is a function that should be called on each fetch. Shouldn't the result metadata already be stored inside S->result?
There was a problem hiding this comment.
Makes sense. I replaced it with an existence check on S->stmt->data->result which I assume is allowed.
|
Not to forget. This PR is aiming to fix https://bugs.php.net/bug.php?id=80267 and https://bugs.php.net/bug.php?id=80458 |
ext/pdo_mysql/mysql_statement.c
Outdated
| { | ||
| pdo_mysql_stmt *S = (pdo_mysql_stmt*)stmt->driver_data; | ||
|
|
||
| if(S->stmt && S->stmt->data && !S->stmt->data->result) { |
There was a problem hiding this comment.
Why is this not just if (!S->result)?
In which case the same check below could also be dropped.
There was a problem hiding this comment.
That's because I thought that with unbuffered queries S->result would be NULL but it looks like I was wrong. IIUC S->result is populated always with the metadata, correct? I changed it now.
|
Had a production application break because a new container was created by CI pipeline on PHP 7.4.13 instead of PHP 7.4.12. The application utilised a data access layer with a generic query method which would always try and fetch data from the statement, irrespective of the query type. Our CI pipelines are allowed to build containers using the latest PHP patch version on the general assumption that it should be safe to use. This was not the case and it has caused downtime for customers; I am greatly looking forward to 7.4.14 hopefully incorporating this PR and addressing 80267/80458. |
Thanks for the feedback. I assume you believe this behaviour is a bug rather than the expected behaviour. Does it mean your application is exclusively using native prepared statements? I also believe this is a bug, but emulated statements have triggered an exception in this scenario for years. This PR is trying to silence the exception in both emulated and native prepared statements. |
|
Confirming that this works on libmysqlclient as well. (The test fails due to type differences, but that's a different problem.) |
nikic
left a comment
There was a problem hiding this comment.
Okay, this looks good to me now. I think this is the right direction. While performing fetches for upsert is incorrect if we're being pedantic, I don't see much benefit in enforcing that. Consistently being able to use fetch even if there are no results is more valuable.
|
Merged as a83cc03 in PHP-7.4 upwards. |
Yes, we exclusively use native prepares. I really don't want to get into the discussion of whether this was strictly-speaking a bug or not; to me as a PHP user, it most certainly was a bug because code that worked flawlessly in 7.4.12 broke overnight in 7.4.13. That defies basic expectations around semantic versioning. Glad to see this PR. |
While the previous fixes did improve error reporting, it turns out that a lack of a result set is not an error. MySQLnd always triggers an OOS error if you try to fetch from a prepared statement and there is no result. The behaviour is now more or less aligned with emulated prepares which would throw an exception with "General error" if a result set was not present. However, in my opinion, a lack of a result is not an error. For UPDATE/DELETE and any other query that doesn't produce results, it is expected that there will be no result returned. As can be observed by the 2 bug reports the community expects an empty array instead of some confusing exception.
This PR aims to fix the problem. First, we check in mysqlnd for a missing result. If there is no result then we return false without an exception. Second, when using emulated prepares and performing an UPSERT query we do the same. This aligns emulated prepares and native prepares back again and produces an empty array when calling fetchAll().