pdo_pgsql: Reset persistent session state on disconnect-equivalent processing #20572
Conversation
KentarouTakeda
requested review from
SakiTakamachi and
devnexen
as code owners
|
nice ! I may have a better look today (or Saki might) as far as statements are deallocated beforehand i.e. "DEALLOCATE " then it should be good but I surely do not know pdo_pgsql code by heart. cheers |
|
nvm so |
| require __DIR__ . '/../../../ext/pdo/tests/pdo_test.inc'; | ||
|
|
||
| $pdo1 = PDOTest::test_factory(__DIR__ . '/common.phpt'); | ||
|
|
There was a problem hiding this comment.
nit: would it be possible to store SELECT pg_backend_pid() query before this one and the other and checking if they re equal ?
There was a problem hiding this comment.
I've added it to all the tests (now 3) related to this issue.
You were right. I was confused myself by the 3 tests, some of which keep the pid the same and some of which intentionally change it :'D
| pdo_pgsql_db_handle *H = (pdo_pgsql_db_handle *)dbh->driver_data; | ||
|
|
||
| if(H->server) { | ||
| res = PQexec(H->server, "DISCARD ALL"); |
There was a problem hiding this comment.
might be nice to have a test with a transaction being interrupted, then forcing the shutdown wdyt ?
There was a problem hiding this comment.
I added a test case that "attempts to perform disconnection-equivalent processing after being disconnected from the outside." This is because session-level parameters and advisory locks have no concept of transactions.
For normal transactions, the previous implementation appears to correctly detect and initialize transactions, even if the BEGIN statement is issued independently with exec() without using beginTransaction():
php-src/ext/pdo_pgsql/pgsql_driver.c
Line 583 in 1ee8dfd
Lines 1588 to 1591 in 1ee8dfd
KentarouTakeda
force-pushed
the
pdo-pgsql/reset-persistent-session-on-disconnect
branch
from
9ff988a to
1754f4c
Compare
| ->query('show log_min_duration_statement;') | ||
| ->fetchColumn(0); | ||
|
|
||
| echo "pid1 is equals to pid2: " . (($pid1 === $pid2) ? 'yes' : 'no') . "\n"; |
There was a problem hiding this comment.
it is supposed to be yes, I would prefer then an assertion or if (...) die(...);
There was a problem hiding this comment.
I've switched to using assert(). I noticed other tests doing the same. Thanks for pointing that out!
There was a problem hiding this comment.
I ll give another round of review later but I do not foresee any major concern, it will probably be ok.
KentarouTakeda
force-pushed
the
pdo-pgsql/reset-persistent-session-on-disconnect
branch
from
1754f4c to
124a1ca
Compare
2 participants
This patch makes
ext/pdo_pgsqlclear session-local state when performing disconnect-equivalent processing by issuing a singleDISCARD ALLon thelibpqconnection.Why
Persistent PDO connections could carry session-scoped state across disconnect-equivalent lifecycles, causing surprising behavior for users (changed session settings, leftover temporary objects or prepared statements, or stuck advisory locks).
Clearing the session on disconnect-equivalent processing prevents these surprises.
ext/pgsqlalready performs a session reset usingRESET ALL;DISCARD ALLis more comprehensive (it also releases advisory locks and removes temporary objects), so pdo_pgsql usesDISCARD ALLfor broader cleanup.Risk / Benefit
The change runs only during disconnect-equivalent cleanup and does not affect active connections. The small runtime cost of the
DISCARD ALLcall is outweighed by reducing hard-to-diagnose bugs caused by stale session state.Ref
ext/pgsqlsRESET ALL