Is it time to kill support for very old servers?

On Fri, Oct 7, 2016 at 4:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

pg_dump alleges support for dumping from servers back to 7.0. Would v10
be a good time to remove some of that code? It's getting harder and
harder to even compile those ancient branches, let alone get people to
test against them (cf. 4806f26f9). My initial thought is to cut support
for pre-7.3 or maybe pre-7.4 servers, as that would allow removal of
support for cases where the server lacks schemas or pg_depend, each of
which requires a fair deal of klugery in pg_dump.

+1 for doing it, and also picking a version that's "easily explainable". If
we can say "now we support back to 8.0" that's a lot better than saying 8.1
or 7.4.

In the same line, maybe we should kill libpq's support for V2 protocol
(which would make the cutoff 7.4). And maybe the server's support too,
though that wouldn't save very much code. The argument for cutting this
isn't so much that we would remove lots of code as that we're removing
code that never gets tested, at least not by us.

Which is a pretty strong argument in itself.

One small problem with cutting libpq's V2 support is that the server's
report_fork_failure_to_client() function still sends a V2-style message.
We could change that in HEAD, certainly, but we don't really want modern
libpq unable to parse such a message from an older server. Possibly we
could handle that specific case with a little special-purpose code and
still be able to take out most of fe-protocol2.c.

Thoughts?

I agree we want the new libpq to be able to deal with that one from old
versions, because 9.6 isn't really old yet. We *should* probably change it
in head for the future anyway, but that means we have to keep it around in
libpq for quite a long while anyway.

Unless the special purpose code becomes long and complicated, I think
that's the best way to do it.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Fri, Oct 7, 2016 at 10:06 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

pg_dump alleges support for dumping from servers back to 7.0. Would v10
be a good time to remove some of that code? It's getting harder and
harder to even compile those ancient branches, let alone get people to
test against them (cf. 4806f26f9). My initial thought is to cut support
for pre-7.3 or maybe pre-7.4 servers, as that would allow removal of
support for cases where the server lacks schemas or pg_depend, each of
which requires a fair deal of klugery in pg_dump.

It's been a long time since I've seen any 7.x versions in the wild,
and the pg_dump stuff is a nuisance to maintain. +1 for dropping
support for anything pre-7.4. Anybody who is upgrading from 7.3 to 10
isn't likely to want to do it in one swing anyway. In the real world,
few upgrades span 14 major versions.

In the same line, maybe we should kill libpq's support for V2 protocol
(which would make the cutoff 7.4). And maybe the server's support too,
though that wouldn't save very much code. The argument for cutting this
isn't so much that we would remove lots of code as that we're removing
code that never gets tested, at least not by us.

One small problem with cutting libpq's V2 support is that the server's
report_fork_failure_to_client() function still sends a V2-style message.
We could change that in HEAD, certainly, but we don't really want modern
libpq unable to parse such a message from an older server. Possibly we
could handle that specific case with a little special-purpose code and
still be able to take out most of fe-protocol2.c.

I definitely think we can't remove client support for anything that
modern servers still send, even if we change 10devel so that it no
longer does so. I think we have to at least wait until all versions
that might send a message are EOL before removing client-side support
for it -- and maybe a bit longer than that.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Oct 7, 2016 at 3:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

pg_dump alleges support for dumping from servers back to 7.0. Would v10
be a good time to remove some of that code? It's getting harder and
harder to even compile those ancient branches, let alone get people to
test against them (cf. 4806f26f9). My initial thought is to cut support
for pre-7.3 or maybe pre-7.4 servers, as that would allow removal of
support for cases where the server lacks schemas or pg_depend, each of
which requires a fair deal of klugery in pg_dump.

I might be expected to be the holdout here but it seems sensible to
me. Removing code in pg_dump to deal with lacking schemas and
pg_depend seems like a major simplification.

In the same line, maybe we should kill libpq's support for V2 protocol
(which would make the cutoff 7.4). And maybe the server's support too,
though that wouldn't save very much code. The argument for cutting this
isn't so much that we would remove lots of code as that we're removing
code that never gets tested, at least not by us.

If it's testing we're concerned about IIRC the current servers can be
arm-twisted into speaking V2 protocol. So it should be possible to
test both modern servers and modern pg_dump using V2 protocol with a
simple tweak.

Somehow removing the whole protocol support seems a bit different to
me than removing pg_dump logic. For one it's nice to be able to run a
modern psql against old servers so you can run a benchmark script. For
another there may be binary-only applications or drivers out there
that are using V2 for whatever reason.

--
greg

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Greg Stark <stark@mit.edu> writes:

On Fri, Oct 7, 2016 at 3:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

