dg / dibi

Dibi - smart database abstraction layer
https://dibiphp.com
Other
487 stars 136 forks source link

'persistent' connection parameter neglected in postgre driver #349

Closed groupnet closed 4 years ago

groupnet commented 4 years ago

Version: 4.x

Bug Description

The config parameter 'persistent' has no effect on how the Postgres connection is made.

Possible Solution

https://github.com/dg/dibi/pull/348

groupnet commented 4 years ago

One more comment: as the default behavior of pg_connect(...) is to establish a persistent connection, it might be a consideration to make postgre connections via dibi persistent per default too. In other words, only if the 'persistent' connection parameter is explicitly set to false, the connection is non-persistent (connection type PGSQL_CONNECT_FORCE_NEW is set).

If you prefer this breaking change to the bug fix above, I'd create a suitable pull request.

dg commented 4 years ago

closed by #348

dg commented 4 years ago

I am confused about pg_connect vs pg_pconnect vs PGSQL_CONNECT_FORCE_NEW. It seems like something different https://github.com/php/php-src/blob/5d6564101192f8560d175cdec96c319d3c878b82/ext/pgsql/pgsql.c#L1358-L1442

groupnet commented 4 years ago

Sorry, I've linked the wrong doc (I mentioned 'pgconnect' but linked docs of 'pgpconnect').

In any case, I would stick to 'pgconnect(...)', because the latest comments about 'pgpconnect(...)' at php.net mention that it is broken:

You should not use pg_pconnect - it's broken. It will work but it doesn't really pool, and it's behaviour is unpredictable. ... (more details: https://www.php.net/manual/en/function.pg-pconnect.php#87550)

Anyway, what I wanted to point out in my original post above was pg_connect(...) seems to find and grab an existing connection anyway, but only if PGSQL_CONNECT_FORCE_NEW is not explicitly set: https://github.com/php/php-src/blob/5d6564101192f8560d175cdec96c319d3c878b82/ext/pgsql/pgsql.c#L1442-L1456

So I was wondering if it wouldn't make more sense, to call pg_connect(...) without PGSQL_CONNECT_FORCE_NEW per default, like this: https://github.com/groupnet/dibi/commit/1e717ba3981d019433113a4758cbebd4721788a2

In a quick'n'dirty test this seems to speedup the re-connection significantly.

dg commented 4 years ago

I don't know, I've never used PostgreSQL. But I would not like to make a BC break in the patch version, so rather in 4.2.

groupnet commented 4 years ago

I wouldn't either. ;) I'll prepare a pull request (rebased to the current master) - feel free to merge it into 4.2 (or not).

Thanks for your good work!

dg commented 4 years ago

Ok, thanks.