Minor optimization to remove PostgreSQL version check (1 SQL query pe… #334
Conversation
…r connection) when the version has been EOL for over 11 years.
|
@IPSO Thanks for your contribution.
Definitely long overdue. I actually have had this on the roadmap for a long while, but lack the time to get to it. With regards to the proposed fix, even if it's true that the risk of causing a regression is very low, I nevertheless believe it may be more appropriate to define |
|
Personally I don't see the point in supporting a product that the original developers haven't supported for over a decade. I would prefer to see the 6.4 driver be removed completely, but of course that is a much larger job and more invasive job. I would also be surprised if PHP v5.X can even connect to a PostgreSQL version that old anymore, seeing as PostgreSQL frontend/backend protocol has been changed several times since then too. Based on that, I don't even feel its worth discussing, but its entirely up to you. |
| @@ -728,11 +728,9 @@ function _connect($str,$user='',$pwd='',$db='',$ctype=0) | |||
| if ($this->_connectionID === false) return false; | |||
| $this->Execute("set datestyle='ISO'"); | |||
|
|
|||
| $info = $this->ServerInfo(); | |||
There was a problem hiding this comment.
$info is also used at line 741 below, so removing this line requires additional changes to ensure the version check works.
There was a problem hiding this comment.
My bad, should be fixed now.
Good idea. That should be fairly straightforward to implement actually, and we could take this opportunity to improve ServerInfo() so that it no longer needs to parse the version string returned by
If I understand that correctly, the problem occurs when pgsql server is 9.0+ (which changed the default from escape to hex), while the pgsql client is < 9.2 (which fixed the problem, at least according to https://bugs.horde.org/ticket/10919#c4). If that is true, then the current check against the server version is not sufficient, we also need to check the client version. |
Make use of the revised ServerInfo() method to avoid running a query with each connection to retrieve the server version number. Fixes ADOdb#334
|
@Mike-Benoit please have a look at https://github.com/dregad/ADOdb/tree/pr334 for an alternative to your PR, along the lines of my previous note, and let me know what you think. |
|
After a quick look it appears to be a better approach, though I haven't tested it yet. Is there any "official" stand on what database server versions ADOdb should or should not support? Official guide lines would be nice so people aren't submitting PR's that remove functionality that may still be required. There are quite a few complaints regarding PHP and LIBPQ versions not matching, I would be quite surprised if PGSQL < v7.x even works in any version of PHP that ADOdb supports. If that turns out to be the case, then all the code introduced to enable/disable _nestSQL is just wasted effort. |
Upon every connection to PostgreSQL ADOdb was calling a "select version()" to see if the server version was >= 7.1 in order to enable nested SQL query mode. However v7.1 has been EOL for over 11 years, so I think its safe to remove this check and just default to nested SQL mode all the time.
Even though its only 1 SQL query, for busy sites such as ours it saves millions of queries/day, especially when using the soon to be merged load balancer which can make multiple connections to different servers.
The PostgreSQL drivers probably could use a refactor, as v6.4 is almost 20 years old, but if someone is using a database that old they are unlikely to be upgrading ADOdb.