Cannot connect to recent versions of pgbouncer about dbdpg HOT 33 CLOSED

Looks like pg_bouncer is returning its own version as the server_version parameter, which is where I suspect this is falling down. Since DBD::Pg uses server_version to determine feature support I'm not seeing how too loosen this without losing the capability; arguably pgbouncer is being stupid here by choosing to override this specific parameter.

From the logs, it is likely this changed in f6e6045, where it would previously fall back to parsing the version from the version string instead of relying solely on the server_version parameter, like it does now:

-       if (imp_dbh->pg_server_version <= 0) {
-               int     cnt, vmaj, vmin, vrev;
-               const char *vers = PQparameterStatus(imp_dbh->conn, "server_version");

-               if (NULL != vers) {
-                       cnt = sscanf(vers, "%d.%d.%d", &vmaj, &vmin, &vrev);
-                       if (cnt >= 2) {
-                               if (cnt == 2) /* Account for devel version e.g. 8.3beta1 */
-                                       vrev = 0;
-                               imp_dbh->pg_server_version = (100 * vmaj + vmin) * 100 + vrev;
-                       }
-               }
-               else {
-                       imp_dbh->pg_server_version = PG_UNKNOWN_VERSION ;
-               }
+       if (imp_dbh->pg_server_version < 80000) {
+               TRACE_PQERRORMESSAGE;
+               strncpy(imp_dbh->sqlstate, "08001", 6); /* sqlclient_unable_to_establish_sqlconnection */
+               pg_error(aTHX_ dbh, CONNECTION_BAD, "Server version 8.0 required");
+               TRACE_PQFINISH;
+               PQfinish(imp_dbh->conn);
+               sv_free((SV *)imp_dbh->savepoints);
+               if (TEND_slow) TRC(DBILOGFP, "%sEnd dbd_db_login (error)\n", THEADER_slow);
+               return 0;
        }

Thinking about a fix; open to suggestions, as obviously not being able to connect to pgbouncer from DBD::Pg is bad.

from dbdpg.

What's wrong with continuing to fall back to the version string? I get that it's less elegant, but unless people on this project have a commit bit for every PostgreSQL-compatible thing, it's not super reasonable to demand this new compatibility all of a sudden.

from dbdpg.

The version string parsing happens in libpq (based on the server_version parameter), it's just the check for pg_server_version < 80000 that's new.

If pgbouncer puts its own version number in the server_version parameter, that already broke using any features that require a newer server, such as NULLs in array bind values and version-specific pg_catalog queries.

from dbdpg.

I suspect it's handling things its own particular way in the case of DBs other than its own console. I found it because I was querying the console.

from dbdpg.

Well, if it's handling the pooled DBs correctly, then I think it's less urgent of an issue and more an issue of convenience. I don't think a specific code path to accommodate this specific use case is worth what could just be a simple drop-down to a psql command if you're just trying to monitor some of pgbouncer's internals. (Not that it wouldn't be nice to support both, but it's not the breaking change I was thinking it was.)

from dbdpg.

If we can't trust server_version, then it's the other software that's broken.

from dbdpg.

@machack666 I added the connect-time check to give a more user-friendly error up-front, instead of having things fail with more mysterious error messages (syntax errors, unknown pg_catalog tables/columns) or scattering checks for obsolete versions around the codebase.

I think a compromise would be to restore those checks, but just throw an error instead of working around the missing bits.

from dbdpg.

@ilmari so basically instead of the error when connecting, you only get an error if you exercise an unsupported code path for this version? We could also see about some sort of warning when connecting if we see server_version is lower than 8.0, though I can see that that would be annoying for users.

from dbdpg.

@machack666 yes, a draft is here: https://github.com/bucardo/dbdpg/compare/relax-8.0-checks
A warning (that can be silenced by an environment variable or connection parameter) might also be an option. Or maybe an environment variable or connection parameter to just disable the version check completely?

from dbdpg.

Yeah, I'm not sure without reviewing more deeply what parts of the code those specific checks are in (so hence what might end up blowing up when connecting as @davidfetter is trying to do), but in theory I like the changes. I do think some method of disabling the safety measures for this might be required though, so presumably an envvar would do the trick. Also, if someone is ending up trying to hack their way around proper version support it's probably not terrible to make an envvar to shut off the warnings too.

from dbdpg.

Any progress on this? I'm generally okay with the fallback-for-stupid-programs (hi pgbouncer!) approach. I need to confirm if/how pgbouncer is actually changing server_version, which is absolutely should not be doing.

from dbdpg.

from dbdpg.

from dbdpg.

I don't like the env approach: DBD::Pg should simply "just work" as much as possible. I'm thinking if the first check fails, we use PQparameterStatus to look at server_version directly, and simply allow things to proceed if it contains 'bouncer'. The dangers seem very low, and limited to failures of newer features when connected directly to pgbouncer meta-database.

As a further ding at pgbouncer, the docs say "Note that server_version, server_encoding and integer_datetimes cannot change after startup.", which implies they should be hands-off to anything but Postgres itself.

from dbdpg.

from dbdpg.

server_version is a string: PQserverVersion converts it to a number.

from dbdpg.

@davidfetter Please give cb71a77 a whirl.

from dbdpg.

Cannot you issue SQL query SHOW server_version_num to get server version when you detect that server is pgbouncer and PQserverVersion does not return useful value?

from dbdpg.

We cannot rely on server_version_num until the minimum backend is 8.2. And we are not going to change that because of pgbouncer's misbehavior. The only real option is to parse SELECT version() ourselves, which seems overkill but I'm open to it.

from dbdpg.

Or you can use SHOW server_version. This is supported back to 8.0 (https://www.postgresql.org/docs/8.0/runtime-config.html) and is easier to parse than SELECT vesion().

from dbdpg.

Yes, good point. Applied a POC in 00a7153

from dbdpg.

This is what I get with HEAD (00a7153):

perl -MDBI -E 'my $dbh_pgb=DBI->connect("dbi:Pg:dbname=pgbouncer;host=/tmp;port=6433","pgbouncer","")
    or die "Could not connect to pgbouncer:$!";
    $sth=$dbh_pgb->prepare("SHOW active_sockets");
    $sth->execute;'
row number 0 is out of range 0..-1
Segmentation fault (core dumped)

from dbdpg.

Interestingly, it doesn't bomb on cb71a77:

perl -MDBI -E 'my $dbh_pgb=DBI->connect("dbi:Pg:dbname=pgbouncer;host=/tmp;port=6433","pgbouncer","")
    or die "Could not connect to pgbouncer:$!";
    $sth=$dbh_pgb->prepare("SHOW active_sockets");
    $sth->execute;'

from dbdpg.

Further experimentation shows that cb71a77 fixes the entire problem, but 00a7153 breaks it again.

from dbdpg.

Okay, thanks for testing. I made that last patch too quick: will redo with a full pgbouncer verification.

from dbdpg.

Another suggestion: As easy-to-parse SHOW server_version_num is supported since 8.2, you need to use hard-to-parse SHOW server_version only for 8.0 and 8.1 (as older 7.x version are not supported anymore). So what about issuing first easy-to-parse SHOW server_version_num and if it returns error (unsupported command) then fallback to hard-to-parse SHOW server_version? I guess that SHOW server_version can return different version formats also in future and it would only complicate parsing it more and more. On the other hand SHOW server_version_num would work in the same way in future versions too. And if you can correctly handle server error that statement is unsupported, this solution would be more error-prone. And because usage of 8.0 and 8.1 versions would be still lower and lower, that fallback code would be use less and less. So in most cases people would use only SHOW server_version_num.

from dbdpg.

If we're going by supported versions--and that seems like a very reasonable thing to go by--we're going with 9.4 or later. Even if we go five versions before the most recent community supported version, we're still talking about 8.4 right now and 9.0 as of 29 days hence.

from dbdpg.

@pali That's not a bad approach, although we should probably rely on PQserverVersion for the first part rather than making a whole round trip for SHOW, as all of this is a pretty edge case condition. So what you are saying is that we don't search for 'bouncer' in the server_version string, but simply fall back to a second check of server_version_num if the first check fails for any reason? I'm ok with that. It may break in the future when pgbouncer reaches version 8, but that seems a long way off as they don't ever seem to bump major version numbers.

from dbdpg.

I guess that SHOW server_version can return different version formats also in future

I'm not too worried about that. If it does, we can just copy the code from fe-exec.c again

from dbdpg.

@davidfetter Nah, I want DBD::Pg to be as backwards compatible as possible, even for "unsupported" versions. Someday we can get rid of 8.x, but it will need to be for a stronger reason than pgbouncer misbehaving. :)

from dbdpg.

Well, that's not going to work: neither SHOW nor SELECT version() work in pgbouncer's special mode. So we're going back to the old patch. @davidfetter please try out HEAD.

from dbdpg.

It works!

from dbdpg.

Great. Version 3.8.1 has bee released with this fix.

from dbdpg.