In the same line, maybe we should kill libpq's support for V2 protocol
(which would make the cutoff 7.4). And maybe the server's support too,
though that wouldn't save very much code. The argument for cutting this
isn't so much that we would remove lots of code as that we're removing
code that never gets tested, at least not by us.

Somehow removing the whole protocol support seems a bit different to
me than removing pg_dump logic. For one it's nice to be able to run a
modern psql against old servers so you can run a benchmark script.

Yeah, but surely pre-7.4 servers are no longer of much interest for that;
or if you want historical results you should also use a matching libpq.

For another there may be binary-only applications or drivers out there
that are using V2 for whatever reason.

The problem with letting it just sit there is that we're not, in fact,
testing it. If we take the above argument seriously then we should
provide some way to configure libpq to prefer V2 and run regression
tests in that mode. Otherwise, if/when we break it, we'll never know it
till we get field reports.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Oct 7, 2016 at 11:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Greg Stark <stark@mit.edu> writes:

On Fri, Oct 7, 2016 at 3:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

In the same line, maybe we should kill libpq's support for V2 protocol
(which would make the cutoff 7.4). And maybe the server's support too,
though that wouldn't save very much code. The argument for cutting this
isn't so much that we would remove lots of code as that we're removing
code that never gets tested, at least not by us.

Somehow removing the whole protocol support seems a bit different to
me than removing pg_dump logic. For one it's nice to be able to run a
modern psql against old servers so you can run a benchmark script.

Yeah, but surely pre-7.4 servers are no longer of much interest for that;
or if you want historical results you should also use a matching libpq.

For another there may be binary-only applications or drivers out there
that are using V2 for whatever reason.

The problem with letting it just sit there is that we're not, in fact,
testing it. If we take the above argument seriously then we should
provide some way to configure libpq to prefer V2 and run regression
tests in that mode. Otherwise, if/when we break it, we'll never know it
till we get field reports.

I agree with that. I think it would be fine to keep V2 support if
somebody wants to do the work to let us have adequate test coverage,
but if nobody volunteers I think we might as well rip it out. I don't
particularly enjoy committing things only to be told that they've
broken something I can't test without unreasonable effort.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Oct 7, 2016 at 11:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Greg Stark <stark@mit.edu> writes:

For another there may be binary-only applications or drivers out there
that are using V2 for whatever reason.

The problem with letting it just sit there is that we're not, in fact,
testing it. If we take the above argument seriously then we should
provide some way to configure libpq to prefer V2 and run regression
tests in that mode. Otherwise, if/when we break it, we'll never know it
till we get field reports.

I agree with that. I think it would be fine to keep V2 support if
somebody wants to do the work to let us have adequate test coverage,
but if nobody volunteers I think we might as well rip it out. I don't
particularly enjoy committing things only to be told that they've
broken something I can't test without unreasonable effort.

When I wrote the above I was thinking of an essentially user-facing
libpq feature, similar to the one JDBC has, to force use of V2 protocol.
But actually, for testing purposes, I don't think that's what we want.
Any such feature would fail to exercise libpq's logic for falling back
from V3 to V2 when it connects to an old server, which is surely something
we'd like to test without actually having a pre-7.4 server at hand.

So what I'm thinking is it'd be sufficient to do something like
this in pqcomm.h:

+#ifndef FORCE_OLD_PROTOCOL
 #define PG_PROTOCOL_LATEST		PG_PROTOCOL(3,0)
+#else /* make like a pre-7.4 server for testing purposes */
+#define PG_PROTOCOL_LATEST		PG_PROTOCOL(2,0)
+#endif

which would cause the server to reject 3.0 requests just as if it were
ancient. Then we could test with that #define, maybe have a buildfarm
critter doing it. (This might break pqmq.c though, so we might need
to work slightly harder than this.)

Also, I realized while perusing this that the server still has support
for protocol 1.0 (!). That's *definitely* dead code. There's not much
of it, but still, I'd rather rip it out than continue to pretend it's
supported.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

This thread gets me thinking about the definition of "support." While
support in practice seems to primarily relate to fixes/updates to the
supported version itself it could just as well apply to interoperability
support by newer versions.

Given that the standard PostgreSQL upgrade process involves upgrading
clients first and using pg_dump from the newer version, it is reasonable to
assume that the clients/utilities for a given version would support
interacting with any prior version that was not EOL at the time the new
major version is released.

In other words, 9.6 was released last month, the same month that 9.1 was
EOL, so 9.6 clients should work with 9.1 through 9.6 servers but from my
perspective there is no need to *guarantee* that 10 would do so. The
standard caveats apply. A new version *might* work for an unsupported older
version but no assurance is offered.

This is effectively a 5-year upgrade "grace period" *after* the EOL date of
a given version which seems plenty generous.

Defining the term of backward compatibility support might be useful in the
future when these types of questions arise.

Cheers,
Steve

On Fri, Oct 7, 2016 at 9:06 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

On 10/7/16 1:08 PM, Steve Crawford wrote:

This is effectively a 5-year upgrade "grace period" *after* the EOL date
of a given version which seems plenty generous.

IMHO we need to be careful here. It's not at all unusual to see servers
running versions that are *far* older than that. It's certainly
understandable that we're not actively supporting those versions any
more, but we also don't want people to effectively be stranded on them
because they can't even get the data out and back into a newer version.
So I think pg_dump at least should try to support as far back as we can
without jumping to lots of hoops in code. I think moving the limit to
8.0 is fine, but I'm not so comfortable with making that limit 9.1.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10 October 2016 at 10:45, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

On 10/7/16 1:08 PM, Steve Crawford wrote:

This is effectively a 5-year upgrade "grace period" *after* the EOL date
of a given version which seems plenty generous.

IMHO we need to be careful here. It's not at all unusual to see servers
running versions that are *far* older than that. It's certainly
understandable that we're not actively supporting those versions any more,
but we also don't want people to effectively be stranded on them because
they can't even get the data out and back into a newer version. So I think
pg_dump at least should try to support as far back as we can without jumping
to lots of hoops in code. I think moving the limit to 8.0 is fine, but I'm
not so comfortable with making that limit 9.1.

The oldest I deal with is 8.2, and that's enough of an aberration that
expecting them to go via 9.6 isn't wholly unreasonable. I agree 9.0 is
way too far.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Oct 7, 2016 at 11:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The problem with letting it just sit there is that we're not, in fact,
testing it. If we take the above argument seriously then we should
provide some way to configure libpq to prefer V2 and run regression
tests in that mode. Otherwise, if/when we break it, we'll never know it
till we get field reports.

I agree with that. I think it would be fine to keep V2 support if
somebody wants to do the work to let us have adequate test coverage,
but if nobody volunteers I think we might as well rip it out. I don't
particularly enjoy committing things only to be told that they've
broken something I can't test without unreasonable effort.

I experimented with this and found out that a majority of the regression
tests fall over, because the error messages come out formatted differently
when using V2. You don't get separate DETAIL or HINT fields, nor cursor
positions, etc. So there's boatloads of silly diffs like this:

***************
*** 69,75 ****
  INSERT INTO testschema.atable VALUES(3);      -- ok
  INSERT INTO testschema.atable VALUES(1);      -- fail (checks index)
  ERROR:  duplicate key value violates unique constraint "anindex"
- DETAIL:  Key (column1)=(1) already exists.
  SELECT COUNT(*) FROM testschema.atable;               -- checks heap
   count 
  -------
--- 69,74 ----

Some of the tests have harder-to-ignore failures because protocol V2
had no way to recover from a failure during COPY IN, short of aborting
the connection:

--- 34,42 ----
  -- missing data: should fail
  COPY x from stdin;
  ERROR:  invalid input syntax for integer: ""
! FATAL:  terminating connection because protocol synchronization was lost
  COPY x from stdin;
! server closed the connection unexpectedly
! 	This probably means the server terminated abnormally
! 	before or while processing the request.
! connection to server was lost

At this point I'm feeling pretty discouraged about testability of V2.
I can't see us maintaining a variant regression test suite that would
accommodate these issues. So the easy idea of "build with some extra
#define and then run the usual regression tests" is out.

Now, we hardly need the entire regression suite to provide adequate
coverage of V2 protocol. You could imagine a small dedicated test
that just checks basic commands, errors, notices, COPY. The problem
is that we'd need infrastructure in either the backend or libpq to
dynamically force use of V2 for that test, and that's work that I for
one don't really care to put in.

Not sure where to go from here, but the idea of dropping V2 support
is seeming attractive again. Or we could just continue the policy
of benign neglect.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10/11/2016 08:23 PM, Tom Lane wrote:

Not sure where to go from here, but the idea of dropping V2 support
is seeming attractive again. Or we could just continue the policy
of benign neglect.

Let's drop it.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I wrote:

pg_dump alleges support for dumping from servers back to 7.0. Would v10
be a good time to remove some of that code? It's getting harder and
harder to even compile those ancient branches, let alone get people to
test against them (cf. 4806f26f9). My initial thought is to cut support
for pre-7.3 or maybe pre-7.4 servers, as that would allow removal of
support for cases where the server lacks schemas or pg_depend, each of
which requires a fair deal of klugery in pg_dump.

