Thread: Removing support for COPY FROM STDIN in protocol version 2
Hi, The server still supports the old protocol version 2. Protocol version 3 was introduced in PostgreSQL 7.4, so there shouldn't be many clients around anymore that don't support it. COPY FROM STDIN is particularly problematic with the old protocol, because the end-of-copy can only be detected by the \. marker. So the server has to read the input one byte at a time, and check for \. as it goes. At [1], I'm working on a patch to change the way the encoding conversion is performed in COPY FROM, so that we convert the data in larger chunks, before scanning the input for line boundaries. We can't do that safely in the old protocol. 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. [1] https://www.postgresql.org/message-id/e7861509-3960-538a-9025-b75a61188e01%40iki.fi - Heikki
Attachment
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&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
Attachment
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
Attachment
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