amphp / postgres

Async Postgres client for PHP based on Amp.
MIT License
97 stars 20 forks source link

Why just "RESET ALL" is used to reset the state of a pooled connection? #34

Closed fabriziomello closed 4 years ago

fabriziomello commented 4 years ago

Hi all,

While looking at how connection pool is implemented I got surprised about the current implementation uses just RESET ALL [1] statement to do the job. According PostgreSQL documentation [2] it will reset just run-time parameters.

Sorry about my very newbie question about the project. I just got curious about how advanced things we can do with PHP nowadays (used PHP for long time in the past) and nowadays I help PostgreSQL development.

So if the real intention here is to restore connection to it initial state in PostgreSQL it should use DISCARD ALL [3] instead, because can exist other temp resource we can left behind without doing it that can lead us to leak resources, for example:

Another example is pgbouncer [1] the most known connection pool for PostgreSQL that uses DISCARD ALL as the default of server_reset_query.

[1] https://github.com/amphp/postgres/blob/master/src/Pool.php#L82 [2] https://www.postgresql.org/docs/current/sql-reset.html [3] https://www.postgresql.org/docs/current/sql-discard.html [4] http://www.pgbouncer.org/config.html#connection-sanity-checks-timeouts

trowski commented 4 years ago

Hi!

Thanks for bringing this to my attention. The intention was restoring the connection to the initial state. I was not aware of DISCARD ALL and thought RESET ALL was the correct statement. I'll update the code and tag a release.