Per subsequent discussion, we should use a round number as the version
cutoff, so attached is a patch that removes pg_dump/pg_dumpall support
for dumping from servers before 8.0. This eliminates circa 1500 lines
of code, or about 4% of the existing code in src/bin/pg_dump/. So it's
not a huge savings, but it eliminates a lot of hard-to-test cases.

I (think I) did not do anything that would damage pg_restore's ability to
read dump archives produced by pre-8.0 versions. I'm pretty hesitant to
remove that code since (a) there's not all that much of it, and (b) it's
easy to imagine scenarios where an old backup dump is all someone's got.
However, that's another set of cases that's darn hard to test, so it's
somewhat questionable whether we may have broken it already.

Comments, objections?

regards, tom lane

On Tue, Oct 11, 2016 at 11:34 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 10/11/2016 08:23 PM, Tom Lane wrote:

Not sure where to go from here, but the idea of dropping V2 support
is seeming attractive again. Or we could just continue the policy
of benign neglect.

Let's drop it.

I agree. The evidence in Tom's latest email suggests that getting
this to a point where it can be tested will be more work than
anybody's likely to want to put in, and the return on investment
doesn't seem likely to be good.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

Re-upping this topic.

On 2016-10-07 10:06:07 -0400, Tom Lane wrote:

In the same line, maybe we should kill libpq's support for V2 protocol
(which would make the cutoff 7.4). And maybe the server's support too,
though that wouldn't save very much code. The argument for cutting this
isn't so much that we would remove lots of code as that we're removing
code that never gets tested, at least not by us.

I'd like to do this in the not too far away future for at least the
backend. There's enough not particularly pretty code to deal with v2
that that'd be worthwhile.

One small problem with cutting libpq's V2 support is that the server's
report_fork_failure_to_client() function still sends a V2-style message.
We could change that in HEAD, certainly, but we don't really want modern
libpq unable to parse such a message from an older server. Possibly we
could handle that specific case with a little special-purpose code and
still be able to take out most of fe-protocol2.c.

We should really fix that so it reports the error as a v3 message,
independent of ripping out libpq-fe support for v2.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund <andres@anarazel.de> writes:

Re-upping this topic.

On 2016-10-07 10:06:07 -0400, Tom Lane wrote:

In the same line, maybe we should kill libpq's support for V2 protocol
(which would make the cutoff 7.4). And maybe the server's support too,
though that wouldn't save very much code. The argument for cutting this
isn't so much that we would remove lots of code as that we're removing
code that never gets tested, at least not by us.

I'd like to do this in the not too far away future for at least the
backend. There's enough not particularly pretty code to deal with v2
that that'd be worthwhile.

Hm, I don't recall that there's very much on the server side that could be
saved --- what's incurring your ire, exactly?

One small problem with cutting libpq's V2 support is that the server's
report_fork_failure_to_client() function still sends a V2-style message.

We should really fix that so it reports the error as a v3 message,
independent of ripping out libpq-fe support for v2.

It might be reasonable to do that, but libpq would have to be prepared
for the other case for many years to come :-(

The real problem in this area, to my mind, is that we're not testing that
code --- either end of it --- in any systematic way. If it's broken it
could take us quite a while to notice.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 2017-09-13 23:39:21 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

Re-upping this topic.

On 2016-10-07 10:06:07 -0400, Tom Lane wrote:

In the same line, maybe we should kill libpq's support for V2 protocol
(which would make the cutoff 7.4). And maybe the server's support too,
though that wouldn't save very much code. The argument for cutting this
isn't so much that we would remove lots of code as that we're removing
code that never gets tested, at least not by us.

I'd like to do this in the not too far away future for at least the
backend. There's enough not particularly pretty code to deal with v2
that that'd be worthwhile.

Hm, I don't recall that there's very much on the server side that could be
saved --- what's incurring your ire, exactly?

In this specific instance it's SendRowDescriptionMessage() for a queries
returning a lot of columns that execute fast. The additional branches
due to the if (proto >= 3) conditionals are noticeable. It's easy enough
to fix by having two for() for the two cases. The added code is annoying
but bearable, what actually concerns me is that it's really hard to test
the v2 support.

One small problem with cutting libpq's V2 support is that the server's
report_fork_failure_to_client() function still sends a V2-style message.

We should really fix that so it reports the error as a v3 message,
independent of ripping out libpq-fe support for v2.

It might be reasonable to do that, but libpq would have to be prepared
for the other case for many years to come :-(

Right. But there seems pretty much no way to get around that. At least
none that I can see? It might be worthwhile and more testable to add a
special case V2 handling for the oom-fork case, but still.

The real problem in this area, to my mind, is that we're not testing that
code --- either end of it --- in any systematic way. If it's broken it
could take us quite a while to notice.

Yea, that concerns me a lot too (see above).

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Sep 13, 2017 at 11:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

One small problem with cutting libpq's V2 support is that the server's
report_fork_failure_to_client() function still sends a V2-style message.

We should really fix that so it reports the error as a v3 message,
independent of ripping out libpq-fe support for v2.

It might be reasonable to do that, but libpq would have to be prepared
for the other case for many years to come :-(

Well, let's get that much done anyway. I'm not 100% sure whether the
time has come to kill v2 with fire, but doing the things that have
been done first has got to be a good idea either way.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017-09-13 23:39:21 -0400, Tom Lane wrote:

The real problem in this area, to my mind, is that we're not testing that
code --- either end of it --- in any systematic way. If it's broken it
could take us quite a while to notice.

Independent of the thrust of my question - why aren't we adding a
'force-v2' option to libpq? A test that basically does something like
postgres[22923][1]=# \setenv PGFORCEV2 1
postgres[22923][1]=# \c
You are now connected to database "postgres" as user "andres".
postgres[22924][1]=>
seems easy enough to add, in fact I tested the above.

And the protocol coverage of the v2 protocol seems small enough that a
single not too large file ought to cover most if it quite easily.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Sep 18, 2017 at 6:53 PM, Andres Freund <andres@anarazel.de> wrote:

On 2017-09-13 23:39:21 -0400, Tom Lane wrote:

The real problem in this area, to my mind, is that we're not testing that
code --- either end of it --- in any systematic way. If it's broken it
could take us quite a while to notice.

Independent of the thrust of my question - why aren't we adding a
'force-v2' option to libpq? A test that basically does something like
postgres[22923][1]=# \setenv PGFORCEV2 1
postgres[22923][1]=# \c
You are now connected to database "postgres" as user "andres".
postgres[22924][1]=>
seems easy enough to add, in fact I tested the above.

And the protocol coverage of the v2 protocol seems small enough that a
single not too large file ought to cover most if it quite easily.

It seems to me that you are looking more for a connection parameter here.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On September 18, 2017 3:50:17 AM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:

On Mon, Sep 18, 2017 at 6:53 PM, Andres Freund <andres@anarazel.de>
wrote:

On 2017-09-13 23:39:21 -0400, Tom Lane wrote:

The real problem in this area, to my mind, is that we're not testing

that

code --- either end of it --- in any systematic way. If it's broken

it

could take us quite a while to notice.

Independent of the thrust of my question - why aren't we adding a
'force-v2' option to libpq? A test that basically does something

like

postgres[22923][1]=# \setenv PGFORCEV2 1
postgres[22923][1]=# \c
You are now connected to database "postgres" as user "andres".
postgres[22924][1]=>
seems easy enough to add, in fact I tested the above.

And the protocol coverage of the v2 protocol seems small enough that

a

single not too large file ought to cover most if it quite easily.

It seems to me that you are looking more for a connection parameter
here.

I'm not seeing a meaningful distinction here? Env vars and connection parameters are handled using the same framework in libpq. And using the env var in the test would be better, because you'd only set one value - hard to do within our non TAP tests (i.e. in an existing psql, started by pg regress) otherwise.

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Sep 18, 2017 at 7:54 PM, Andres Freund <andres@anarazel.de> wrote:

It seems to me that you are looking more for a connection parameter
here.

I'm not seeing a meaningful distinction here? Env vars and connection parameters are handled using the same framework in libpq. And using the env var in the test would be better, because you'd only set one value - hard to do within our non TAP tests (i.e. in an existing psql, started by pg regress) otherwise.

Or both? I don't really understand why an environment variable is
better than a connection string. For the TAP tests, you could just set
the base of the connection string once and you are done as well. See
the SSL tests for example.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On September 18, 2017 4:08:21 AM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:

On Mon, Sep 18, 2017 at 7:54 PM, Andres Freund <andres@anarazel.de>
wrote:

It seems to me that you are looking more for a connection parameter
here.

I'm not seeing a meaningful distinction here? Env vars and connection

parameters are handled using the same framework in libpq. And using
the env var in the test would be better, because you'd only set one
value - hard to do within our non TAP tests (i.e. in an existing psql,
started by pg regress) otherwise.

Or both? I don't really understand why an environment variable is
better than a connection string. For the TAP tests, you could just set
the base of the connection string once and you are done as well. See
the SSL tests for example.

Did you read what I wrote?
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Sep 18, 2017 at 8:09 PM, Andres Freund <andres@anarazel.de> wrote:

On September 18, 2017 4:08:21 AM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:

On Mon, Sep 18, 2017 at 7:54 PM, Andres Freund <andres@anarazel.de>
wrote:

It seems to me that you are looking more for a connection parameter
here.

I'm not seeing a meaningful distinction here? Env vars and connection

parameters are handled using the same framework in libpq. And using
the env var in the test would be better, because you'd only set one
value - hard to do within our non TAP tests (i.e. in an existing psql,
started by pg regress) otherwise.

Or both? I don't really understand why an environment variable is
better than a connection string. For the TAP tests, you could just set
the base of the connection string once and you are done as well. See
the SSL tests for example.

Did you read what I wrote?

Sorry, I missed the "non" with "TAP" tests. Having a connection
parameter would still be low-cost in maintenance, so if you add that
at the same time I won't complain.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On September 18, 2017 4:15:31 AM PDT, Michael Paquier <michael.paquier@gmail.com> wrote:

On Mon, Sep 18, 2017 at 8:09 PM, Andres Freund <andres@anarazel.de>
wrote:

On September 18, 2017 4:08:21 AM PDT, Michael Paquier

<michael.paquier@gmail.com> wrote:

On Mon, Sep 18, 2017 at 7:54 PM, Andres Freund <andres@anarazel.de>
wrote:

It seems to me that you are looking more for a connection parameter
here.

I'm not seeing a meaningful distinction here? Env vars and

connection

parameters are handled using the same framework in libpq. And using
the env var in the test would be better, because you'd only set one
value - hard to do within our non TAP tests (i.e. in an existing

psql,

started by pg regress) otherwise.

Or both? I don't really understand why an environment variable is
better than a connection string. For the TAP tests, you could just

set

the base of the connection string once and you are done as well. See
the SSL tests for example.

Did you read what I wrote?

Sorry, I missed the "non" with "TAP" tests. Having a connection
parameter would still be low-cost in maintenance, so if you add that
at the same time I won't complain.

Private:

And now you missed the "same infrastructure" part.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Sep 18, 2017 at 8:17 PM, Andres Freund <andres@anarazel.de> wrote:

And now you missed the "same infrastructure" part.

I am sort of aware of that per the list of parameters at the top
fe-connect.c, thanks for the reminer :)
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Sep 18, 2017 at 7:17 AM, Andres Freund <andres@anarazel.de> wrote:

Private:

Not so much.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 09/18/2017 04:54 AM, Robert Haas wrote:

On Mon, Sep 18, 2017 at 7:17 AM, Andres Freund <andres@anarazel.de> wrote:

Private:

Not so much.

Well, as much as the Internet is actually private:

https://ilccyberreport.files.wordpress.com/2013/06/nsa11.jpg

JD

;)

--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc

PostgreSQL Centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://pgconf.us
***** Unless otherwise stated, opinions are my own. *****

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017-09-18 02:53:03 -0700, Andres Freund wrote:

On 2017-09-13 23:39:21 -0400, Tom Lane wrote:

The real problem in this area, to my mind, is that we're not testing that
code --- either end of it --- in any systematic way. If it's broken it
could take us quite a while to notice.

Independent of the thrust of my question - why aren't we adding a
'force-v2' option to libpq? A test that basically does something like
postgres[22923][1]=# \setenv PGFORCEV2 1
postgres[22923][1]=# \c
You are now connected to database "postgres" as user "andres".
postgres[22924][1]=>
seems easy enough to add, in fact I tested the above.

And the protocol coverage of the v2 protocol seems small enough that a
single not too large file ought to cover most if it quite easily.

Here's what I roughly was thinking of. I don't quite like the name, and
the way the version is specified for libpq (basically just the "raw"
integer). Not sure if others have an opinion on that. I personally
would lean towards not documenting this option...

There's a few things that I couldn't find easy ways to test:
- the v2 specific binary protocol - I don't quite see how we could test
that without writing C
- version error checks - pg_regress/psql errors out in non-interactive
mode if a connection fails to be established. This we could verify
with a s simple tap test.

Coverage of the relevant files is a good bit higher afterwards. Although
our libpq coverage is generally pretty damn awful.

Regards,

Andres

On 2017-09-20 01:32:36 -0700, Andres Freund wrote:

On 2017-09-18 02:53:03 -0700, Andres Freund wrote:

On 2017-09-13 23:39:21 -0400, Tom Lane wrote:

The real problem in this area, to my mind, is that we're not testing that
code --- either end of it --- in any systematic way. If it's broken it
could take us quite a while to notice.

Independent of the thrust of my question - why aren't we adding a
'force-v2' option to libpq? A test that basically does something like
postgres[22923][1]=# \setenv PGFORCEV2 1
postgres[22923][1]=# \c
You are now connected to database "postgres" as user "andres".
postgres[22924][1]=>
seems easy enough to add, in fact I tested the above.

And the protocol coverage of the v2 protocol seems small enough that a
single not too large file ought to cover most if it quite easily.

Here's what I roughly was thinking of. I don't quite like the name, and
the way the version is specified for libpq (basically just the "raw"
integer). Not sure if others have an opinion on that. I personally
would lean towards not documenting this option...

There's a few things that I couldn't find easy ways to test:
- the v2 specific binary protocol - I don't quite see how we could test
that without writing C
- version error checks - pg_regress/psql errors out in non-interactive
mode if a connection fails to be established. This we could verify
with a s simple tap test.

Coverage of the relevant files is a good bit higher afterwards. Although
our libpq coverage is generally pretty damn awful.

Any opinions on this? Obviously this needs some cleanup, but I'd like to
know whether we've concensus on adding a connection option for this goal
before investing more time into this.

A nearby thread [1]/messages/by-id/20170914063418.sckdzgjfrsbekae4@alap3.anarazel.de whacks around some the v2 code, which triggered me
to look into this. I obviously can just use thiese patches to test those
patches during development, but it seems better to keep coverage.

Thanks,

Andres

[1]: /messages/by-id/20170914063418.sckdzgjfrsbekae4@alap3.anarazel.de

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Oct 11, 2017 at 9:39 AM, Andres Freund <andres@anarazel.de> wrote:

On 2017-09-20 01:32:36 -0700, Andres Freund wrote:

Coverage of the relevant files is a good bit higher afterwards. Although
our libpq coverage is generally pretty damn awful.

Any opinions on this? Obviously this needs some cleanup, but I'd like to
know whether we've concensus on adding a connection option for this goal
before investing more time into this.

A nearby thread [1] whacks around some the v2 code, which triggered me
to look into this. I obviously can just use thiese patches to test those
patches during development, but it seems better to keep coverage.

FWIW, I think that moving forward with such a possibility is a good
thing, including having a connection parameter. This would pay in the
long term if a new protocol version is added. 0001 should document the
new parameter.

+   if (conn->forced_protocol_version != NULL)
+   {
+       conn->pversion = atoi(conn->forced_protocol_version);
+   }
This should check for strlen > 0 as well.
-- 
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 2017-10-11 10:09:34 +0900, Michael Paquier wrote:

On Wed, Oct 11, 2017 at 9:39 AM, Andres Freund <andres@anarazel.de> wrote:

On 2017-09-20 01:32:36 -0700, Andres Freund wrote:

Coverage of the relevant files is a good bit higher afterwards. Although
our libpq coverage is generally pretty damn awful.

Any opinions on this? Obviously this needs some cleanup, but I'd like to
know whether we've concensus on adding a connection option for this goal
before investing more time into this.

A nearby thread [1] whacks around some the v2 code, which triggered me
to look into this. I obviously can just use thiese patches to test those
patches during development, but it seems better to keep coverage.

FWIW, I think that moving forward with such a possibility is a good
thing, including having a connection parameter. This would pay in the
long term if a new protocol version is added.

0001 should document the new parameter.

I'm actually inclined not to, and keep this as a undocumented debugging
option. Limiting the use of this option to people willing to read the
code seems like a good idea to me.

+   if (conn->forced_protocol_version != NULL)
+   {
+       conn->pversion = atoi(conn->forced_protocol_version);
+   }
This should check for strlen > 0 as well.

Why? Note that we don't do elsehwere in fe-connect.c.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Oct 11, 2017 at 10:14 AM, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2017-10-11 10:09:34 +0900, Michael Paquier wrote:

On Wed, Oct 11, 2017 at 9:39 AM, Andres Freund <andres@anarazel.de> wrote:

On 2017-09-20 01:32:36 -0700, Andres Freund wrote:

Coverage of the relevant files is a good bit higher afterwards. Although
our libpq coverage is generally pretty damn awful.

Any opinions on this? Obviously this needs some cleanup, but I'd like to
know whether we've concensus on adding a connection option for this goal
before investing more time into this.

A nearby thread [1] whacks around some the v2 code, which triggered me
to look into this. I obviously can just use thiese patches to test those
patches during development, but it seems better to keep coverage.

FWIW, I think that moving forward with such a possibility is a good
thing, including having a connection parameter. This would pay in the
long term if a new protocol version is added.

0001 should document the new parameter.

I'm actually inclined not to, and keep this as a undocumented debugging
option. Limiting the use of this option to people willing to read the
code seems like a good idea to me.

It seems important to me to document things present. There is a
section in the docs listing developer-only parameters for runtime
configuration, why not separating "Parameter Key Words" into two
sections then? One for the main parameters and one for developer-only
parameters.

+   if (conn->forced_protocol_version != NULL)
+   {
+       conn->pversion = atoi(conn->forced_protocol_version);
+   }
This should check for strlen > 0 as well.

Why? Note that we don't do elsehwere in fe-connect.c.

Because it seems to me that the default value of the parameter should
be an empty string instead of D. Feels more consistent with the
others.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 2017-10-11 10:40:11 +0900, Michael Paquier wrote:

+   if (conn->forced_protocol_version != NULL)
+   {
+       conn->pversion = atoi(conn->forced_protocol_version);
+   }
This should check for strlen > 0 as well.

Why? Note that we don't do elsehwere in fe-connect.c.

Because it seems to me that the default value of the parameter should
be an empty string instead of D. Feels more consistent with the
others.

I'm not following. The "D" is in the 'dispchar' field, not the value
field, no? The default value is NULL?

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Oct 11, 2017 at 10:46 AM, Andres Freund <andres@anarazel.de> wrote:

I'm not following. The "D" is in the 'dispchar' field, not the value
field, no? The default value is NULL?

Oops, yes. I misread the code. Other debug options are not documented,
so fine for me to not provide any documentation then.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Oct 10, 2017 at 9:14 PM, Andres Freund <andres@anarazel.de> wrote:

I'm actually inclined not to, and keep this as a undocumented debugging
option. Limiting the use of this option to people willing to read the
code seems like a good idea to me.

-1. I use the documentation to find things, even though I am a
developer, and I don't think hiding things from users is a good idea
anyway. We can say that we recommend against using the option and
that it's just for testing, but it should be documented anyway.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Oct 11, 2017 at 10:28 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Oct 10, 2017 at 9:14 PM, Andres Freund <andres@anarazel.de> wrote:

I'm actually inclined not to, and keep this as a undocumented debugging
option. Limiting the use of this option to people willing to read the
code seems like a good idea to me.

-1. I use the documentation to find things, even though I am a
developer, and I don't think hiding things from users is a good idea
anyway. We can say that we recommend against using the option and
that it's just for testing, but it should be documented anyway.

You would be disappointed with the current state of things then.
fe-connect.c lists a couple of debug options:
- authtype, which is not used anywhere, still kept to not reject
connection strings using it. I can get that this is not documented.
- tty, same argument as authtype.
- replication, which is still available, and that I have for example
used a couple of times for debugging the replication parser (psql can
work directly with some replication commands as you know). But this is
not documented.

FWIW, I would like to see things properly documented as well. And
please note that I am fine to write such a patch as well.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 9/20/17 04:32, Andres Freund wrote:

Here's what I roughly was thinking of. I don't quite like the name, and
the way the version is specified for libpq (basically just the "raw"
integer).

"forced_protocol_version" reads wrong to me. I think
"force_protocol_version" might be better. Other than that, no issues
with this concept.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017-10-16 16:59:59 -0400, Peter Eisentraut wrote:

On 9/20/17 04:32, Andres Freund wrote:

Here's what I roughly was thinking of. I don't quite like the name, and
the way the version is specified for libpq (basically just the "raw"
integer).

"forced_protocol_version" reads wrong to me. I think
"force_protocol_version" might be better. Other than that, no issues
with this concept.

Yea, I agree. I've read through the patch since, and it struck me as
odd. Not sure how I came up with it...

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Oct 16, 2017 at 10:08 PM, Andres Freund <andres@anarazel.de> wrote:

On 2017-10-16 16:59:59 -0400, Peter Eisentraut wrote:

On 9/20/17 04:32, Andres Freund wrote:

Here's what I roughly was thinking of. I don't quite like the name, and
the way the version is specified for libpq (basically just the "raw"
integer).

"forced_protocol_version" reads wrong to me. I think
"force_protocol_version" might be better. Other than that, no issues
with this concept.

Yea, I agree. I've read through the patch since, and it struck me as
odd. Not sure how I came up with it...

Andres, could you update the patch?
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Nov 3, 2017 at 9:30 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Mon, Oct 16, 2017 at 10:08 PM, Andres Freund <andres@anarazel.de> wrote:

On 2017-10-16 16:59:59 -0400, Peter Eisentraut wrote:

On 9/20/17 04:32, Andres Freund wrote:

Here's what I roughly was thinking of. I don't quite like the name, and
the way the version is specified for libpq (basically just the "raw"
integer).

"forced_protocol_version" reads wrong to me. I think
"force_protocol_version" might be better. Other than that, no issues
with this concept.

Yea, I agree. I've read through the patch since, and it struck me as
odd. Not sure how I came up with it...

Andres, could you update the patch?

Seeing no activity for three weeks and as we are close to the end of
the CF, I am marking this one as returned with feedback.
--
Michael