Removing support for COPY FROM STDIN in protocol version 2

Heikki Linnakangas <hlinnaka@iki.fi> writes:

I propose that we remove server support for COPY FROM STDIN with
protocol version 2, per attached patch. Even if we could still support
it, it would be a very rarely used and tested codepath, prone to bugs.
Perhaps we could remove support for the old protocol altogether, but I'm
not proposing that we go that far just yet.

I'm not really on board with half-baked removal of protocol 2.
If we're going to kill it we should just kill it altogether.
(The argument that it's untested surely applies to the rest
of the P2 code as well.)

I have a vague recollection that JDBC users still like to use
protocol 2 for some reason --- is that out of date?

regards, tom lane

On 2021-Feb-03, Tom Lane wrote:

I have a vague recollection that JDBC users still like to use
protocol 2 for some reason --- is that out of date?

2016:

commit c3d8571e53cc5b702dae2f832b02c872ad44c3b7
Author: Vladimir Sitnikov <sitnikov.vladimir@gmail.com>
AuthorDate: Sat Aug 6 12:22:17 2016 +0300
CommitDate: Sat Aug 13 11:27:16 2016 +0300

fix: support cases when user-provided queries have 'returning'

This change includes: drop v2 protocol support, and query parsing refactoring.
Currently query parse cache is still per-connection, however "returningColumNames"
are part of cache key, thus the parse cache can be made global.

This fixes #488 (see org.postgresql.test.jdbc3.GeneratedKeysTest)

This commit does remove all files in
pgjdbc/src/main/java/org/postgresql/core/v2/, leaving only "v3/".

--
�lvaro Herrera Valdivia, Chile
"Those who use electric razors are infidels destined to burn in hell while
we drink from rivers of beer, download free vids and mingle with naked
well shaved babes." (http://slashdot.org/comments.pl?sid=44793&amp;cid=4647152)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2021-Feb-03, Tom Lane wrote:

I have a vague recollection that JDBC users still like to use
protocol 2 for some reason --- is that out of date?

[ yes, since 2016 ]

Then let's kill it dead, server and libpq both.

regards, tom lane

On 03/02/2021 18:29, Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2021-Feb-03, Tom Lane wrote:

I have a vague recollection that JDBC users still like to use
protocol 2 for some reason --- is that out of date?

[ yes, since 2016 ]

Then let's kill it dead, server and libpq both.

Ok, works for me. I'll prepare a larger patch to do that.

Since we're on a removal-spree, it'd also be nice to get rid of the
"fast-path" function call interface, PQfn(). However, libpq is using it
internally in the lo_*() functions, so if we remove it from the server,
lo_*() will stop working with old libpq versions. It would be good to
change those functions now to use PQexecParams() instead, so that we
could remove the fast-path server support in the future.

- Heikki

Heikki Linnakangas <hlinnaka@iki.fi> writes:

Since we're on a removal-spree, it'd also be nice to get rid of the
"fast-path" function call interface, PQfn(). However, libpq is using it
internally in the lo_*() functions, so if we remove it from the server,
lo_*() will stop working with old libpq versions. It would be good to
change those functions now to use PQexecParams() instead, so that we
could remove the fast-path server support in the future.

I'm disinclined to touch that. It is considered part of protocol v3,
and there is no very good reason to suppose that nothing but libpq
is using it. Besides, what would it really save? fastpath.c has
not been a source of maintenance problems.

regards, tom lane

On Wed, Feb 03, 2021 at 11:29:37AM -0500, Tom Lane wrote:

Then let's kill it dead, server and libpq both.

Yeah.
--
Michael

On 04/02/2021 08:54, Michael Paquier wrote:

On Wed, Feb 03, 2021 at 11:29:37AM -0500, Tom Lane wrote:

Then let's kill it dead, server and libpq both.

Yeah.

Ok, here we go.

One interesting thing I noticed while doing this:

Up until now, we always used the old protocol for errors that happened
early in backend startup, before we processed the client's protocol
version and set the FrontendProtocol variable. I'm sure that made sense
when V3 was introduced, but it was a surprise to me, and I didn't find
that documented anywhere. I changed it so that we use V3 errors, if
FrontendProtocol is not yet set.

However, I kept rudimentary support for sending errors in protocol
version 2. This way, if a client tries to connect with an old client, we
still send the "unsupported frontend protocol" error in the old format.
Likewise, I kept the code in libpq to understand v2 ErrorResponse
messages during authentication.

- Heikki

On 2021-Feb-04, Heikki Linnakangas wrote:

On 04/02/2021 08:54, Michael Paquier wrote:

On Wed, Feb 03, 2021 at 11:29:37AM -0500, Tom Lane wrote:

Then let's kill it dead, server and libpq both.

Yeah.

Ok, here we go.

Are you going to bump the .so version for this? I think that should be
done, since some functions disappear and there are struct changes. It
is curious, though, to see that exports.txt needs no changes.

(I'm not sure what's our protocol for so-version changes. Do we wait
till end of cycle, or do we put it together with the commit that
modifies the library? src/tools/RELEASE_CHANGES doesn't say)

--
�lvaro Herrera Valdivia, Chile

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2021-Feb-04, Heikki Linnakangas wrote:

Ok, here we go.

Are you going to bump the .so version for this? I think that should be
done, since some functions disappear and there are struct changes. It
is curious, though, to see that exports.txt needs no changes.

Uh, what? There should be no externally visible ABI changes in libpq
(he says without having read the patch). If there's a need for a library
major version bump, that'd be sufficient reason not to do this IMO.

regards, tom lane

On 2021-Feb-04, Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2021-Feb-04, Heikki Linnakangas wrote:

Ok, here we go.

Are you going to bump the .so version for this? I think that should be
done, since some functions disappear and there are struct changes. It
is curious, though, to see that exports.txt needs no changes.

Uh, what? There should be no externally visible ABI changes in libpq
(he says without having read the patch). If there's a need for a library
major version bump, that'd be sufficient reason not to do this IMO.

Yeah, the changes I was thinking about are all in libpq-int.h so that's
not really a problem. But one enum in libpq-fe.h renumbers values, and
I think it's better to keep the old value labelled as "unused" to avoid
any changes.

--
�lvaro Herrera Valdivia, Chile

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Yeah, the changes I was thinking about are all in libpq-int.h so that's
not really a problem. But one enum in libpq-fe.h renumbers values, and
I think it's better to keep the old value labelled as "unused" to avoid
any changes.

Oh, yeah, can't do that. libpq-fe.h probably shouldn't change at all;
but certainly we can't renumber existing enum values there.

regards, tom lane

On 04/02/2021 17:35, Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Yeah, the changes I was thinking about are all in libpq-int.h so that's
not really a problem. But one enum in libpq-fe.h renumbers values, and
I think it's better to keep the old value labelled as "unused" to avoid
any changes.

Oh, yeah, can't do that. libpq-fe.h probably shouldn't change at all;
but certainly we can't renumber existing enum values there.

Ah, right, there's even a comment above the enum that says that's a no
no. But yeah, fixing that, I see no need for .so version bump.

- Heikki

On Thu, Feb 4, 2021 at 11:47 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 04/02/2021 17:35, Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Yeah, the changes I was thinking about are all in libpq-int.h so that's
not really a problem. But one enum in libpq-fe.h renumbers values, and
I think it's better to keep the old value labelled as "unused" to avoid
any changes.

Oh, yeah, can't do that. libpq-fe.h probably shouldn't change at all;
but certainly we can't renumber existing enum values there.

Ah, right, there's even a comment above the enum that says that's a no
no. But yeah, fixing that, I see no need for .so version bump.

I was able to build libpq and psql on 7.3 with the tooling found on RHEL 7
(the rest of the tree refused to build, but that's not relevant here) and
got the expected message when trying to connect:

master:
Welcome to psql 7.3.21, the PostgreSQL interactive terminal.

patch:
psql: FATAL: unsupported frontend protocol 2.0: server supports 3.0 to 3.0

I couldn't find any traces of version 2 in the tree with the patch applied.
The enum mentioned above seems the only issue that needs to be fixed before
commit.

--
John Naylor
EDB: http://www.enterprisedb.com

Heikki Linnakangas <hlinnaka@iki.fi> writes:

On 04/02/2021 08:54, Michael Paquier wrote:

On Wed, Feb 03, 2021 at 11:29:37AM -0500, Tom Lane wrote:

Then let's kill it dead, server and libpq both.

Ok, here we go.

One interesting thing I noticed while doing this:

Up until now, we always used the old protocol for errors that happened
early in backend startup, before we processed the client's protocol
version and set the FrontendProtocol variable. I'm sure that made sense
when V3 was introduced, but it was a surprise to me, and I didn't find
that documented anywhere. I changed it so that we use V3 errors, if
FrontendProtocol is not yet set.

However, I kept rudimentary support for sending errors in protocol
version 2. This way, if a client tries to connect with an old client, we
still send the "unsupported frontend protocol" error in the old format.
Likewise, I kept the code in libpq to understand v2 ErrorResponse
messages during authentication.

Yeah, we clearly need to send the "unsupported frontend protocol" error
in as old a protocol as we can. Another point here is that if the
postmaster fails to fork() a child process, it has a hack to spit out
an error message without using backend/libpq at all, and that sends
in 2.0 protocol. IIRC that's partly because it's simpler, as well
as backward-friendly. So we should keep these vestiges.

I rebased the 0001 patch (it'd bit-rotted slightly), read it over,
and did some light testing. I found a couple of other places where
we could drop code: any client-side code that has to act differently
for pre-7.4 servers can lose that option, because it'll never be
talking to one of those now.

Patched psql, trying to connect to a 7.3 server, reports this:

$ psql -h ...
psql: error: connection to server at "sss2" (192.168.1.3), port 5432 failed: FATAL: unsupported frontend protocol

$

Conversely, 7.3 psql trying to connect to a patched server reports:

$ psql -h ...
psql: FATAL: unsupported frontend protocol 2.0: server supports 3.0 to 3.0

$

I'm not sure where the extra newlines are coming from, and it seems
unlikely to be worth worrying over. This behavior is good enough for me.

I concur that 0001 attached is committable. I have not looked at
your 0002, though.

regards, tom lane

I wrote:

I concur that 0001 attached is committable. I have not looked at
your 0002, though.

Oh ... grepping discovered one more loose end: mention of fe-protocol2.c
has to be removed from src/interfaces/libpq/nls.mk.

regards, tom lane

On 04/03/2021 01:32, Tom Lane wrote:

Patched psql, trying to connect to a 7.3 server, reports this:

$ psql -h ...
psql: error: connection to server at "sss2" (192.168.1.3), port 5432 failed: FATAL: unsupported frontend protocol

$

Conversely, 7.3 psql trying to connect to a patched server reports:

$ psql -h ...
psql: FATAL: unsupported frontend protocol 2.0: server supports 3.0 to 3.0

$

I'm not sure where the extra newlines are coming from, and it seems
unlikely to be worth worrying over. This behavior is good enough for me.

fe-connect.c appends a newline for any errors in pre-3.0 format:

/*
* The postmaster typically won't end its message with a
* newline, so add one to conform to libpq conventions.
*/
appendPQExpBufferChar(&conn->errorMessage, '\n');

That comment is wrong. The postmaster *does* end all its error messages
with a newline. This changed in commit 9b4bfbdc2c in 7.2. Before that,
postmaster had its own function, PacketSendError(), to send error
messages, and it did not append a newline. Commit 9b4bfbdc2 changed
postmaster to use elog(...) like everyone else, and elog(...) has always
appended a newline. So I think this extra newline that libpq adds is
needed if you try to connect to PostgreSQL 7.1 or earlier. I couldn't
commpile a 7.1 server to verify this, though.

I changed that code in libpq to check if the message already has a
newline, and only append one if it doesn't. This fixes the extra newline
when connecting with new libpq to a 7.3 server (and in the fork failure
message).

I concur that 0001 attached is committable. I have not looked at
your 0002, though.

Removed the entry from nls.mk, and pushed 0001. Thanks!

- Heikki

Heikki Linnakangas <hlinnaka@iki.fi> writes:

On 04/03/2021 01:32, Tom Lane wrote:

I'm not sure where the extra newlines are coming from, and it seems
unlikely to be worth worrying over. This behavior is good enough for me.

fe-connect.c appends a newline for any errors in pre-3.0 format:

/*
* The postmaster typically won't end its message with a
* newline, so add one to conform to libpq conventions.
*/
appendPQExpBufferChar(&conn->errorMessage, '\n');

That comment is wrong. The postmaster *does* end all its error messages
with a newline. This changed in commit 9b4bfbdc2c in 7.2.

Ah-hah, and the bit you show here came in with 2af360ed1, in 7.0.
I'm surprised though that we didn't notice that the newline was now
usually redundant. This was a commonly taken code path until 7.4.

Anyway, your fix seems fine ... I wonder if we should back-patch it?

regards, tom lane

Heikki Linnakangas <hlinnaka@iki.fi> writes:

I concur that 0001 attached is committable. I have not looked at
your 0002, though.

Removed the entry from nls.mk, and pushed 0001. Thanks!

It seems that buildfarm member walleye doesn't like this.
Since nothing else is complaining, I confess bafflement
as to why. walleye seems to be our only active mingw animal,
so maybe there's a platform dependency somewhere ... but
how would (mostly) removal of code expose that?

regards, tom lane

On 04/03/2021 22:04, Tom Lane wrote:

Heikki Linnakangas <hlinnaka@iki.fi> writes:

I concur that 0001 attached is committable. I have not looked at
your 0002, though.

Removed the entry from nls.mk, and pushed 0001. Thanks!

It seems that buildfarm member walleye doesn't like this.
Since nothing else is complaining, I confess bafflement
as to why. walleye seems to be our only active mingw animal,
so maybe there's a platform dependency somewhere ... but
how would (mostly) removal of code expose that?

Strange indeed. The commands that are crashing seem far detached from
any FE/BE protocol handling, and I don't see any other pattern either:

2021-03-04 05:08:45.953 EST [4080:94] DETAIL: Failed process was
running: copy (insert into copydml_test default values) to stdout;

2021-03-04 05:09:22.690 EST [4080:100] DETAIL: Failed process was
running: CREATE INDEX CONCURRENTLY concur_index7 ON concur_heap(f1);

2021-03-04 05:09:33.546 EST [4080:106] DETAIL: Failed process was
running: ANALYZE vaccluster;

2021-03-04 05:09:42.452 EST [4080:112] DETAIL: Failed process was
running: FETCH BACKWARD 1 FROM foo24;

2021-03-04 05:10:06.874 EST [4080:118] DETAIL: Failed process was
running: REFRESH MATERIALIZED VIEW CONCURRENTLY mvtest_tvmm;

2021-03-04 05:12:23.890 EST [4080:125] DETAIL: Failed process was
running: CREATE SUBSCRIPTION regress_testsub CONNECTION 'testconn'
PUBLICATION testpub;

2021-03-04 05:15:46.421 EST [4080:297] DETAIL: Failed process was
running: INSERT INTO xmltest VALUES (3, '<wrong');

Dare I suggest a compiler bug? gcc 8.1 isn't the fully up-to-date,
although I don't know if there's a newer gcc available on this platform.
Joseph, any chance we could see a backtrace or some other details from
those crashes?

'drongo' just reported linker errors:

postgres.def : error LNK2001: unresolved external symbol
GetOldFunctionMessage
[c:\\prog\\bf\\root\\HEAD\\pgsql.build\\postgres.vcxproj]
postgres.def : error LNK2001: unresolved external symbol errfunction
[c:\\prog\\bf\\root\\HEAD\\pgsql.build\\postgres.vcxproj]
postgres.def : error LNK2001: unresolved external symbol pq_getstring
[c:\\prog\\bf\\root\\HEAD\\pgsql.build\\postgres.vcxproj]
postgres.def : error LNK2001: unresolved external symbol pq_putbytes
[c:\\prog\\bf\\root\\HEAD\\pgsql.build\\postgres.vcxproj]
Release/postgres/postgres.lib : fatal error LNK1120: 4 unresolved
externals [c:\\prog\\bf\\root\\HEAD\\pgsql.build\\postgres.vcxproj]
Done Building Project
"c:\\prog\\bf\\root\\HEAD\\pgsql.build\\postgres.vcxproj" (default
targets) -- FAILED.

Looks like it wasn't a clean build, those functions and all the callers
were removed by the patch. That's a separate issue than on 'walleye' -
unless that was also not a completely clean build?

- Heikki