Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

with ATTR_EMULATE_PREPARES = false, don't ignore deadlock errors #5937

Conversation

SjonHortensius
Copy link
Contributor

I have tested this patch and can confirm it also fixes bug#77163

@SjonHortensius SjonHortensius changed the base branch from master to PHP-7.4 August 5, 2020 05:12
@php-pulls php-pulls force-pushed the PHP-7.4 branch 2 times, most recently from 4e0f9e4 to 6a5f851 Compare August 5, 2020 08:37
@SjonHortensius SjonHortensius force-pushed the fix_bug79914_pdo_ignores_deadlock_errors branch 3 times, most recently from 6addcb2 to d7027cb Compare August 8, 2020 08:35
@cmb69 cmb69 mentioned this pull request Aug 8, 2020
@cmb69
Copy link
Member

cmb69 commented Aug 25, 2020

To fix the compile error, a forward declaration of _pdo_mysql_stmt_error() could be added:

 ext/pdo_mysql/php_pdo_mysql_int.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ext/pdo_mysql/php_pdo_mysql_int.h b/ext/pdo_mysql/php_pdo_mysql_int.h
index cfaf9fe1cf..b206333248 100644
--- a/ext/pdo_mysql/php_pdo_mysql_int.h
+++ b/ext/pdo_mysql/php_pdo_mysql_int.h
@@ -150,6 +150,7 @@ typedef struct {
 extern const pdo_driver_t pdo_mysql_driver;
 
 extern int _pdo_mysql_error(pdo_dbh_t *dbh, pdo_stmt_t *stmt, const char *file, int line);
+extern int _pdo_mysql_stmt_error(pdo_dbh_t *dbh, pdo_stmt_t *stmt, const char *file, int line);
 #define pdo_mysql_error(s) _pdo_mysql_error(s, NULL, __FILE__, __LINE__)
 #define pdo_mysql_error_stmt(s) _pdo_mysql_error(stmt->dbh, stmt, __FILE__, __LINE__)
 #define pdo_mysql_error_stmt_emulate_false(s) _pdo_mysql_stmt_error(stmt->dbh, stmt, __FILE__, __LINE__)

I can confirm that the PR would fix #77163 then. However, several tests are failing, and at least some of these indicate that this PR is not quite correct.

@SjonHortensius SjonHortensius force-pushed the fix_bug79914_pdo_ignores_deadlock_errors branch 4 times, most recently from 365a495 to 94786ed Compare August 25, 2020 09:52
@SjonHortensius
Copy link
Contributor Author

@houxiaoxian as you can see a couple of tests fail because of this change:

  • Bug #74376 (Invalid free of persistent results on error/connection loss) [ext/pdo_mysql/tests/bug_74376.phpt]
  • PDO::ATTR_ORACLE_NULLS [ext/pdo_mysql/tests/pdo_mysql_attr_oracle_nulls.phpt]
  • MySQL PDOStatement->nextRowSet() with PDO::MYSQL_ATTR_MULTI_STATEMENTS either true or false [ext/pdo_mysql/tests/pdo_mysql_multi_stmt_nextrowset.phpt]
  • MySQL PDOStatement->bindParam() [ext/pdo_mysql/tests/pdo_mysql_stmt_bindparam.phpt]
  • MySQL PDOStatement->closeCursor() [ext/pdo_mysql/tests/pdo_mysql_stmt_closecursor.phpt]
  • MySQL PDOStatement->closeCursor() [ext/pdo_mysql/tests/pdo_mysql_stmt_closecursor_empty.phpt]

someone will need to fix these to get this PR accepted. If you are going to refactor your fix, would you please consider re-integrating your changes into _pdo_mysql_error instead of your new _pdo_mysql_stmt_error, which is mostly a copy?

@houxiaoxian
Copy link

OK, I'll try it, thanks.@SjonHortensius

@houxiaoxian
Copy link

pdo_mysql.zip
try it , :)

@SjonHortensius SjonHortensius force-pushed the fix_bug79914_pdo_ignores_deadlock_errors branch from 94786ed to efca656 Compare September 1, 2020 07:30
@houxiaoxian
Copy link

the new patch just modify pdo_statement.c.

@houxiaoxian
Copy link

all the files changed before should return to the original.

@SjonHortensius
Copy link
Contributor Author

the new patch just modify pdo_statement.c.

thanks, that looks a lot cleaner 👍

I do however see a few segmentation faults in the tests that need to be fixed

@houxiaoxian
Copy link

the new patch just modify pdo_statement.c.

thanks, that looks a lot cleaner

I do however see a few segmentation faults in the tests that need to be fixed

the test error is same to the last test. I want to know whether this will passed with No changes. Can we try it.

@SjonHortensius
Copy link
Contributor Author

@houxiaoxian you can see the tests for the same branch without modifications here: https://travis-ci.org/github/php/php-src/builds/722772381 - they all seem to pass fine

@kamil-tekiela kamil-tekiela mentioned this pull request Sep 24, 2020
@SjonHortensius
Copy link
Contributor Author

suspended in favor of #6203

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants