Обсуждение: [PATCH] Send numeric version to clients

Hi all

It's recently been observed[1] that the 10.0 version scheme change has
affected PostGIS, which relies on parsing the server version string
and broke when faced with a string like "10.0devel" since it expected
a minor version.

In that thread Tom points out [2] that they should be using
PG_VERSION_NUM from the Makefile, or PG_CATALOG_VERSION from the
headers.

The same sort of problems apply to network clients, but network
clients don't currently get that option because we only send
server_version on the wire in the startup packet. We don't send
server_version_num.

It'll be immediately useful to have this since clients can test for
it, use it if there, and fall back to their old version parsing code
if there's no server_version_num. Version 10.0 is the perfect time to
do this since many clients will need to update their version handling
anyway, and we can just tell them to use server_version_num now.

I'm a PgJDBC committer (albeit largely inactive lately), so I'm
thoroughly familiar with at least one client, and I'd really like to
be able to have PgJDBC able to prefer server_version_num. That's how I
originally started looking at this. Also, clients relying on
server_version makes configure's --with-extra-version nearly unusable
in practice since if you build Pg 9.4.9-mypatch a bunch of clients
fall over, as I discovered when working on BDR.

I'm not convinced by the cost concerns that originally caused it not
to be made GUC_REPORT [3], or at least that they still apply today.
Since that change 10 years ago networks have changed a lot. More
significantly we've since added both IntervalStyle ([4], df7641e2, Ron
Mayer / Tom) and application_name ([5], 59ed94ad, Marko Kreen / Tom)
as GUC_REPORT params.

The startup packet is 331 bytes on my system, with a short
application_name 'psql', short username 'craig', etc. Adding
server_version_num would push it up ~26 bytes to ~357, a 7% increase
on a packet we send once at connection establishment. Given that
network performance is overwhelmingly dominated by round-trip costs
even on fast local networks this is going to be practically
undetectable. A minimal connect-and-disconnect psql session with no
SSL exchanges 14 packets of 1363 bytes (TCP level), of which 670 bytes
are server -> client. So that's a 4% increase on the network size of
the most utterly trivial conversation possible, with zero new packets
and zero new round trips.

Unsurprisingly the pgbench effects are undetectable.

Compare that to the effects of [6] pipelining work on the protocol,
where I got speedups of 300x or more by tackling round trip costs.

Can we please send server_version_num on the wire for 10.0? Patches attached.

(BTW, I noticed while prepping that patch that we have identically
duplicated docs for GUC_REPORT params in protocol.sgml and
libpq.sgml.)

[1] https://www.postgresql.org/message-id/000001d2014c$f84b9190$e8e2b4b0$@pcorp.us
[2] https://www.postgresql.org/message-id/1585.1472410329@sss.pgh.pa.us
[3] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e35ea51
[4] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=df7641e2
[5] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=59ed94ad
[6] https://commitfest.postgresql.org/10/634/

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Craig Ringer <craig@2ndquadrant.com> writes:
> The same sort of problems apply to network clients, but network
> clients don't currently get that option because we only send
> server_version on the wire in the startup packet. We don't send
> server_version_num.

> It'll be immediately useful to have this since clients can test for
> it, use it if there, and fall back to their old version parsing code
> if there's no server_version_num.

I think that this would merely create an attractive nuisance: people
would be very likely to omit the "fallback" code and thereby build
clients that fail for no very good reason on pre-v10 servers.  As a
demonstration that that's not a hypothetical risk, I assert that
that's exactly what you propose telling them to do:

> Version 10.0 is the perfect time to
> do this since many clients will need to update their version handling
> anyway, and we can just tell them to use server_version_num now.

Sure, it'd be great if we'd done it like this to start with.  But that
ship sailed long ago, and redefining how clients ought to determine
server version at this point is just a bad idea.

I'm also fairly dubious that this is a large problem; surely most
C-coded clients use libpq, for instance, and we already solved this
for them in PQserverVersion.  Or if they aren't using PQserverVersion,
why not?
        regards, tom lane

On Mon, Aug 29, 2016 at 7:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Craig Ringer <craig@2ndquadrant.com> writes:
>> The same sort of problems apply to network clients, but network
>> clients don't currently get that option because we only send
>> server_version on the wire in the startup packet. We don't send
>> server_version_num.
>
>> It'll be immediately useful to have this since clients can test for
>> it, use it if there, and fall back to their old version parsing code
>> if there's no server_version_num.
>
> I think that this would merely create an attractive nuisance: people
> would be very likely to omit the "fallback" code and thereby build
> clients that fail for no very good reason on pre-v10 servers.  As a
> demonstration that that's not a hypothetical risk, I assert that
> that's exactly what you propose telling them to do:
>
>> Version 10.0 is the perfect time to
>> do this since many clients will need to update their version handling
>> anyway, and we can just tell them to use server_version_num now.

You know, I've kind of been on Craig's side of this running dispute up
until now, but I have to admit that this seems like an awfully good
argument.  The fact is that nobody's going to be able to rely on
server_version_num until 9.6 is long gone - which doesn't mean late
2021, when official community support will end, but several years
after that, when most everyone has actually stopped using it.  Of
course, by that time, assuming we don't backpedal on our decision to
go with this new versioning scheme, three part version numbers will be
equally dead and gone, and it's actually a lot easier to extract the
major version number from the new format than the old.  For example,
you can just apply atoi() to it:

if (atoi(server_version) < 12)   fprintf(stderr, "server is ancient, some features will not work\n");

That wouldn't be nearly good enough with three part version numbers
because you need the second component, as well.  But all that's going
away now. If you need a port to some other language:

In Perl, you can test int($server_version).
In Javascript, you can test parseInt(server_version).
In Python, it's a bit harder.  But int(re.sub(r'[^\d+]+', '',
server_version)) seems to work.
In Ruby, server_version.to_i seems to do the trick.

Note that these are all one-liners, and I bet the same is true in
mostly other languages.  Even in notoriously verbose languages like
Java or Cobol or ADA it can't be very hard.[1]

If you want the major and minor version numbers, you might need to
write a subroutine for that containing several lines of code ... but
if you're testing for the presence or absence of a feature, that's
irrelevant 99% of the time, and in any event, it's probably 2-3 lines
of code in most.  Note that the C code that implements the version of
PQserverVersion() that handles both old and new style version number
is 33 lines of code, counting variable declarations, comments, and
whitespace.  And approximately half of that is for compatibility with
the old format.

Long story short, I kind of agree that it might have been better to
expose server_version_num rather than server_version in the beginning,
but I'm not sure that it really helps anybody now, especially given
our decision to simplify the version number format going forward.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

[1] I expect someone to argue (at great length?) that Java should not
be categorized as a notoriously verbose language.

On Mon, Aug 29, 2016 at 11:37 AM, Robert Haas <robertmhaas@gmail.com> wrote:

> Note that these are all one-liners, and I bet the same is true in
> mostly other languages.  Even in notoriously verbose languages like
> Java or Cobol or ADA it can't be very hard.

Regarding Java, for anything above the driver itself the
JDBC API says the DatabaseMetaData class must implement these
methods:

int getDatabaseMajorVersion()   Retrieves the major version number of the underlying database.
int getDatabaseMinorVersion()   Retrieves the minor version number of the underlying database.
String getDatabaseProductVersion()   Retrieves the version number of this database product.

That *should* make it just a problem for the driver itself.  That
seems simple enough until you check what those methods have been
returning so far.  A somewhat minimal program to return these
values:

import java.sql.*;
public final class PrintVersion
{   public static void main(String[] args) throws Exception   {       Class.forName("org.postgresql.Driver");
Connectioncon =
 
DriverManager.getConnection("jdbc:postgresql://localhost/test?user=kgrittn");       DatabaseMetaData dbmd =
con.getMetaData();      System.out.println(dbmd.getDatabaseMajorVersion() + "  " +
 
dbmd.getDatabaseMinorVersion());       con.close();   }
}

... outputs this:

9  6

I'm not sure what the right thing to do is here.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner <kgrittn@gmail.com> writes:
> Regarding Java, for anything above the driver itself the
> JDBC API says the DatabaseMetaData class must implement these
> methods:
> ...
> That *should* make it just a problem for the driver itself.  That
> seems simple enough until you check what those methods have been
> returning so far.

Seems like we just make getDatabaseMinorVersion() return zero for
any major >= 10.  That might not have been the best thing to do in
a green field, but given the precedent ...
        regards, tom lane

<p dir="ltr"><p dir="ltr">On 30 Aug 2016 9:07 AM, "Dave Cramer" <<a
href="mailto:pg@fastcrypt.com">pg@fastcrypt.com</a>>wrote:<br /> ><br /> ><br /> > On 29 August 2016 at
15:42,Tom Lane <<a href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>> wrote:<br /> >><br /> >>
KevinGrittner <<a href="mailto:kgrittn@gmail.com">kgrittn@gmail.com</a>> writes:<br /> >> > Regarding
Java,for anything above the driver itself the<br /> >> > JDBC API says the DatabaseMetaData class must
implementthese<br /> >> > methods:<br /> >> > ...<br /> >> > That *should* make it just a
problemfor the driver itself.  That<br /> >> > seems simple enough until you check what those methods have
been<br/> >> > returning so far.<br /> >><br /> >> Seems like we just make
getDatabaseMinorVersion()return zero for<br /> >> any major >= 10.  That might not have been the best thing to
doin<br /> >> a green field, but given the precedent ...<br /> >><br /> >>                        
regards,tom lane<br /> >><br /> >><br /> > seems to me that it should report 10 for the major and
whatevercomes after the . for the minor ?<p dir="ltr">IMO it just needs to be consistent with the numeric version we
reportelsewhere - PG_VERSION_NUM, server_version_num etc.<br /><p dir="ltr">><br /> > Dave Cramer<br /> ><br
/>> <a href="mailto:davec@postgresintl.com">davec@postgresintl.com</a><br /> > <a
href="http://www.postgresintl.com">www.postgresintl.com</a><br/> > 

On 30 August 2016 at 00:37, Robert Haas <robertmhaas@gmail.com> wrote:

> Long story short, I kind of agree that it might have been better to
> expose server_version_num rather than server_version in the beginning,
> but I'm not sure that it really helps anybody now, especially given
> our decision to simplify the version number format going forward.

OK, that's that then. We can fix this properly when the fabled v4
protocol moves into the real world, and keep hacking around it in the
mean time. No point restating my disagreement for the 1000th time,
done is done and better things for us all to spend our time on.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services