Cannot connect to recent versions of pgbouncer

[davidfetter]

The check for versions is now too strict.

$ perl -MDBI -E '$dbh=DBI->connect("dbi:Pg:dbname=pgbouncer;host=/tmp;port=6432","pgbouncer","") or die "Cannot connect to pgbouncer: $!"'
DBI connect('dbname=pgbouncer;host=/tmp;port=6432','pgbouncer',...) failed: Server version 8.0 required at -e line 1.
Cannot connect to pgbouncer:  at -e line 1.

[machack666]

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 f6e60450, 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.

[davidfetter]

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.

[ilmari]

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.

[davidfetter]

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.

[machack666]

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.)

[machack666]

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

[ilmari]

@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.

[machack666]

@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.

[ilmari]

@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?

[machack666]

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.

[turnstep]

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.

[machack666]

The only place it’s changing server_version is when connecting to the “pgbouncer” monitoring “database" itself; it’s not happening when connecting to client-level databases. While I agree it’s stupid it would seem that allowing an environmental override of the detected server_version would be the easiest way forward. I also think that limiting the failure from a connect-time error to a “throw error when we use said functionality” error is probably best practice.

[davidfetter]

On Wed, Jun 26, 2019 at 09:17:03AM -0700, Greg Sabino Mullane wrote:

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.

It's doing this for its own console, which I was connecting to with DBD::Pg, which in turn was to find the row types of the SHOW commands.

Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778

Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate

[turnstep]

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.

[machack666]

Isn’t server_version an int param? Maybe I’m thinking of DBD::Pg’s internal representation of PQServerVersion.

In any case, ugly but I guess it’ll work…

Best,

David

[turnstep]

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

[turnstep]

@davidfetter Please give https://github.com/bucardo/dbdpg/commit/cb71a77ca5bcd18a0c2f628843f8ae98e729ffe0 a whirl.

[pali]

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?

[turnstep]

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.

[pali]

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().

[turnstep]

Yes, good point. Applied a POC in 00a715349ca6298578cc0a072e41dce25a19b865

[davidfetter]

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

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)

[davidfetter]

Interestingly, it doesn't bomb on cb71a77ca5bcd18a0c2f628843f8ae98e729ffe0:

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;'

[davidfetter]

Further experimentation shows that cb71a77ca5bcd18a0c2f628843f8ae98e729ffe0 fixes the entire problem, but 00a715349ca6298578cc0a072e41dce25a19b865 breaks it again.

[turnstep]

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

[pali]

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.

[davidfetter]

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.

[turnstep]

@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.

[turnstep]

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

[turnstep]

@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. :)

[turnstep]

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.

[davidfetter]

It works!

[turnstep]

Great. Version 3.8.1 has bee released with this fix.