pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedprotocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
24+ messages / 5 participants
[nested] [flat]
* protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-06-28 17:24 Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 24+ messages in thread
From: Robert Haas @ 2017-06-28 17:24 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
On Mon, Mar 28, 2011 at 7:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> Now if we had a track record showing that we could tweak the protocol
>> version without causing problems, it'd be fine with me to do it for this
>> usage. But we don't, and this particular case doesn't seem like the
>> place to start.
>
> And, btw, a moment's study of the protocol version checking code in
> postmaster.c shows that bumping the minor version number to 3.1 *would*
> break things: a client requesting 3.1 from a current postmaster would
> get a failure.
>
> Maybe we oughta change that logic --- it's not clear to me that there's
> any meaningful difference between major and minor numbers given the
> current postmaster behavior.
I just had a conversation at Postgres Vision that reminded me of the
old thread quoted aboce, and it seems to me that what Tom said there
is a good idea. We've made quite a number of protocol version changes
over the years -- most recently, SCRAM introduced AuthenticationSASL
-- without ever bumping the protocol version. In the case of the
change that this thread was originally about, namely the introduction
of CopyBoth, it could be argued that there is no real need to bump the
protocol version, because the client initiates the use of the new
protocol message and, before doing so, can know whether the server
version is new enough to support that protocol message based on the
ParameterStatus message for server_version which is sent after
authentication is complete. Fair enough, but:
1. If we want to add a new message which is initiated by the server
rather than the client, the server currently has no way of knowing
whether the client is new enough to understand the message, because
the client never identifies itself except by submitting a protocol
version which must always be exactly 3.0 or the connection will be
killed. SCRAM itself suffers from this problem; if an older libpq
tries to connect to a v10 server, they'll get a lousy error message
unless they've got a minor release new enough to contain
96d0f988b150aa0b52b44b8c1adbc7ef59262a1a or one of its siblings.
That's not the worst problem ever, but at the same time it's not
really a good design to rely on people upgrading the clients in order
to get the correct message about the client being too old. It would
be much better if the client identified itself to the server. Then if
the server sees that the client is old, it can give the client an
appropriate error, and if the client is new enough, then it can be
sure that it will be understood.
2. If we want to add a new message is initiated by the client before
authentication is complete, ParameterStatus messages won't have been
sent yet, so the client has no idea what capabilities the server might
be able to support.
3. It's a bit unclean to use the server version as a proxy for the
wire protocol version; there are non-PostgreSQL servers which speak
the PostgreSQL wire protocol, and making them impersonate a specific
version of PostgreSQL in order to clarify which wire protocol features
they support isn't really very nice. So maybe there's a case for
bumping the minor version each time we actually change the protocol
rather than only in the cases where the client has no other method of
discovering the server version. For example, you can imagine a
connection proxy like pgpool or pgbouncer wanting to limit the client
to a particular wire protocol version -- the highest that it supports
-- while proxying to a newer server version sitting behind it. That
doesn't work if the server version is being abused as a sort of
back-door protocol version.
Here's my proposal:
- If the server receives a StartupMessage for v3.x where x > the
version it knows, instead of just slamming the connection shut, it
responds by sending some new message (let's say,
NegotiateProtocolVersion) specifying the highest protocol version it
supports.
- The client may then continue by sending a new StartupMessage with a
version with a version number that is within range.
If we had this, then bumping the minor protocol version wouldn't
require retrying connections, which among other problems generates
scary error messages in the logs; instead, they would succeed, but
with one additional round trip. That seems like it would represent a
substantial improvement over what we have now, where bumping the
protocol version is such an incredibly painful thing that we never do
it, even when we've actually broken wire protocol compatibility!
Of course, it would take a few releases before server versions that
support this mechanism were widespread, and so clients using a
hypothetical version 3.1 would still have to be prepared to retry the
whole connection, but if we use that as a reason not to make a change
of this sort then it's not clear how things ever get any better than
they are now. And it's clear that there is pent-up demand for the
ability to make minor additions to the protocol without breaking the
whole thing - see
https://www.postgresql.org/message-id/1318235405.31687.0.camel%40fsopti579.F-Secure.com
and https://wiki.postgresql.org/wiki/Todo#Wire_Protocol_Changes_.2F_v4_Protocol
- and while it's not clear that all of those ideas are good ones, some
of them probably are, and even if by some remote chance all of the
people who suggested them are all dead wrong, there will be more
things suggested in the future. We shouldn't wait until our back is
totally against the wall before we introduce some mechanism here that
is better than what we have today. I'm not really keen on trying to
introduce a v4 protocol any time soon, but I really like the idea of
having a reasonably low-pain way to introduce a v3.1 that adds modest
new capabilities.
Thoughts, better ideas?
--
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-06-28 17:47 Tom Lane <tgl@sss.pgh.pa.us>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 24+ messages in thread
From: Tom Lane @ 2017-06-28 17:47 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
> Here's my proposal:
> - If the server receives a StartupMessage for v3.x where x > the
> version it knows, instead of just slamming the connection shut, it
> responds by sending some new message (let's say,
> NegotiateProtocolVersion) specifying the highest protocol version it
> supports.
> - The client may then continue by sending a new StartupMessage with a
> version with a version number that is within range.
How about a slightly simpler design: we specify that all 3.x protocol
versions shall be mutually compatible so long as each side knows not
to send something that the other side doesn't understand. The field
in the StartupMessage is understood to be the max protocol version the
client knows how to cope with. If the client sends something > 3.0,
then the server responds with a ServerProtocolVersion message containing
the max protocol version it understands. Now both sides know what the
other side can do and should be able to adapt their behavior accordingly.
(If the client doesn't get a ServerProtocolVersion message, it should
assume server protocol 3.0.)
In order to make the world safe for this, we'd have to adjust existing
releases to not complain about 3.x for x > 0, and the sooner we do that
the better chance of clients being able to make use of this within a
reasonable timeframe.
It's possible that we should do something that's not based on just a
linear protocol version number, but instead involves some kind of
bitmask of capabilities, say. So the ServerProtocolVersion message
maybe needs to be a bit more complicated, and if the client does get
one back, maybe it should forward a ClientProtocolVersion message with
its own bitmask. But these things could be designed in detail later.
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-06-28 19:01 Robert Haas <robertmhaas@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 24+ messages in thread
From: Robert Haas @ 2017-06-28 19:01 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
On Wed, Jun 28, 2017 at 1:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> Here's my proposal:
>
>> - If the server receives a StartupMessage for v3.x where x > the
>> version it knows, instead of just slamming the connection shut, it
>> responds by sending some new message (let's say,
>> NegotiateProtocolVersion) specifying the highest protocol version it
>> supports.
>
>> - The client may then continue by sending a new StartupMessage with a
>> version with a version number that is within range.
>
> How about a slightly simpler design: we specify that all 3.x protocol
> versions shall be mutually compatible so long as each side knows not
> to send something that the other side doesn't understand. The field
> in the StartupMessage is understood to be the max protocol version the
> client knows how to cope with. If the client sends something > 3.0,
> then the server responds with a ServerProtocolVersion message containing
> the max protocol version it understands. Now both sides know what the
> other side can do and should be able to adapt their behavior accordingly.
> (If the client doesn't get a ServerProtocolVersion message, it should
> assume server protocol 3.0.)
One problem with that is that it means that the format of the
StartupMessage itself can never change, which I think is not a good
choice. If, for example, we want to add a new option to the startup
message (aside from the existing user, database, options, and
replication keywords), we really can't do that today. It wouldn't be
so bad if unrecognized parameters were just ignored; the client would
know from the ServerProtocolVersion (or ParameterStatus) message that
server had ignored those options and could respond as it saw fit. But
because they are treated as GUCs the server will error out. This is
clearly not an entirely theoretical concern considering that
'replication' was added as a keyword here within recent memory.
> In order to make the world safe for this, we'd have to adjust existing
> releases to not complain about 3.x for x > 0, and the sooner we do that
> the better chance of clients being able to make use of this within a
> reasonable timeframe.
>
> It's possible that we should do something that's not based on just a
> linear protocol version number, but instead involves some kind of
> bitmask of capabilities, say. So the ServerProtocolVersion message
> maybe needs to be a bit more complicated, and if the client does get
> one back, maybe it should forward a ClientProtocolVersion message with
> its own bitmask. But these things could be designed in detail later.
Right. So for example we could decide that any parameter names that
are passed in the startup packet that begin with an underscore are not
GUCs but some kind of protocol extension; the server replies with some
message containing a list of the ones which were not understood.
--
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-06-29 01:44 Craig Ringer <craig@2ndquadrant.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 2 replies; 24+ messages in thread
From: Craig Ringer @ 2017-06-29 01:44 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
On 29 June 2017 at 03:01, Robert Haas <robertmhaas@gmail.com> wrote:
> One problem with that is that it means that the format of the
> StartupMessage itself can never change, which I think is not a good
> choice.
The startup message could be immediately followed by another
supplemental message, though.
- Startup["Protocol 3.2"]
- Extra Info Message
A server that only knows protocol version 3.1 would read the 3.1
message and reply with a NegotiateProtocolVersion or whatever, then
*discard all messages until it sees a new startup message*, so it
wouldn't be upset. Quite like what we do when reading until Sync after
an error.
Existing servers would bounce the connection when they saw the 3.2
field and the client would have to reconnect with 3.0. But we could
possibly even backpatch the nicer behaviour, since it's simple and
will only ever come into effect when sent a higher protocol version
msg that no existing client generates.
If we ever landed up wanting to greatly revise the startup message
down the track, we could land up sending a stub startup message with
protocol 3.4 or whatever that just tells aware servers "real startup
message follows". Bit of a waste, but not that bad given how expensive
connections are. A dummy startup message is quite small.
I don't have a strong opinion here, just raising the possibility that
not being able to vary the startup message format may not be the end
of the world.
> If, for example, we want to add a new option to the startup
> message (aside from the existing user, database, options, and
> replication keywords), we really can't do that today.
Right. An obvious example would be to put a starttls-like request in
it, asking the server to initiate TLS negotiation if supported.
Or an option asking the server to determine the DB to connect to based
on some property of the user.
Similarly, I've repeatedly wanted ways to specify client support for /
requests for optional protocol messages, like sending the xid and
commit record lsn of a commit along with the commit message.
Both those could be done in immediate follow-up messages, though. I
can't personally think of much right away that wouldn't work pretty
well in a follow-on message. There's a small one-off protocol size
overhead with doing it that way, but no extra latency, so who cares?
> It wouldn't be
> so bad if unrecognized parameters were just ignored; the client would
> know from the ServerProtocolVersion (or ParameterStatus) message that
> server had ignored those options and could respond as it saw fit.
Yeah. In retrospect it's a pity the key/value pairs don't come with
some kind of optional/required flag so the client can say "error if
you don't understand this, it's important" or "I don't care if you
don't understand this, and I'll notice when you fail to GUC_REPORT it
back to me". Or some convention like underscore-prefixing for
optional.
>> It's possible that we should do something that's not based on just a
>> linear protocol version number, but instead involves some kind of
>> bitmask of capabilities, say. So the ServerProtocolVersion message
>> maybe needs to be a bit more complicated, and if the client does get
>> one back, maybe it should forward a ClientProtocolVersion message with
>> its own bitmask. But these things could be designed in detail later.
>
> Right. So for example we could decide that any parameter names that
> are passed in the startup packet that begin with an underscore are not
> GUCs but some kind of protocol extension; the server replies with some
> message containing a list of the ones which were not understood.
There's a *lot* of value to that idea when you consider proxies like
PgPool-II and PgBouncer that may struggle to satisfy all capabilities
of some protocol 3.3 but want to offer some key parts of it.
There's a price in terms of complexity but probably not a big one.
Some places that would be "client protocol version >= 3.3" would
instead be "client has_capability(x)". Possibly cleaner to read even
if we _did_ use a linear protocol version, frankly.
Also, importantly in my opinion, this would let clients turn things
on/off easily. I badly want this myself, as I really want the server
to be able to reply to each COMMIT with an extended CommandComplete
message or extra-info message after it, containing the commit record
LSN. Similarly, I really want to be able to send the xid of an xact on
the wire when one is assigned to an xact. Many clients won't need,
want or understand these things and whether they're protocol 3.0 or
3.9 they can just not ask for them.
Capabilities will make startup messages bigger. Personally I don't
care much about that, as on modern networks it's all about latency not
message size. We'd use abbreviated capability names I expect. If the
list gets too big we could always roll up capabilities that have
become universally adopted into the next protocol version bump so it's
assumed that a client announcing proto 3.3 supports feature x and
doesn't need to list it.
--
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-06-29 01:55 Craig Ringer <craig@2ndquadrant.com>
parent: Craig Ringer <craig@2ndquadrant.com>
1 sibling, 0 replies; 24+ messages in thread
From: Craig Ringer @ 2017-06-29 01:55 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
On 29 June 2017 at 09:44, Craig Ringer <craig@2ndquadrant.com> wrote:
> I
> can't personally think of much right away that wouldn't work pretty
> well in a follow-on message.
Actually, I take that back, there's one thing that's bugged me for a
while that wouldn't work well this way: determining the correct text
encoding with which to interpret the database name, user name, and any
string GUC values. Right now we take a wild stab in the dark and use
the server encoding.
e.g. in a terminal with LC_ALL=en_US.UTF-8 I run
postgres=# CREATE ROLE "café";
CREATE ROLE
postgres=# CREATE DATABASE "café" with owner "café";
CREATE DATABASE
postgres=# \du ca*
List of roles
Role name | Attributes | Member of
-----------+--------------+-----------
café | Cannot login | {}
postgres=# \l ca*
List of databases
Name | Owner | Encoding | Collate | Ctype | Access privileges
------+-------+----------+-------------+-------------+-------------------
café | café | UTF8 | en_AU.UTF-8 | en_AU.UTF-8 |
(1 row)
then in a terminal with LC_ALL=en_US.ISO-8859-1 I run:
[craig@ayaki-localdomain log]$ psql -U "café"
psql: FATAL: role "café" does not exist
[craig@ayaki-localdomain log]$ psql
psql (9.6.3, server 9.5.7)
Type "help" for help.
craig=> \du ca*
List of roles
Role name | Attributes | Member of
-----------+--------------+-----------
café | Cannot login | {}
craig=> \l ca*
List of databases
Name | Owner | Encoding | Collate | Ctype | Access privileges
------+-------+----------+-------------+-------------+-------------------
café | café | UTF8 | en_AU.UTF-8 | en_AU.UTF-8 |
(1 row)
craig=> \c café
FATAL: database "café" does not exist
Previous connection kept
Um, say what?
This happens because psql does server=>client encoding conversion once
connected, but there's no way to convert the startup message with the
username.
That IMO is an argument to allow startup message format change. (It'd
also then let the server reply with correctly-encoded pre-auth
messages for errors, something we currently fail to do).
--
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-06-29 02:27 Tom Lane <tgl@sss.pgh.pa.us>
parent: Craig Ringer <craig@2ndquadrant.com>
1 sibling, 2 replies; 24+ messages in thread
From: Tom Lane @ 2017-06-29 02:27 UTC (permalink / raw)
To: Craig Ringer <craig@2ndquadrant.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
Craig Ringer <craig@2ndquadrant.com> writes:
> On 29 June 2017 at 03:01, Robert Haas <robertmhaas@gmail.com> wrote:
>> It wouldn't be
>> so bad if unrecognized parameters were just ignored; the client would
>> know from the ServerProtocolVersion (or ParameterStatus) message that
>> server had ignored those options and could respond as it saw fit.
> Yeah. In retrospect it's a pity the key/value pairs don't come with
> some kind of optional/required flag so the client can say "error if
> you don't understand this, it's important" or "I don't care if you
> don't understand this, and I'll notice when you fail to GUC_REPORT it
> back to me". Or some convention like underscore-prefixing for
> optional.
Yeah. Back in the day I helped design the PNG image format, and one
of the better ideas in it was to make a distinction between critical and
noncritical chunks within a PNG file; that was exactly the idea you're
getting at here. I agree with the suggestion to drive this off a
parameter-name-based convention. Having leading underscore indicate
a noncritical parameter sounds fine.
I don't really like any of the ideas that have been mentioned that would
introduce extra network round trips into session startup. It's expensive
enough already. The only thing we seem to be really hurting on is the
ability for the client to add extensions to the original StartupMessage,
and this seems like it can fix that.
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-06-29 04:23 Craig Ringer <craig@2ndquadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 24+ messages in thread
From: Craig Ringer @ 2017-06-29 04:23 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
On 29 June 2017 at 10:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Craig Ringer <craig@2ndquadrant.com> writes:
>> On 29 June 2017 at 03:01, Robert Haas <robertmhaas@gmail.com> wrote:
>>> It wouldn't be
>>> so bad if unrecognized parameters were just ignored; the client would
>>> know from the ServerProtocolVersion (or ParameterStatus) message that
>>> server had ignored those options and could respond as it saw fit.
>
>> Yeah. In retrospect it's a pity the key/value pairs don't come with
>> some kind of optional/required flag so the client can say "error if
>> you don't understand this, it's important" or "I don't care if you
>> don't understand this, and I'll notice when you fail to GUC_REPORT it
>> back to me". Or some convention like underscore-prefixing for
>> optional.
>
> Yeah. Back in the day I helped design the PNG image format, and one
> of the better ideas in it was to make a distinction between critical and
> noncritical chunks within a PNG file; that was exactly the idea you're
> getting at here. I agree with the suggestion to drive this off a
> parameter-name-based convention. Having leading underscore indicate
> a noncritical parameter sounds fine.
>
> I don't really like any of the ideas that have been mentioned that would
> introduce extra network round trips into session startup. It's expensive
> enough already. The only thing we seem to be really hurting on is the
> ability for the client to add extensions to the original StartupMessage,
> and this seems like it can fix that.
It does. But I don't see anywhere that extra round trips have been discussed.
--
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-06-29 04:24 Craig Ringer <craig@2ndquadrant.com>
parent: Craig Ringer <craig@2ndquadrant.com>
0 siblings, 0 replies; 24+ messages in thread
From: Craig Ringer @ 2017-06-29 04:24 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
On 29 June 2017 at 12:23, Craig Ringer <craig@2ndquadrant.com> wrote:
> It does. But I don't see anywhere that extra round trips have been discussed.
Ah, right, they're implied by having the server respond with some
downversion message and ignore input until the client sends a new
startup message. That'll only happen when a too-new client connects to
an older server, but that's probably not an especially unusual case.
--
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-06-29 12:18 Robert Haas <robertmhaas@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 2 replies; 24+ messages in thread
From: Robert Haas @ 2017-06-29 12:18 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
On Wed, Jun 28, 2017 at 10:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah. Back in the day I helped design the PNG image format, and one
> of the better ideas in it was to make a distinction between critical and
> noncritical chunks within a PNG file; that was exactly the idea you're
> getting at here. I agree with the suggestion to drive this off a
> parameter-name-based convention. Having leading underscore indicate
> a noncritical parameter sounds fine.
I'm not sure if non-critical is exactly the right terminology. What
we want to do is distinguish between things that are intended as
protocol-level options vs. things that are intended as GUCs. Of
course, there's nothing that actually prevents a a GUC from starting
with an underscore:
rhaas=# set _foo._bar = 1;
SET
Maybe we should instead pick a GUC namespace and reserve it for the
use of protocol level options; e.g. pg_protocol.<anything> becomes
invalid as the name of a GUC, but can be included in a startup packet
(or do we need to pick something shorter, like _pg, to keep the
message short?). Servers ignore anything that they don't understand.
So:
1. The client sends a StartupMessage 3.x for version 3.x. We could
bump the version explicitly, or perhaps we should just coin a version
of libpq for every server release; e.g. whatever PostgreSQL 11 ships
is version 3.11, etc. It includes any protocol options that don't
exist today as pg_protocol.<whatever> in the startup packet.
2. If the client version is anything other than 3.0, the server
responds with a ServerProtocolVersion indicating the highest version
it supports, and ignores any pg_protocol.<whatever> options not known
to it as being either third-party extensions or something from a
future version. If the initial response to the startup message is
anything other than a ServerProtocolVersion message, the client should
assume it's talking to a 3.0 server. (To make this work, we would
back-patch a change into existing releases to allow any 3.x protocol
version and ignore any pg_protocol.<whatever> options that were
specified.)
If either the client or the server is unhappy about the age of the
other, then it can disconnect; e.g. if the server is configured to
require the use of whizzbang-2 security, and the client protocol
version indicates that at most whizzbang-1.9 is available, then the
server can close the connection with a suitable complaint; conversely,
if the connection string had require_whizzbang=2, and the server is
too old to support that, then the client can decide to bail out when
it receives the ServerProtocolVersion message.
Still just thinking out loud here. Thoughts?
--
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-06-29 23:29 Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 1 reply; 24+ messages in thread
From: Satyanarayana Narlapuram @ 2017-06-29 23:29 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
-----Original Message-----
From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Robert Haas
Sent: Thursday, June 29, 2017 5:18 AM
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: protocol version negotiation (Re: [HACKERS] Libpq PGRES_COPY_BOTH - version compatibility)
> 1. The client sends a StartupMessage 3.x for version 3.x. We could bump the version explicitly, or perhaps we should just coin a version of libpq for every server release; e.g. whatever PostgreSQL 11 ships is version 3.11, etc. It includes any protocol options that don't exist today as > pg_protocol.<whatever> in the startup packet.
+1 on this. Happy to read this conversation. I am hopeful that this provides us a path to include parameters needed for Azure database for PostgreSQL service (host name, and connection id in the startupmessage). For someone wondering what they are, please see the threads below.
https://www.postgresql.org/message-id/DM2PR03MB416343FC02D6E977FEF2EB191C00%40DM2PR03MB416.namprd03....
https://www.postgresql.org/message-id/DM2PR03MB4168F3C796B2965FDC4CF9991C00%40DM2PR03MB416.namprd03....
2. If the client version is anything other than 3.0, the server responds with a ServerProtocolVersion indicating the highest version it supports, and ignores any pg_protocol.<whatever> options not known to it as being either third-party extensions or something from a future version. If the initial response to the startup message is anything other than a ServerProtocolVersion message, the client should assume it's talking to a 3.0 server. (To make this work, we would back-patch a change into existing releases to allow any 3.x protocol version and ignore any pg_protocol.<whatever> options that were specified.)
> If the client version is anything other than 3.0, the server responds with a ServerProtocolVersion indicating the highest version it supports, and ignores any pg_protocol.<whatever> options not known to it as being either third-party extensions or something from a future version. If > the initial response to the startup message is anything other than a ServerProtocolVersion message, the client should assume it's talking to a 3.0 server. (To make this work, we would back-patch a change into existing releases to allow any 3.x protocol version and ignore any > pg_protocol.<whatever> options that were specified.)
We can avoid one round trip if the server accepts the startupmessage as is (including understanding all the parameters supplied by the client), and in the cases where server couldn’t accept the startupmessage / require negotiation, it should send ServerProtocolVersion message that contains both MIN and MAX versions it can support. Providing Min version helps server enforce the client Min protocol version, and provides a path to deprecate older versions. Thoughts?
> If either the client or the server is unhappy about the age of the other, then it can disconnect; e.g. if the server is configured to require the use of whizzbang-2 security, and the client protocol version indicates that at most whizzbang-1.9 is available, then the server can close the > connection with a suitable complaint; conversely, if the connection string had require_whizzbang=2, and the server is too old to support that, then the client can decide to bail out when it receives the ServerProtocolVersion message.
Does the proposal also include the client can negotiate the protocol version on the same connection rather than going through connection setup process again? The state machine may not sound simple with this proposal but helps bringing down total time taken for the login.
Client / server can disconnect any time they think the negotiation failed.
Thanks,
Satya
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-06-30 02:49 Craig Ringer <craig@2ndquadrant.com>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 0 replies; 24+ messages in thread
From: Craig Ringer @ 2017-06-30 02:49 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
On 29 June 2017 at 20:18, Robert Haas <robertmhaas@gmail.com> wrote:
> I'm not sure if non-critical is exactly the right terminology. What
> we want to do is distinguish between things that are intended as
> protocol-level options vs. things that are intended as GUCs.
We probably also need to be able to differentiate between "this
protocol option would be nice if you support it, please tell me in
your parameter status message if you managed to honour it" vs "this
connection request is invalid if you cannot honour this protocol
option, do not attempt to proceed".
If the option changes the meaning of the request entirely in some way
we probably don't want a server that doesn't understand carrying on
anyway.
But I think it's probably sufficient if the server's reply to the
client includes the options it was able to recognise and honour, so
the client can immediately nope out if it doesn't like the server's
lack of support for some option.
> Maybe we should instead pick a GUC namespace and reserve it for the
> use of protocol level options; e.g. pg_protocol.<anything> becomes
> invalid as the name of a GUC, but can be included in a startup packet
> (or do we need to pick something shorter, like _pg, to keep the
> message short?). Servers ignore anything that they don't understand.
> So:
Yes, but something shorter probably, Tom's already expressed concerns
elsewhere about bandwidth use from reporting server_version_num and I
can't imagine numerous pg_protocol_blahblah entries being popular with
many people at all.
_pg_... seems sufficient IMO, we don't use that in GUCs to date, and
nobody's likely to.
Or to be consistent with protocol and libpq stuff, _pq_... ?
> 1. The client sends a StartupMessage 3.x for version 3.x. We could
> bump the version explicitly, or perhaps we should just coin a version
> of libpq for every server release; e.g. whatever PostgreSQL 11 ships
> is version 3.11, etc. It includes any protocol options that don't
> exist today as pg_protocol.<whatever> in the startup packet.
Not a fan of bumping for the sake of it, it puts more work on 3rd
party drivers for what seems like no real gain.
Bump when we change something.
> 2. If the client version is anything other than 3.0, the server
> responds with a ServerProtocolVersion indicating the highest version
> it supports
Highest and lowest, I think Satyanarayana Narlapuram is right there.
Also which startup parameter protocol options were recognised and
honoured, so the client can immediately notice and bail if the server
didn't recognise something it requires. (think behaviour akin to
sslmode=require for _pg_starttls).
> and ignores any pg_protocol.<whatever> options not known
> to it as being either third-party extensions or something from a
> future version.
I think ignoring is fine if the server lists the ones it recognises in
its reply, per above.
> If the initial response to the startup message is
> anything other than a ServerProtocolVersion message, the client should
> assume it's talking to a 3.0 server. (To make this work, we would
> back-patch a change into existing releases to allow any 3.x protocol
> version and ignore any pg_protocol.<whatever> options that were
> specified.)
Such a backpatch makes sense, and is pretty safe.
--
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-07-02 22:44 Robert Haas <robertmhaas@gmail.com>
parent: Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>
0 siblings, 0 replies; 24+ messages in thread
From: Robert Haas @ 2017-07-02 22:44 UTC (permalink / raw)
To: Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
On Thu, Jun 29, 2017 at 7:29 PM, Satyanarayana Narlapuram
<Satyanarayana.Narlapuram@microsoft.com> wrote:
> -----Original Message-----
The formatting of this message differs from the style normally used on
this mailing list, and is hard to read.
> 2. If the client version is anything other than 3.0, the server responds with a ServerProtocolVersion indicating the highest version it supports, and ignores any pg_protocol.<whatever> options not known to it as being either third-party extensions or something from a future version. If the initial response to the startup message is anything other than a ServerProtocolVersion message, the client should assume it's talking to a 3.0 server. (To make this work, we would back-patch a change into existing releases to allow any 3.x protocol version and ignore any pg_protocol.<whatever> options that were specified.)
>
> We can avoid one round trip if the server accepts the startupmessage as is (including understanding all the parameters supplied by the client), and in the cases where server couldn’t accept the startupmessage / require negotiation, it should send ServerProtocolVersion message that contains both MIN and MAX versions it can support. Providing Min version helps server enforce the client Min protocol version, and provides a path to deprecate older versions. Thoughts?
With this latest proposal, there are no extra round-trips anyway. I
don't much see the point of having the server advertise a minimum
supported version. The idea of new minor protocol versions is to add
*optional* features, so there shouldn't be an issue with the client
being too old to talk to the server altogether. Of course, the server
might be configured to reject the client unless some particular new
feature is in use, but that's best handled by a message about the
specific problem at hand rather than a generic complaint.
> Does the proposal also include the client can negotiate the protocol version on the same connection rather than going through connection setup process again? The state machine may not sound simple with this proposal but helps bringing down total time taken for the login.
Nothing in that proposal involved an extra connection setup process;
if that's not clear, you might want to reread it.
--
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-10-04 01:35 Badrul Chowdhury <bachow@microsoft.com>
0 siblings, 1 reply; 24+ messages in thread
From: Badrul Chowdhury @ 2017-10-04 01:35 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
Hello,
Kindly review this patch that implements the proposal for pgwire protocol negotiation described in this thread.
A big thanks to @Satyanarayana Narlapuram<mailto:Satyanarayana.Narlapuram@microsoft.com> for his help and guidance in implementing the patch.
Please note:
1. Pgwire protocol v3.0 with negotiation is called v3.1.
2. There are 2 patches for the change: a BE-specific patch that will be backported and a FE-specific patch that is only for pg10 and above.
/*************************/
/* Feature In A Nutshell */
/*************************/
This feature enables a newer FE to connect to an older BE using a protocol negotiation phase in the connection setup. The FE sends its protocol version and a list of startup parameters to the BE to start a connection. If the FE pgwire version is newer than the BE's or if the BE could not recognize some of the startup parameters,
the BE simply ignores the parameters that it did not recognize and sends a ServerProtocolVersion message containing the minimum and maximum versions it supports as well as a list of the parameters that it did honour. The FE then decides whether it wants to continue connecting- in the patch, the FE continues only if the parameters that the BE did not honour were optional, where optional parameters are denoted by a proper prefix of "_pq_" in their names.
/************/
/* BE Patch */
/************/
Files modified:
src/
└── backend/
└── postmaster/
└── postmaster.c
└── utils/misc/
└── guc.c
src/
└── include/postmaster/
└── postmaster.h
Design decisions:
1. The BE sends a message type of ServerProtocolVersion if the FE is newer or the FE is not using v3.0
1.a Message structure: [ 'Y' | msgLength | min_version | max_version | param_list_len | list of param names ]
1.b 'Y' was selected to denote ServerProtocolVersion messages as it is the first available character from the bottom of the list of available characters that is not being used to denote any message type at present.
2. Added support for BE to accept optional parameters
2.a An optional parameter is a startup parameter with "_pq_" as a proper prefix in its name. The check to add a placeholder in /src/backend/utils/misc/guc.c was modified to allow optional parameters.
2.b The string comparison in is_optional() is encoding-aware as the BE's encoding might be different from the FE's.
/************/
/* FE Patch */
/************/
Files modified:
src/
└── Makefile
src/
└── Makefile.global.in
src/
└── bin/
└── pg_dump/
└── pg_dump.c
└── scripts/
├── clusterdb.c
├── createuser.c
├── reindexdb.c
└── vacuumdb.c
src/
└── common/
└── Makefile
src/
└── fe_utils/
└── Makefile
└── simple_list.c
src/
└── include/
└── fe_utils/
└── simple_list.h
└── libpq/
└── pqcomm.h
src/
└── interfaces/libpq/
├── fe-connect.c
├── fe-protocol3.c
├── libpq-fe.h
└── libpq-int.h
src/
└── tools/msvc/
└── Mkvcbuild.pm
Design decisions:
1. Added mechanism to send startup parameters to BE:
SimpleStringList startup_parameters in /src/interfaces/libpq/fe-connect.c enables sending parameters to the BE. The startup_parameters list is parsed in /src/interfaces/libpq/fe-protocol3.c and the arguments are sent in the startup packet to the BE.
This is mostly used for testing at this point- one would send additional startup parameters like so in PQconnectStartParams():
simple_string_list_append(&startup_parameters, "_pq_A");
simple_string_list_append(&startup_parameters, "1");
startup_parameters.immutable = true; // PQconnectStartParams() is called twice for the same connection; the immutable flag ensures that the same parameter is not added twice
2. Added a CONNECTION_NEGOTIATING state in /src/interfaces/libpq/fe-connect.c to parse the ServerProtocolVersion message from the BE
2.a The FE terminates the connection if BE rejected non-optional parameters.
3. The changes to the following files were due to the addition of the immutable field in the SimpleStringList struct
3.a /src/bin/pg_dump/pg_dump.c
3.b /src/bin/scripts/clusterdb.c
3.c /src/bin/scripts/createuser.c
3.d /src/bin/scripts/reindexdb.c
3.e /src/bin/scripts/vacuumdb.c
4. Added some dependencies to the libpq project to be able to use SimpleStringList in fe-connect.c.
4.a Visual C compiler
4.a.1 /src/tools/msvc/Mkvcbuild.pm was modified to add the reference for visual c compiler
4.b Non-windows compilers: the following files were modified
4.b.1 /src/Makefile
4.b.2 /src/Makefile.global.in
4.b.3 /src/common/Makefile
4.b.4 /src/fe_utils/Makefile
5. Bumped max_pg_protocol version from 3.0 to 3.1 in /src/include/libpq/pqcomm.h
/***********/
/* Testing */
/***********/
An outline of the testing framework used to validate the code:
1. Newer FE can connect to older BE
1.a Change FE protocol version in line 1811 of /src/interfaces/libpq/fe-connect.c
1.a.1 conn->pversion = PG_PROTOCOL(3, 1); succeeds now whereas it would have failed before: a result of bumping max_pg_protocol from 3.0 to 3.1
1.a.2 conn->pversion = PG_PROTOCOL(4, 0); succeds now whereas it would have failed before: older BE is capable of handling newer FE now
2. Older drivers work as expected
2.a Change FE protocol version in line 1811 of /src/interfaces/libpq/fe-connect.c
2.a.1 conn->pversion = PG_PROTOCOL(1, 9); fails now and failed before as well as it is below min_pg_protocol=PG_PROTOCOL(2, 0)
2.a.2 conn->pversion = PG_PROTOCOL(2, 0); succeeds now and succeeded before as well as it is in the supported range PG_PROTOCOL(2, 0) to PG_PROTOCOL(3, 1)
2.a.3 conn->pversion = PG_PROTOCOL(3, 0); succeeds now and succeeded before as well as it is in the supported range PG_PROTOCOL(2, 0) to PG_PROTOCOL(3, 1)
3. BE support for optional parameters
3.a Accept names with "_pq_" as proper prefix, eg "_pq_A" would succeed
3.b Reject all others without crashing
3.b.1 Any string not like "_pq_A" should fail
Looking forward to your feedback,
Thank you,
Badrul Chowdhury
From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Robert Haas
Sent: Sunday, July 2, 2017 3:45 PM
To: Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
On Thu, Jun 29, 2017 at 7:29 PM, Satyanarayana Narlapuram
<Satyanarayana.Narlapuram@microsoft.com<mailto:Satyanarayana.Narlapuram@microsoft.com>> wrote:
> -----Original Message-----
The formatting of this message differs from the style normally used on
this mailing list, and is hard to read.
> 2. If the client version is anything other than 3.0, the server responds with a ServerProtocolVersion indicating the highest version it supports, and ignores any pg_protocol.<whatever> options not known to it as being either third-party extensions or something from a future version. If the initial response to the startup message is anything other than a ServerProtocolVersion message, the client should assume it's talking to a 3.0 server. (To make this work, we would back-patch a change into existing releases to allow any 3.x protocol version and ignore any pg_protocol.<whatever> options that were specified.)
>
> We can avoid one round trip if the server accepts the startupmessage as is (including understanding all the parameters supplied by the client), and in the cases where server couldn’t accept the startupmessage / require negotiation, it should send ServerProtocolVersion message that contains both MIN and MAX versions it can support. Providing Min version helps server enforce the client Min protocol version, and provides a path to deprecate older versions. Thoughts?
With this latest proposal, there are no extra round-trips anyway. I
don't much see the point of having the server advertise a minimum
supported version. The idea of new minor protocol versions is to add
*optional* features, so there shouldn't be an issue with the client
being too old to talk to the server altogether. Of course, the server
might be configured to reject the client unless some particular new
feature is in use, but that's best handled by a message about the
specific problem at hand rather than a generic complaint.
> Does the proposal also include the client can negotiate the protocol version on the same connection rather than going through connection setup process again? The state machine may not sound simple with this proposal but helps bringing down total time taken for the login.
Nothing in that proposal involved an extra connection setup process;
if that's not clear, you might want to reread it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org<mailto:pgsql-hackers@postgresql.org>)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
[application/octet-stream] be_pgwire3.1.patch (8.1K, ../../BN6PR21MB0772FFA0CBD298B76017744CD1730@BN6PR21MB0772.namprd21.prod.outlook.com/3-be_pgwire3.1.patch)
download | inline diff:
From bf371b501580cfa300c5198cc11ac67acc7631c3 Mon Sep 17 00:00:00 2001
From: Badrul Chowdhury <bachow@microsoft.com>
Date: Mon, 25 Sep 2017 13:42:44 -0700
Subject: [PATCH] =?UTF-8?q?Introducing=20pgwire=20v3.1:=201.=20adds=20supp?=
=?UTF-8?q?ort=20for=20backend=20(BE)=20to=20accept=20optional=20parameter?=
=?UTF-8?q?s,=20ie=20parameters=20that=20have=20=E2=80=9C=5Fpq=5F=E2=80=9D?=
=?UTF-8?q?=20as=20a=20proper=20prefix=20in=20their=20names=202.=20creates?=
=?UTF-8?q?=20data=20structure=20for=20passing=20in=20additional=20paramet?=
=?UTF-8?q?ers=20in=20FE,=20adding=20command=20line=20support=20is=20out?=
=?UTF-8?q?=20of=20scope=20of=20this=20item=203.=20enhances=20FE->BE=20pro?=
=?UTF-8?q?tocol=20negotiation:=20adds=20support=20for=20newer=20FE=20to?=
=?UTF-8?q?=20connect=20to=20older=20BE=20while=20maintaining=20back-compa?=
=?UTF-8?q?tibility,=20ie=20same=20FE=20<->=20BE,=20older=20FE=20to=20newe?=
=?UTF-8?q?r=20BE=20work=20as=20before?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
---
src/backend/postmaster/postmaster.c | 112 ++++++++++++++++++++---
src/backend/utils/misc/guc.c | 22 ++++-
src/include/postmaster/postmaster.h | 7 ++
20 files changed, 372 insertions(+), 40 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 95180b2ef5..21d77333be 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -103,6 +103,7 @@
#include "lib/ilist.h"
#include "libpq/auth.h"
#include "libpq/libpq.h"
+#include "libpq/pqformat.h"
#include "libpq/pqsignal.h"
#include "miscadmin.h"
#include "pg_getopt.h"
@@ -567,6 +568,77 @@ HANDLE PostmasterHandle;
#endif
/*
+ * Initiate protocol negotiation phase;
+ * protocol negotiation is only supported for pgwire version 3.x, x>0.
+ *
+ * Ensure that the packet write buffer is flushed.
+ */
+int
+NegotiateServerProtocol(Port *port)
+{
+ if (whereToSendOutput != DestRemote ||
+ PG_PROTOCOL_MAJOR(FrontendProtocol) < 3)
+ return -1;
+
+ int sendStatus = 0;
+
+ /* NegotiateServerProtocol packet structure
+ *
+ * [ 'Y' | msgLength | min_version | max_version | param_list_len | list of param names ]
+ */
+
+ sendStatus = SendServerProtocolVersionMessage(port);
+
+ /* Ensure that the message buffer is flushed */
+ pq_flush();
+
+ return sendStatus;
+}
+
+/*
+ * Construct and send a ServerProtocolVersion message.
+ *
+ * Message contains:
+ * 1. minimum, maximum versions supported by the BE,
+ * 2. number of parameters that were honored by the BE from startup packet,
+ * 3. a list of strings consisting of the parameter names accepted by BE.
+ */
+int
+SendServerProtocolVersionMessage(Port *port)
+{
+ StringInfoData buf;
+
+ /* PG message type*/
+ pq_beginmessage(&buf, 'Y');
+
+ /* Protocol version numbers */
+ pq_sendint(&buf, PG_PROTOCOL_EARLIEST, sizeof(int32)); /* min */
+ pq_sendint(&buf, PG_PROTOCOL_LATEST, sizeof(int32)); /* max */
+
+ /* Length of parameter list; parameter list consists of (key, value) pairs */
+ pq_sendint(&buf, list_length(port->guc_options) / 2, sizeof(int32));
+
+ ListCell *gucopts = list_head(port->guc_options);
+ while (gucopts)
+ {
+ char *name;
+
+ /* First comes key, which we need. */
+ name = lfirst(gucopts);
+ gucopts = lnext(gucopts);
+
+ /* Then comes value, which we don't need. */
+ gucopts = lnext(gucopts);
+
+ pq_sendstring(&buf, name);
+ }
+
+ pq_endmessage(&buf);
+
+ return 0;
+}
+
+/*
* Postmaster main entry point
*/
void
@@ -2050,20 +2122,6 @@ retry1:
*/
FrontendProtocol = proto;
- /* Check we can handle the protocol the frontend is using. */
-
- if (PG_PROTOCOL_MAJOR(proto) < PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST) ||
- PG_PROTOCOL_MAJOR(proto) > PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) ||
- (PG_PROTOCOL_MAJOR(proto) == PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) &&
- PG_PROTOCOL_MINOR(proto) > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST)))
- ereport(FATAL,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("unsupported frontend protocol %u.%u: server supports %u.0 to %u.%u",
- PG_PROTOCOL_MAJOR(proto), PG_PROTOCOL_MINOR(proto),
- PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST),
- PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST),
- PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST))));
-
/*
* Now fetch parameters out of startup packet and save them into the Port
* structure. All data structures attached to the Port struct must be
@@ -2145,9 +2203,35 @@ retry1:
ereport(FATAL,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("invalid startup packet layout: expected terminator as last byte")));
+
+ /*
+ * Need to negotiate pgwire protocol if
+ * 1. FE version is not the same as BE version
+ * 2. FE version is not 3.0
+ */
+ if (FrontendProtocol != PG_PROTOCOL_LATEST
+ && FrontendProtocol != PG_PROTOCOL(3, 0))
+ {
+ /* Negotiate parameters after all the error-checking is done */
+ if (NegotiateServerProtocol(port))
+ return STATUS_ERROR;
+ }
}
else
{
+ /* Check we can handle the protocol the frontend is using. */
+ if (PG_PROTOCOL_MAJOR(proto) < PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST) ||
+ PG_PROTOCOL_MAJOR(proto) > PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) ||
+ (PG_PROTOCOL_MAJOR(proto) == PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) &&
+ PG_PROTOCOL_MINOR(proto) > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST)))
+ ereport(FATAL,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("unsupported frontend protocol %u.%u: server supports %u.0 to %u.%u",
+ PG_PROTOCOL_MAJOR(proto), PG_PROTOCOL_MINOR(proto),
+ PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST),
+ PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST),
+ PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST))));
+
/*
* Get the parameters from the old-style, fixed-width-fields startup
* packet as C strings. The packet destination was cleared first so a
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 246fea8693..69441f3f86 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3961,6 +3961,7 @@ static int GUCNestLevel = 0; /* 1 when in main transaction */
static int guc_var_compare(const void *a, const void *b);
static int guc_name_compare(const char *namea, const char *nameb);
+static bool is_optional(const char *guc_name);
static void InitializeGUCOptionsFromEnvironment(void);
static void InitializeOneGUCOption(struct config_generic *gconf);
static void push_old_value(struct config_generic *gconf, GucAction action);
@@ -4416,7 +4417,7 @@ find_option(const char *name, bool create_placeholders, int elevel)
/*
* Check if the name is qualified, and if so, add a placeholder.
*/
- if (strchr(name, GUC_QUALIFIER_SEPARATOR) != NULL)
+ if (strchr(name, GUC_QUALIFIER_SEPARATOR) != NULL || is_optional(name))
return add_placeholder_variable(name, elevel);
}
@@ -4467,6 +4468,25 @@ guc_name_compare(const char *namea, const char *nameb)
return 0;
}
+/*
+ * A GUC variable is deemed optional if the name contains "_pq_" as a proper prefix.
+ *
+ * It takes the whole struct as input in case we want to move away from name-based
+ * tagging of optional variables.
+ */
+bool
+is_optional(const char *guc_name)
+{
+ const char *optionalPrefix = "_pq_";
+ bool isOptional = false;
+
+ /* "_pq_" must be a proper prefix of the guc name in all encodings */
+ if (guc_name_compare(guc_name, optionalPrefix) == 1 &&
+ strstr(guc_name, optionalPrefix))
+ isOptional = true;
+
+ return isOptional;
+}
/*
* Initialize GUC options during program startup.
diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h
index 0f85908b09..da0a3a79c3 100644
--- a/src/include/postmaster/postmaster.h
+++ b/src/include/postmaster/postmaster.h
@@ -13,6 +13,10 @@
#ifndef _POSTMASTER_H
#define _POSTMASTER_H
+#include "postgres.h"
+
+#include "libpq/libpq-be.h"
+
/* GUC options */
extern bool EnableSSL;
extern int ReservedBackends;
@@ -46,6 +50,9 @@ extern int postmaster_alive_fds[2];
extern const char *progname;
+extern int NegotiateServerProtocol(Port *port);
+extern int SendServerProtocolVersionMessage(Port *port);
+
extern void PostmasterMain(int argc, char *argv[]) pg_attribute_noreturn();
extern void ClosePostmasterPorts(bool am_syslogger);
--
2.13.2.windows.1
[application/octet-stream] fe_pgwire3.1.patch (21.4K, ../../BN6PR21MB0772FFA0CBD298B76017744CD1730@BN6PR21MB0772.namprd21.prod.outlook.com/4-fe_pgwire3.1.patch)
download | inline diff:
From bf371b501580cfa300c5198cc11ac67acc7631c3 Mon Sep 17 00:00:00 2001
From: Badrul Chowdhury <bachow@microsoft.com>
Date: Mon, 25 Sep 2017 13:42:44 -0700
Subject: [PATCH] =?UTF-8?q?Introducing=20pgwire=20v3.1:=201.=20adds=20supp?=
=?UTF-8?q?ort=20for=20backend=20(BE)=20to=20accept=20optional=20parameter?=
=?UTF-8?q?s,=20ie=20parameters=20that=20have=20=E2=80=9C=5Fpq=5F=E2=80=9D?=
=?UTF-8?q?=20as=20a=20proper=20prefix=20in=20their=20names=202.=20creates?=
=?UTF-8?q?=20data=20structure=20for=20passing=20in=20additional=20paramet?=
=?UTF-8?q?ers=20in=20FE,=20adding=20command=20line=20support=20is=20out?=
=?UTF-8?q?=20of=20scope=20of=20this=20item=203.=20enhances=20FE->BE=20pro?=
=?UTF-8?q?tocol=20negotiation:=20adds=20support=20for=20newer=20FE=20to?=
=?UTF-8?q?=20connect=20to=20older=20BE=20while=20maintaining=20back-compa?=
=?UTF-8?q?tibility,=20ie=20same=20FE=20<->=20BE,=20older=20FE=20to=20newe?=
=?UTF-8?q?r=20BE=20work=20as=20before?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
---
src/Makefile | 2 +-
src/Makefile.global.in | 2 +-
src/bin/pg_dump/pg_dump.c | 10 +--
src/bin/scripts/clusterdb.c | 2 +-
src/bin/scripts/createuser.c | 2 +-
src/bin/scripts/reindexdb.c | 6 +-
src/bin/scripts/vacuumdb.c | 4 +-
src/common/Makefile | 2 +-
src/fe_utils/Makefile | 2 +-
src/fe_utils/simple_list.c | 4 +
src/include/fe_utils/simple_list.h | 1 +
src/include/libpq/pqcomm.h | 2 +-
src/interfaces/libpq/fe-connect.c | 173 +++++++++++++++++++++++++++++++++++-
src/interfaces/libpq/fe-protocol3.c | 47 ++++++++--
src/interfaces/libpq/libpq-fe.h | 1 +
src/interfaces/libpq/libpq-int.h | 9 +-
src/tools/msvc/Mkvcbuild.pm | 2 +-
20 files changed, 372 insertions(+), 40 deletions(-)
diff --git a/src/Makefile b/src/Makefile
index 380da92c75..048d2d7ca8 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -20,10 +20,10 @@ SUBDIRS = \
backend/utils/mb/conversion_procs \
backend/snowball \
include \
+ fe_utils \
interfaces \
backend/replication/libpqwalreceiver \
backend/replication/pgoutput \
- fe_utils \
bin \
pl \
makefiles \
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index e8b3a519cb..c3b45ce650 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -480,7 +480,7 @@ endif
# This macro is for use by libraries linking to libpq. (Because libpgport
# isn't created with the same link flags as libpq, it can't be used.)
-libpq = -L$(libpq_builddir) -lpq
+libpq = -L$(libpq_builddir) -lpq -L$(top_builddir)/src/common -lpgcommon -L$(top_builddir)/src/fe_utils -lpgfeutils
# This macro is for use by client executables (not libraries) that use libpq.
# We force clients to pull symbols from the non-shared libraries libpgport
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 75f08cd792..9d3feb0a0a 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -110,16 +110,16 @@ static int strict_names = 0;
* The string lists record the patterns given by command-line switches,
* which we then convert to lists of OIDs of matching objects.
*/
-static SimpleStringList schema_include_patterns = {NULL, NULL};
+static SimpleStringList schema_include_patterns = {NULL, NULL, NULL};
static SimpleOidList schema_include_oids = {NULL, NULL};
-static SimpleStringList schema_exclude_patterns = {NULL, NULL};
+static SimpleStringList schema_exclude_patterns = {NULL, NULL, NULL};
static SimpleOidList schema_exclude_oids = {NULL, NULL};
-static SimpleStringList table_include_patterns = {NULL, NULL};
+static SimpleStringList table_include_patterns = {NULL, NULL, NULL};
static SimpleOidList table_include_oids = {NULL, NULL};
-static SimpleStringList table_exclude_patterns = {NULL, NULL};
+static SimpleStringList table_exclude_patterns = {NULL, NULL, NULL};
static SimpleOidList table_exclude_oids = {NULL, NULL};
-static SimpleStringList tabledata_exclude_patterns = {NULL, NULL};
+static SimpleStringList tabledata_exclude_patterns = {NULL, NULL, NULL};
static SimpleOidList tabledata_exclude_oids = {NULL, NULL};
diff --git a/src/bin/scripts/clusterdb.c b/src/bin/scripts/clusterdb.c
index a6640aa57b..79fa46afd2 100644
--- a/src/bin/scripts/clusterdb.c
+++ b/src/bin/scripts/clusterdb.c
@@ -60,7 +60,7 @@ main(int argc, char *argv[])
bool quiet = false;
bool alldb = false;
bool verbose = false;
- SimpleStringList tables = {NULL, NULL};
+ SimpleStringList tables = {NULL, NULL, NULL};
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pgscripts"));
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index 0e36edcc5d..ff33ec63c3 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -58,7 +58,7 @@ main(int argc, char *argv[])
char *host = NULL;
char *port = NULL;
char *username = NULL;
- SimpleStringList roles = {NULL, NULL};
+ SimpleStringList roles = {NULL, NULL, NULL};
enum trivalue prompt_password = TRI_DEFAULT;
bool echo = false;
bool interactive = false;
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index ffd611e7bb..2aede05156 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -68,9 +68,9 @@ main(int argc, char *argv[])
bool echo = false;
bool quiet = false;
bool verbose = false;
- SimpleStringList indexes = {NULL, NULL};
- SimpleStringList tables = {NULL, NULL};
- SimpleStringList schemas = {NULL, NULL};
+ SimpleStringList indexes = {NULL, NULL, NULL};
+ SimpleStringList tables = {NULL, NULL, NULL};
+ SimpleStringList schemas = {NULL, NULL, NULL};
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pgscripts"));
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 5d2869ea6b..df77eccf5f 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -123,7 +123,7 @@ main(int argc, char *argv[])
vacuumingOptions vacopts;
bool analyze_in_stages = false;
bool alldb = false;
- SimpleStringList tables = {NULL, NULL};
+ SimpleStringList tables = {NULL, NULL, NULL};
int concurrentCons = 1;
int tbl_count = 0;
@@ -342,7 +342,7 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
PGconn *conn;
SimpleStringListCell *cell;
ParallelSlot *slots = NULL;
- SimpleStringList dbtables = {NULL, NULL};
+ SimpleStringList dbtables = {NULL, NULL, NULL};
int i;
bool failed = false;
bool parallel = concurrentCons > 1;
diff --git a/src/common/Makefile b/src/common/Makefile
index 80e78d72fe..fa29c3e0ac 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -24,7 +24,7 @@ subdir = src/common
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
-override CPPFLAGS := -DFRONTEND $(CPPFLAGS)
+override CPPFLAGS := -DFRONTEND $(CPPFLAGS) -fPIC
LIBS += $(PTHREAD_LIBS)
# don't include subdirectory-path-dependent -I and -L switches
diff --git a/src/fe_utils/Makefile b/src/fe_utils/Makefile
index ebce38ceb4..7706bc5fa9 100644
--- a/src/fe_utils/Makefile
+++ b/src/fe_utils/Makefile
@@ -17,7 +17,7 @@ subdir = src/fe_utils
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
-override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS)
+override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS) -fPIC
OBJS = mbprint.o print.o psqlscan.o simple_list.o string_utils.o
diff --git a/src/fe_utils/simple_list.c b/src/fe_utils/simple_list.c
index 21a2e57297..0c26bead24 100644
--- a/src/fe_utils/simple_list.c
+++ b/src/fe_utils/simple_list.c
@@ -62,6 +62,10 @@ simple_oid_list_member(SimpleOidList *list, Oid val)
void
simple_string_list_append(SimpleStringList *list, const char *val)
{
+ /* Cannot append to immutable list */
+ if (list->is_immutable)
+ return;
+
SimpleStringListCell *cell;
cell = (SimpleStringListCell *)
diff --git a/src/include/fe_utils/simple_list.h b/src/include/fe_utils/simple_list.h
index 97bb34f191..ea5e9af7b4 100644
--- a/src/include/fe_utils/simple_list.h
+++ b/src/include/fe_utils/simple_list.h
@@ -41,6 +41,7 @@ typedef struct SimpleStringList
{
SimpleStringListCell *head;
SimpleStringListCell *tail;
+ bool is_immutable;
} SimpleStringList;
diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h
index 10c7434c41..3a8ea400ac 100644
--- a/src/include/libpq/pqcomm.h
+++ b/src/include/libpq/pqcomm.h
@@ -108,7 +108,7 @@ typedef struct
/* The earliest and latest frontend/backend protocol version supported. */
#define PG_PROTOCOL_EARLIEST PG_PROTOCOL(2,0)
-#define PG_PROTOCOL_LATEST PG_PROTOCOL(3,0)
+#define PG_PROTOCOL_LATEST PG_PROTOCOL(3,1)
typedef uint32 ProtocolVersion; /* FE/BE protocol version number */
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d0e97ecdd4..60ce6e797c 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -350,6 +350,11 @@ static const PQEnvironmentOption EnvironmentOptions[] =
static const char uri_designator[] = "postgresql://";
static const char short_uri_designator[] = "postgres://";
+static const char *optional_parameter_prefix = "_pq_";
+
+/* A list of string options to add as startup options */
+static SimpleStringList startup_parameters = {NULL, NULL, NULL};
+
static bool connectOptions1(PGconn *conn, const char *conninfo);
static bool connectOptions2(PGconn *conn);
static int connectDBStart(PGconn *conn);
@@ -2001,6 +2006,9 @@ PQconnectPoll(PGconn *conn)
int optval;
PQExpBufferData savedMessage;
+ /* Flag to check if newer FE is connecting to older BE. */
+ bool server_is_older = false;
+
if (conn == NULL)
return PGRES_POLLING_FAILED;
@@ -2018,6 +2026,7 @@ PQconnectPoll(PGconn *conn)
/* These are reading states */
case CONNECTION_AWAITING_RESPONSE:
+ case CONNECTION_NEGOTIATING:
case CONNECTION_AUTH_OK:
{
/* Load waiting data */
@@ -2462,7 +2471,8 @@ keep_going: /* We will come back to here until there is
*/
if (PG_PROTOCOL_MAJOR(conn->pversion) >= 3)
startpacket = pqBuildStartupPacket3(conn, &packetlen,
- EnvironmentOptions);
+ EnvironmentOptions,
+ &startup_parameters);
else
startpacket = pqBuildStartupPacket2(conn, &packetlen,
EnvironmentOptions);
@@ -2649,6 +2659,12 @@ keep_going: /* We will come back to here until there is
return PGRES_POLLING_READING;
}
+ if (beresp == 'Y')
+ {
+ conn->status = CONNECTION_NEGOTIATING;
+ goto keep_going;
+ }
+
/*
* Validate message type: we expect only an authentication
* request or an error here. Anything else probably means
@@ -2893,6 +2909,161 @@ keep_going: /* We will come back to here until there is
goto keep_going;
}
+ case CONNECTION_NEGOTIATING:
+ {
+ int minServerVersion; /* Min pgwire protocol supported by server */
+ int maxServerVersion; /* Max pgwire protocol supported by server */
+ int param_list_len; /* Length of list of parameters honored by server */
+ int proper_prefix_len; /* Length of required prefix in optional parameter name */
+ int i; /* Index for loop */
+ PQExpBuffer buf; /* Buffer for data */
+ char *rejected_param; /* Parameter that was rejected by the BE */
+ int originalMsgLen; /* Length of message sans msg type */
+ int runningMsgLength; /* Copy of originalMsgLen, will not be preserved */
+ int available; /* Bytes available in message body */
+
+ /* Mark 'Y' as consumed: 1 byte for message type */
+ conn->inCursor = conn->inStart + 1;
+
+ /*
+ * Block until message length is read.
+ *
+ * No need to account for 2.x fixed-length message because this state cannot
+ * be reached by pre-3.0 server.
+ */
+ if (pqGetInt(&originalMsgLen, sizeof(int32), conn))
+ return PGRES_POLLING_READING;
+
+ runningMsgLength = originalMsgLen;
+
+ /* Block until each of the fields in the packet is read */
+ if (pqGetInt(&minServerVersion, sizeof(int32), conn) ||
+ pqGetInt(&maxServerVersion, sizeof(int32), conn) ||
+ pqGetInt(¶m_list_len, sizeof(int32), conn))
+ {
+ return PGRES_POLLING_READING;
+ }
+
+ /* 4 bytes each for msgLength, min, max, and length of list of param names */
+ runningMsgLength -= sizeof(int32) * 4;
+
+ /* Create empty buffer */
+ buf = createPQExpBuffer();
+ for (i = 0; i < param_list_len; ++i)
+ {
+ if (pqGets(buf, conn))
+ {
+ available = conn->inEnd - conn->inCursor;
+ if (available < runningMsgLength)
+ {
+ /* Enlarge buffer if required */
+ if (pqCheckInBufferSpace(conn->inCursor + (size_t)runningMsgLength, conn))
+ goto error_return;
+
+ return PGRES_POLLING_READING;
+ }
+ else
+ {
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("encountered error while attempting "
+ "to read parameter name from connection\n"));
+ /* Free buffer */
+ destroyPQExpBuffer(buf);
+
+ goto error_return;
+ }
+ }
+
+ /* Read string successfully, decrement message length to reflect this */
+ runningMsgLength -= buf->len + 1; /* +1 for NULL */
+
+ simple_string_list_member(&startup_parameters, buf->data);
+
+ /* pqGets() resets the buffer, so buffer is clean for next iteration */
+ }
+
+ /* Free buffer */
+ destroyPQExpBuffer(buf);
+
+ /* Check for extraneous data in packet, +1 to account for message type char */
+ if (conn->inCursor != conn->inStart + 1 + originalMsgLen)
+ {
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("Extraneous data in ServerProtocolMessage packet\n"));
+ goto error_return;
+ }
+
+ /* Mark read data consumed */
+ conn->inStart = conn->inCursor;
+
+ /*
+ * Emit error if FE is older than min supported by BE.
+ *
+ * This check will be effective once the minimum BE version is >= 3.0;
+ * otherwise, older FEs will be turned away when parsing startup packet.
+ */
+ if (PG_PROTOCOL_MAJOR(conn->pversion) < PG_PROTOCOL_MAJOR(minServerVersion) ||
+ (PG_PROTOCOL_MAJOR(conn->pversion) == PG_PROTOCOL_MAJOR(minServerVersion) &&
+ PG_PROTOCOL_MINOR(conn->pversion) < PG_PROTOCOL_MINOR(minServerVersion)))
+ {
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("unsupported frontend protocol %u.%u: "
+ "server supports %u.%u to %u.%u"),
+ PG_PROTOCOL_MAJOR(conn->pversion),
+ PG_PROTOCOL_MINOR(conn->pversion),
+ PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST),
+ PG_PROTOCOL_MINOR(PG_PROTOCOL_EARLIEST),
+ PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST),
+ PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST));
+ goto error_return;
+ }
+
+ /* Set flag to enable parameter check if newer FE is connecting to older BE
+ * In this case, check if the non-optional parameters sent
+ * in StartupMsg by FE were accepted by the BE.
+ *
+ * If the FE version is the same as BE or it falls in the [min, max]
+ * range of BE, all the parameters sent by FE should be accepted by BE,
+ * so skip the accepted parameter check.
+ */
+ else if (PG_PROTOCOL_MAJOR(conn->pversion) > PG_PROTOCOL_MAJOR(maxServerVersion) ||
+ (PG_PROTOCOL_MAJOR(conn->pversion) == PG_PROTOCOL_MAJOR(maxServerVersion) &&
+ PG_PROTOCOL_MINOR(conn->pversion) > PG_PROTOCOL_MINOR(maxServerVersion)))
+ {
+ server_is_older = true;
+ }
+
+ /*
+ * Check whether all required parameters sent by newer FE were accepted by older BE.
+ * Do not error out if optional parameters were rejected by the BE.
+ */
+ while (server_is_older && (rejected_param = simple_string_list_not_touched(&startup_parameters)) != NULL)
+ {
+ /*
+ * Terminate connection if the rejected parameter is not optional.
+ * This is not encoding-aware, which is okay because it is all on client-side
+ * optional_parameter_prefix must be a proper prefix of the rejected paramteter's name.
+ */
+ proper_prefix_len = strlen(optional_parameter_prefix);
+ if (strlen(rejected_param) <= proper_prefix_len ||
+ strncmp(rejected_param, optional_parameter_prefix, proper_prefix_len) != 0)
+ {
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("Non-optional startup parameter \'%s\'"
+ "was rejected by server\n"),
+ rejected_param);
+ goto error_return;
+
+ }
+
+ /* Mark member as visited */
+ simple_string_list_member(&startup_parameters, rejected_param);
+ }
+
+ conn->status = CONNECTION_AWAITING_RESPONSE;
+ goto keep_going;
+ }
+
case CONNECTION_AUTH_OK:
{
/*
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index a484fe80a1..a3b841dc23 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -22,6 +22,12 @@
#include "mb/pg_wchar.h"
+/*
+ * Simple string list data structure to track
+ * startup parameters that were accepted.
+ */
+#include "fe_utils/simple_list.h"
+
#ifdef WIN32
#include "win32.h"
#else
@@ -54,7 +60,8 @@ static int getReadyForQuery(PGconn *conn);
static void reportErrorPosition(PQExpBuffer msg, const char *query,
int loc, int encoding);
static int build_startup_packet(const PGconn *conn, char *packet,
- const PQEnvironmentOption *options);
+ const PQEnvironmentOption *options,
+ SimpleStringList *startup_parameters);
/*
@@ -2116,15 +2123,16 @@ pqFunctionCall3(PGconn *conn, Oid fnid,
*/
char *
pqBuildStartupPacket3(PGconn *conn, int *packetlen,
- const PQEnvironmentOption *options)
+ const PQEnvironmentOption *options,
+ SimpleStringList *startup_parameters)
{
char *startpacket;
- *packetlen = build_startup_packet(conn, NULL, options);
+ *packetlen = build_startup_packet(conn, NULL, options, startup_parameters);
startpacket = (char *) malloc(*packetlen);
if (!startpacket)
return NULL;
- *packetlen = build_startup_packet(conn, startpacket, options);
+ *packetlen = build_startup_packet(conn, startpacket, options, startup_parameters);
return startpacket;
}
@@ -2139,11 +2147,15 @@ pqBuildStartupPacket3(PGconn *conn, int *packetlen,
*/
static int
build_startup_packet(const PGconn *conn, char *packet,
- const PQEnvironmentOption *options)
+ const PQEnvironmentOption *options,
+ SimpleStringList *startup_parameters)
{
int packet_len = 0;
const PQEnvironmentOption *next_eo;
const char *val;
+ int nCells = 0;
+ char *name, *value;
+ SimpleStringListCell *pCurr;
/* Protocol version comes first. */
if (packet)
@@ -2195,6 +2207,31 @@ build_startup_packet(const PGconn *conn, char *packet,
}
}
+ if (startup_parameters)
+ {
+ for (pCurr = startup_parameters->head; pCurr != NULL; pCurr = pCurr->next, ++nCells)
+ {
+ if ((nCells % 2) == 0)
+ name = pCurr->val;
+ else
+ {
+ value = pCurr->val;
+ ADD_STARTUP_OPTION(name, value);
+
+ /* Mark value consumed */
+ pCurr->touched = true;
+ }
+ }
+
+ if ((nCells % 2) != 0)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("at least one parameter specified in"
+ "StartupMessage does not have a value\n"));
+ pqSaveErrorResult(conn);
+ }
+ }
+
/* Add trailing terminator */
if (packet)
packet[packet_len] = '\0';
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 1d915e7915..d04f48544c 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -58,6 +58,7 @@ typedef enum
CONNECTION_MADE, /* Connection OK; waiting to send. */
CONNECTION_AWAITING_RESPONSE, /* Waiting for a response from the
* postmaster. */
+ CONNECTION_NEGOTIATING, /* Negotiating pgwire protocol between FE/BE */
CONNECTION_AUTH_OK, /* Received authentication; waiting for
* backend startup. */
CONNECTION_SETENV, /* Negotiating environment. */
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 42913604e3..10121075f2 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -43,6 +43,12 @@
/* include stuff found in fe only */
#include "pqexpbuffer.h"
+/*
+ * Simple string list data structure to track
+ * startup parameters that were accepted.
+ */
+#include "fe_utils/simple_list.h"
+
#ifdef ENABLE_GSS
#if defined(HAVE_GSSAPI_H)
#include <gssapi.h>
@@ -598,7 +604,8 @@ extern PGresult *pqFunctionCall2(PGconn *conn, Oid fnid,
/* === in fe-protocol3.c === */
extern char *pqBuildStartupPacket3(PGconn *conn, int *packetlen,
- const PQEnvironmentOption *options);
+ const PQEnvironmentOption *options,
+ SimpleStringList *parameters);
extern void pqParseInput3(PGconn *conn);
extern int pqGetErrorNotice3(PGconn *conn, bool isError);
extern void pqBuildErrorMessage3(PQExpBuffer msg, const PGresult *res,
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 686c7369f6..a440ca303f 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -234,7 +234,7 @@ sub mkvcbuild
$libpq->UseDef('src/interfaces/libpq/libpqdll.def');
$libpq->ReplaceFile('src/interfaces/libpq/libpqrc.c',
'src/interfaces/libpq/libpq.rc');
- $libpq->AddReference($libpgport);
+ $libpq->AddReference($libpgcommon, $libpgfeutils, $libpgport);
# The OBJS scraper doesn't know about ifdefs, so remove fe-secure-openssl.c
# and sha2_openssl.c if building without OpenSSL, and remove sha2.c if
--
2.13.2.windows.1
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-10-04 01:46 Tom Lane <tgl@sss.pgh.pa.us>
parent: Badrul Chowdhury <bachow@microsoft.com>
0 siblings, 1 reply; 24+ messages in thread
From: Tom Lane @ 2017-10-04 01:46 UTC (permalink / raw)
To: Badrul Chowdhury <bachow@microsoft.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
Badrul Chowdhury <bachow@microsoft.com> writes:
> 1. Pgwire protocol v3.0 with negotiation is called v3.1.
> 2. There are 2 patches for the change: a BE-specific patch that will be backported and a FE-specific patch that is only for pg10 and above.
TBH, anything that presupposes a backported change in the backend is
broken by definition. We expect libpq to be able to connect to older
servers, and that has to include servers that didn't get this memo.
It would be all right for libpq to make a second connection attempt
if its first one fails, as we did in the 2.0 -> 3.0 change.
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-10-04 11:54 Robert Haas <robertmhaas@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 24+ messages in thread
From: Robert Haas @ 2017-10-04 11:54 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Badrul Chowdhury <bachow@microsoft.com>; Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
On Tue, Oct 3, 2017 at 9:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Badrul Chowdhury <bachow@microsoft.com> writes:
>> 1. Pgwire protocol v3.0 with negotiation is called v3.1.
>> 2. There are 2 patches for the change: a BE-specific patch that will be backported and a FE-specific patch that is only for pg10 and above.
>
> TBH, anything that presupposes a backported change in the backend is
> broken by definition. We expect libpq to be able to connect to older
> servers, and that has to include servers that didn't get this memo.
>
> It would be all right for libpq to make a second connection attempt
> if its first one fails, as we did in the 2.0 -> 3.0 change.
Hmm, that's another approach, but I prefer the one advocated by Tom Lane.
https://www.postgresql.org/message-id/30788.1498672033@sss.pgh.pa.us
https://www.postgresql.org/message-id/24357.1498703265%40sss.pgh.pa.us
--
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-10-04 23:30 Badrul Chowdhury <bachow@microsoft.com>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 0 replies; 24+ messages in thread
From: Badrul Chowdhury @ 2017-10-04 23:30 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
Okay, I will add a mechanism to try connecting with 3.0 if 3.1 fails- that should be a few lines of code fe-connect.c; this will eliminate the need for a back-patch. What do you think of the rest of the change?
Thanks,
Badrul
-----Original Message-----
From: Robert Haas [mailto:robertmhaas@gmail.com]
Sent: Wednesday, October 4, 2017 4:54 AM
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Badrul Chowdhury <bachow@microsoft.com>; Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
On Tue, Oct 3, 2017 at 9:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Badrul Chowdhury <bachow@microsoft.com> writes:
>> 1. Pgwire protocol v3.0 with negotiation is called v3.1.
>> 2. There are 2 patches for the change: a BE-specific patch that will be backported and a FE-specific patch that is only for pg10 and above.
>
> TBH, anything that presupposes a backported change in the backend is
> broken by definition. We expect libpq to be able to connect to older
> servers, and that has to include servers that didn't get this memo.
>
> It would be all right for libpq to make a second connection attempt if
> its first one fails, as we did in the 2.0 -> 3.0 change.
Hmm, that's another approach, but I prefer the one advocated by Tom Lane.
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.postgresql.org%2Fmessage-id%2F3...
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.postgresql.org%2Fmessage-id%2F2...
--
Robert Haas
EnterpriseDB: https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.enterprisedb.com&data=02%7C0...
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-10-06 21:07 Badrul Chowdhury <bachow@microsoft.com>
parent: Robert Haas <robertmhaas@gmail.com>
1 sibling, 1 reply; 24+ messages in thread
From: Badrul Chowdhury @ 2017-10-06 21:07 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
Hi Tom and Robert,
I added a mechanism to fall back to v3.0 if the BE fails to start when FE initiates a connection with v3.1 (with optional startup parameters). This completely eliminates the need to backpatch older servers, ie newer FE can connect to older BE. Please let me know what you think.
Thanks,
Badrul
-----Original Message-----
From: Robert Haas [mailto:robertmhaas@gmail.com]
Sent: Wednesday, October 4, 2017 4:54 AM
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Badrul Chowdhury <bachow@microsoft.com>; Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
On Tue, Oct 3, 2017 at 9:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Badrul Chowdhury <bachow@microsoft.com> writes:
>> 1. Pgwire protocol v3.0 with negotiation is called v3.1.
>> 2. There are 2 patches for the change: a BE-specific patch that will be backported and a FE-specific patch that is only for pg10 and above.
>
> TBH, anything that presupposes a backported change in the backend is
> broken by definition. We expect libpq to be able to connect to older
> servers, and that has to include servers that didn't get this memo.
>
> It would be all right for libpq to make a second connection attempt if
> its first one fails, as we did in the 2.0 -> 3.0 change.
Hmm, that's another approach, but I prefer the one advocated by Tom Lane.
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.postgresql.org%2Fmessage-id%2F3...
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.postgresql.org%2Fmessage-id%2F2...
--
Robert Haas
EnterpriseDB: https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.enterprisedb.com&data=02%7C0...
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
Attachments:
[application/octet-stream] pgwire3.1.patch (32.2K, ../../MWHPR21MB0783B10F0EDA603553274F7DD1710@MWHPR21MB0783.namprd21.prod.outlook.com/2-pgwire3.1.patch)
download | inline diff:
From 442f54a04fa814fb99f3bf46dc66060d1c58555e Mon Sep 17 00:00:00 2001
From: Badrul Chowdhury <bachow@microsoft.com>
Date: Mon, 25 Sep 2017 13:42:44 -0700
Subject: [PATCH] =?UTF-8?q?Introducing=20pgwire=20v3.1:=201.=20adds=20supp?=
=?UTF-8?q?ort=20for=20backend=20(BE)=20to=20accept=20optional=20parameter?=
=?UTF-8?q?s,=20ie=20parameters=20that=20have=20=E2=80=9C=5Fpq=5F=E2=80=9D?=
=?UTF-8?q?=20as=20a=20proper=20prefix=20in=20their=20names=202.=20creates?=
=?UTF-8?q?=20data=20structure=20for=20passing=20in=20additional=20paramet?=
=?UTF-8?q?ers=20in=20FE,=20adding=20command=20line=20support=20is=20out?=
=?UTF-8?q?=20of=20scope=20of=20this=20item=203.=20enhances=20FE->BE=20pro?=
=?UTF-8?q?tocol=20negotiation:=20adds=20support=20for=20newer=20FE=20to?=
=?UTF-8?q?=20connect=20to=20older=20BE=20while=20maintaining=20back-compa?=
=?UTF-8?q?tibility,=20ie=20same=20FE=20<->=20BE,=20older=20FE=20to=20newe?=
=?UTF-8?q?r=20BE=20work=20as=20before=204.=20adds=20support=20for=20newer?=
=?UTF-8?q?=20FE=20to=20connect=20to=20old=20BE=20that=20doesn't=20support?=
=?UTF-8?q?=20protocol=20negotiation:=20FE=20downgrades=20to=20v3.0=20if?=
=?UTF-8?q?=20sending=20startup=20parameters=20fails?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
---
src/Makefile | 2 +-
src/Makefile.global.in | 2 +-
src/backend/postmaster/postmaster.c | 112 +++++++++++++++---
src/backend/utils/misc/guc.c | 22 +++-
src/bin/pg_dump/pg_dump.c | 10 +-
src/bin/scripts/clusterdb.c | 2 +-
src/bin/scripts/createuser.c | 2 +-
src/bin/scripts/reindexdb.c | 6 +-
src/bin/scripts/vacuumdb.c | 4 +-
src/common/Makefile | 2 +-
src/fe_utils/Makefile | 2 +-
src/fe_utils/simple_list.c | 4 +
src/include/fe_utils/simple_list.h | 1 +
src/include/libpq/pqcomm.h | 2 +-
src/include/postmaster/postmaster.h | 7 ++
src/interfaces/libpq/fe-connect.c | 218 ++++++++++++++++++++++++++++++++++--
src/interfaces/libpq/fe-protocol3.c | 42 ++++++-
src/interfaces/libpq/libpq-fe.h | 1 +
src/interfaces/libpq/libpq-int.h | 10 +-
src/tools/msvc/Mkvcbuild.pm | 2 +-
20 files changed, 404 insertions(+), 49 deletions(-)
diff --git a/src/Makefile b/src/Makefile
index 380da92c75..048d2d7ca8 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -20,10 +20,10 @@ SUBDIRS = \
backend/utils/mb/conversion_procs \
backend/snowball \
include \
+ fe_utils \
interfaces \
backend/replication/libpqwalreceiver \
backend/replication/pgoutput \
- fe_utils \
bin \
pl \
makefiles \
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index e8b3a519cb..c3b45ce650 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -480,7 +480,7 @@ endif
# This macro is for use by libraries linking to libpq. (Because libpgport
# isn't created with the same link flags as libpq, it can't be used.)
-libpq = -L$(libpq_builddir) -lpq
+libpq = -L$(libpq_builddir) -lpq -L$(top_builddir)/src/common -lpgcommon -L$(top_builddir)/src/fe_utils -lpgfeutils
# This macro is for use by client executables (not libraries) that use libpq.
# We force clients to pull symbols from the non-shared libraries libpgport
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 95180b2ef5..21d77333be 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -103,6 +103,7 @@
#include "lib/ilist.h"
#include "libpq/auth.h"
#include "libpq/libpq.h"
+#include "libpq/pqformat.h"
#include "libpq/pqsignal.h"
#include "miscadmin.h"
#include "pg_getopt.h"
@@ -567,6 +568,77 @@ HANDLE PostmasterHandle;
#endif
/*
+ * Initiate protocol negotiation phase;
+ * protocol negotiation is only supported for pgwire version 3.x, x>0.
+ *
+ * Ensure that the packet write buffer is flushed.
+ */
+int
+NegotiateServerProtocol(Port *port)
+{
+ if (whereToSendOutput != DestRemote ||
+ PG_PROTOCOL_MAJOR(FrontendProtocol) < 3)
+ return -1;
+
+ int sendStatus = 0;
+
+ /* NegotiateServerProtocol packet structure
+ *
+ * [ 'Y' | msgLength | min_version | max_version | param_list_len | list of param names ]
+ */
+
+ sendStatus = SendServerProtocolVersionMessage(port);
+
+ /* Ensure that the message buffer is flushed */
+ pq_flush();
+
+ return sendStatus;
+}
+
+/*
+ * Construct and send a ServerProtocolVersion message.
+ *
+ * Message contains:
+ * 1. minimum, maximum versions supported by the BE,
+ * 2. number of parameters that were honored by the BE from startup packet,
+ * 3. a list of strings consisting of the parameter names accepted by BE.
+ */
+int
+SendServerProtocolVersionMessage(Port *port)
+{
+ StringInfoData buf;
+
+ /* PG message type*/
+ pq_beginmessage(&buf, 'Y');
+
+ /* Protocol version numbers */
+ pq_sendint(&buf, PG_PROTOCOL_EARLIEST, sizeof(int32)); /* min */
+ pq_sendint(&buf, PG_PROTOCOL_LATEST, sizeof(int32)); /* max */
+
+ /* Length of parameter list; parameter list consists of (key, value) pairs */
+ pq_sendint(&buf, list_length(port->guc_options) / 2, sizeof(int32));
+
+ ListCell *gucopts = list_head(port->guc_options);
+ while (gucopts)
+ {
+ char *name;
+
+ /* First comes key, which we need. */
+ name = lfirst(gucopts);
+ gucopts = lnext(gucopts);
+
+ /* Then comes value, which we don't need. */
+ gucopts = lnext(gucopts);
+
+ pq_sendstring(&buf, name);
+ }
+
+ pq_endmessage(&buf);
+
+ return 0;
+}
+
+/*
* Postmaster main entry point
*/
void
@@ -2050,20 +2122,6 @@ retry1:
*/
FrontendProtocol = proto;
- /* Check we can handle the protocol the frontend is using. */
-
- if (PG_PROTOCOL_MAJOR(proto) < PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST) ||
- PG_PROTOCOL_MAJOR(proto) > PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) ||
- (PG_PROTOCOL_MAJOR(proto) == PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) &&
- PG_PROTOCOL_MINOR(proto) > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST)))
- ereport(FATAL,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("unsupported frontend protocol %u.%u: server supports %u.0 to %u.%u",
- PG_PROTOCOL_MAJOR(proto), PG_PROTOCOL_MINOR(proto),
- PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST),
- PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST),
- PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST))));
-
/*
* Now fetch parameters out of startup packet and save them into the Port
* structure. All data structures attached to the Port struct must be
@@ -2145,9 +2203,35 @@ retry1:
ereport(FATAL,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("invalid startup packet layout: expected terminator as last byte")));
+
+ /*
+ * Need to negotiate pgwire protocol if
+ * 1. FE version is not the same as BE version
+ * 2. FE version is not 3.0
+ */
+ if (FrontendProtocol != PG_PROTOCOL_LATEST
+ && FrontendProtocol != PG_PROTOCOL(3, 0))
+ {
+ /* Negotiate parameters after all the error-checking is done */
+ if (NegotiateServerProtocol(port))
+ return STATUS_ERROR;
+ }
}
else
{
+ /* Check we can handle the protocol the frontend is using. */
+ if (PG_PROTOCOL_MAJOR(proto) < PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST) ||
+ PG_PROTOCOL_MAJOR(proto) > PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) ||
+ (PG_PROTOCOL_MAJOR(proto) == PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) &&
+ PG_PROTOCOL_MINOR(proto) > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST)))
+ ereport(FATAL,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("unsupported frontend protocol %u.%u: server supports %u.0 to %u.%u",
+ PG_PROTOCOL_MAJOR(proto), PG_PROTOCOL_MINOR(proto),
+ PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST),
+ PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST),
+ PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST))));
+
/*
* Get the parameters from the old-style, fixed-width-fields startup
* packet as C strings. The packet destination was cleared first so a
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 246fea8693..69441f3f86 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3961,6 +3961,7 @@ static int GUCNestLevel = 0; /* 1 when in main transaction */
static int guc_var_compare(const void *a, const void *b);
static int guc_name_compare(const char *namea, const char *nameb);
+static bool is_optional(const char *guc_name);
static void InitializeGUCOptionsFromEnvironment(void);
static void InitializeOneGUCOption(struct config_generic *gconf);
static void push_old_value(struct config_generic *gconf, GucAction action);
@@ -4416,7 +4417,7 @@ find_option(const char *name, bool create_placeholders, int elevel)
/*
* Check if the name is qualified, and if so, add a placeholder.
*/
- if (strchr(name, GUC_QUALIFIER_SEPARATOR) != NULL)
+ if (strchr(name, GUC_QUALIFIER_SEPARATOR) != NULL || is_optional(name))
return add_placeholder_variable(name, elevel);
}
@@ -4467,6 +4468,25 @@ guc_name_compare(const char *namea, const char *nameb)
return 0;
}
+/*
+ * A GUC variable is deemed optional if the name contains "_pq_" as a proper prefix.
+ *
+ * It takes the whole struct as input in case we want to move away from name-based
+ * tagging of optional variables.
+ */
+bool
+is_optional(const char *guc_name)
+{
+ const char *optionalPrefix = "_pq_";
+ bool isOptional = false;
+
+ /* "_pq_" must be a proper prefix of the guc name in all encodings */
+ if (guc_name_compare(guc_name, optionalPrefix) == 1 &&
+ strstr(guc_name, optionalPrefix))
+ isOptional = true;
+
+ return isOptional;
+}
/*
* Initialize GUC options during program startup.
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 75f08cd792..9d3feb0a0a 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -110,16 +110,16 @@ static int strict_names = 0;
* The string lists record the patterns given by command-line switches,
* which we then convert to lists of OIDs of matching objects.
*/
-static SimpleStringList schema_include_patterns = {NULL, NULL};
+static SimpleStringList schema_include_patterns = {NULL, NULL, NULL};
static SimpleOidList schema_include_oids = {NULL, NULL};
-static SimpleStringList schema_exclude_patterns = {NULL, NULL};
+static SimpleStringList schema_exclude_patterns = {NULL, NULL, NULL};
static SimpleOidList schema_exclude_oids = {NULL, NULL};
-static SimpleStringList table_include_patterns = {NULL, NULL};
+static SimpleStringList table_include_patterns = {NULL, NULL, NULL};
static SimpleOidList table_include_oids = {NULL, NULL};
-static SimpleStringList table_exclude_patterns = {NULL, NULL};
+static SimpleStringList table_exclude_patterns = {NULL, NULL, NULL};
static SimpleOidList table_exclude_oids = {NULL, NULL};
-static SimpleStringList tabledata_exclude_patterns = {NULL, NULL};
+static SimpleStringList tabledata_exclude_patterns = {NULL, NULL, NULL};
static SimpleOidList tabledata_exclude_oids = {NULL, NULL};
diff --git a/src/bin/scripts/clusterdb.c b/src/bin/scripts/clusterdb.c
index a6640aa57b..79fa46afd2 100644
--- a/src/bin/scripts/clusterdb.c
+++ b/src/bin/scripts/clusterdb.c
@@ -60,7 +60,7 @@ main(int argc, char *argv[])
bool quiet = false;
bool alldb = false;
bool verbose = false;
- SimpleStringList tables = {NULL, NULL};
+ SimpleStringList tables = {NULL, NULL, NULL};
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pgscripts"));
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index 0e36edcc5d..ff33ec63c3 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -58,7 +58,7 @@ main(int argc, char *argv[])
char *host = NULL;
char *port = NULL;
char *username = NULL;
- SimpleStringList roles = {NULL, NULL};
+ SimpleStringList roles = {NULL, NULL, NULL};
enum trivalue prompt_password = TRI_DEFAULT;
bool echo = false;
bool interactive = false;
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index ffd611e7bb..2aede05156 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -68,9 +68,9 @@ main(int argc, char *argv[])
bool echo = false;
bool quiet = false;
bool verbose = false;
- SimpleStringList indexes = {NULL, NULL};
- SimpleStringList tables = {NULL, NULL};
- SimpleStringList schemas = {NULL, NULL};
+ SimpleStringList indexes = {NULL, NULL, NULL};
+ SimpleStringList tables = {NULL, NULL, NULL};
+ SimpleStringList schemas = {NULL, NULL, NULL};
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pgscripts"));
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 5d2869ea6b..df77eccf5f 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -123,7 +123,7 @@ main(int argc, char *argv[])
vacuumingOptions vacopts;
bool analyze_in_stages = false;
bool alldb = false;
- SimpleStringList tables = {NULL, NULL};
+ SimpleStringList tables = {NULL, NULL, NULL};
int concurrentCons = 1;
int tbl_count = 0;
@@ -342,7 +342,7 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
PGconn *conn;
SimpleStringListCell *cell;
ParallelSlot *slots = NULL;
- SimpleStringList dbtables = {NULL, NULL};
+ SimpleStringList dbtables = {NULL, NULL, NULL};
int i;
bool failed = false;
bool parallel = concurrentCons > 1;
diff --git a/src/common/Makefile b/src/common/Makefile
index 80e78d72fe..fa29c3e0ac 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -24,7 +24,7 @@ subdir = src/common
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
-override CPPFLAGS := -DFRONTEND $(CPPFLAGS)
+override CPPFLAGS := -DFRONTEND $(CPPFLAGS) -fPIC
LIBS += $(PTHREAD_LIBS)
# don't include subdirectory-path-dependent -I and -L switches
diff --git a/src/fe_utils/Makefile b/src/fe_utils/Makefile
index ebce38ceb4..7706bc5fa9 100644
--- a/src/fe_utils/Makefile
+++ b/src/fe_utils/Makefile
@@ -17,7 +17,7 @@ subdir = src/fe_utils
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
-override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS)
+override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS) -fPIC
OBJS = mbprint.o print.o psqlscan.o simple_list.o string_utils.o
diff --git a/src/fe_utils/simple_list.c b/src/fe_utils/simple_list.c
index 21a2e57297..ed67f254f3 100644
--- a/src/fe_utils/simple_list.c
+++ b/src/fe_utils/simple_list.c
@@ -64,6 +64,10 @@ simple_string_list_append(SimpleStringList *list, const char *val)
{
SimpleStringListCell *cell;
+ /* Cannot append to immutable list */
+ if (list->is_immutable)
+ return;
+
cell = (SimpleStringListCell *)
pg_malloc(offsetof(SimpleStringListCell, val) + strlen(val) + 1);
diff --git a/src/include/fe_utils/simple_list.h b/src/include/fe_utils/simple_list.h
index 97bb34f191..ea5e9af7b4 100644
--- a/src/include/fe_utils/simple_list.h
+++ b/src/include/fe_utils/simple_list.h
@@ -41,6 +41,7 @@ typedef struct SimpleStringList
{
SimpleStringListCell *head;
SimpleStringListCell *tail;
+ bool is_immutable;
} SimpleStringList;
diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h
index 10c7434c41..3a8ea400ac 100644
--- a/src/include/libpq/pqcomm.h
+++ b/src/include/libpq/pqcomm.h
@@ -108,7 +108,7 @@ typedef struct
/* The earliest and latest frontend/backend protocol version supported. */
#define PG_PROTOCOL_EARLIEST PG_PROTOCOL(2,0)
-#define PG_PROTOCOL_LATEST PG_PROTOCOL(3,0)
+#define PG_PROTOCOL_LATEST PG_PROTOCOL(3,1)
typedef uint32 ProtocolVersion; /* FE/BE protocol version number */
diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h
index 0f85908b09..da0a3a79c3 100644
--- a/src/include/postmaster/postmaster.h
+++ b/src/include/postmaster/postmaster.h
@@ -13,6 +13,10 @@
#ifndef _POSTMASTER_H
#define _POSTMASTER_H
+#include "postgres.h"
+
+#include "libpq/libpq-be.h"
+
/* GUC options */
extern bool EnableSSL;
extern int ReservedBackends;
@@ -46,6 +50,9 @@ extern int postmaster_alive_fds[2];
extern const char *progname;
+extern int NegotiateServerProtocol(Port *port);
+extern int SendServerProtocolVersionMessage(Port *port);
+
extern void PostmasterMain(int argc, char *argv[]) pg_attribute_noreturn();
extern void ClosePostmasterPorts(bool am_syslogger);
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d0e97ecdd4..db22fca57a 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -86,8 +86,11 @@ static int ldapServiceLookup(const char *purl, PQconninfoOption *options,
* application_name in a startup packet. We hard-wire the value rather
* than looking into errcodes.h since it reflects historical behavior
* rather than that of the current code.
+ *
+ * Note that application_name is a startup parameter.
+ * The same error code is used for all unrecognized startup parameters.
*/
-#define ERRCODE_APPNAME_UNKNOWN "42704"
+#define ERRCODE_PARAMETER_UNKNOWN "42704"
/* This is part of the protocol so just define it */
#define ERRCODE_INVALID_PASSWORD "28P01"
@@ -350,6 +353,11 @@ static const PQEnvironmentOption EnvironmentOptions[] =
static const char uri_designator[] = "postgresql://";
static const char short_uri_designator[] = "postgres://";
+static const char *optional_parameter_prefix = "_pq_";
+
+/* A list of string options to add as startup options */
+static SimpleStringList startup_parameters = {NULL, NULL, NULL};
+
static bool connectOptions1(PGconn *conn, const char *conninfo);
static bool connectOptions2(PGconn *conn);
static int connectDBStart(PGconn *conn);
@@ -1803,8 +1811,9 @@ connectDBStart(PGconn *conn)
*/
conn->whichhost = 0;
conn->addr_cur = conn->connhost[0].addrlist;
- conn->pversion = PG_PROTOCOL(3, 0);
+ conn->pversion = PG_PROTOCOL(3, 1);
conn->send_appname = true;
+ conn->send_startup_params = true;
conn->status = CONNECTION_NEEDED;
/*
@@ -2001,6 +2010,9 @@ PQconnectPoll(PGconn *conn)
int optval;
PQExpBufferData savedMessage;
+ /* Flag to check if newer FE is connecting to older BE. */
+ bool server_is_older = false;
+
if (conn == NULL)
return PGRES_POLLING_FAILED;
@@ -2018,6 +2030,7 @@ PQconnectPoll(PGconn *conn)
/* These are reading states */
case CONNECTION_AWAITING_RESPONSE:
+ case CONNECTION_NEGOTIATING:
case CONNECTION_AUTH_OK:
{
/* Load waiting data */
@@ -2462,7 +2475,8 @@ keep_going: /* We will come back to here until there is
*/
if (PG_PROTOCOL_MAJOR(conn->pversion) >= 3)
startpacket = pqBuildStartupPacket3(conn, &packetlen,
- EnvironmentOptions);
+ EnvironmentOptions,
+ &startup_parameters);
else
startpacket = pqBuildStartupPacket2(conn, &packetlen,
EnvironmentOptions);
@@ -2649,6 +2663,12 @@ keep_going: /* We will come back to here until there is
return PGRES_POLLING_READING;
}
+ if (beresp == 'Y')
+ {
+ conn->status = CONNECTION_NEGOTIATING;
+ goto keep_going;
+ }
+
/*
* Validate message type: we expect only an authentication
* request or an error here. Anything else probably means
@@ -2716,10 +2736,19 @@ keep_going: /* We will come back to here until there is
appendPQExpBufferChar(&conn->errorMessage, '\n');
/*
- * If we tried to open the connection in 3.0 protocol,
- * fall back to 2.0 protocol.
+ * If we tried to open the connection in 3.1 protocol,
+ * fall back to 3.0 protocol; fall back to 2.0 from 3.0.
*/
- if (PG_PROTOCOL_MAJOR(conn->pversion) >= 3)
+ if (PG_PROTOCOL_MAJOR(conn->pversion) >= 3 &&
+ PG_PROTOCOL_MINOR(conn->pversion) >= 1)
+ {
+ conn->pversion = PG_PROTOCOL(3, 0);
+ /* Must drop the old connection */
+ pqDropConnection(conn, true);
+ conn->status = CONNECTION_NEEDED;
+ goto keep_going;
+ }
+ else if (PG_PROTOCOL_MAJOR(conn->pversion) >= 3)
{
conn->pversion = PG_PROTOCOL(2, 0);
/* Must drop the old connection */
@@ -2893,6 +2922,161 @@ keep_going: /* We will come back to here until there is
goto keep_going;
}
+ case CONNECTION_NEGOTIATING:
+ {
+ int minServerVersion; /* Min pgwire protocol supported by server */
+ int maxServerVersion; /* Max pgwire protocol supported by server */
+ int param_list_len; /* Length of list of parameters honored by server */
+ int proper_prefix_len; /* Length of required prefix in optional parameter name */
+ int i; /* Index for loop */
+ PQExpBuffer buf; /* Buffer for data */
+ char *rejected_param; /* Parameter that was rejected by the BE */
+ int originalMsgLen; /* Length of message sans msg type */
+ int runningMsgLength; /* Copy of originalMsgLen, will not be preserved */
+ int available; /* Bytes available in message body */
+
+ /* Mark 'Y' as consumed: 1 byte for message type */
+ conn->inCursor = conn->inStart + 1;
+
+ /*
+ * Block until message length is read.
+ *
+ * No need to account for 2.x fixed-length message because this state cannot
+ * be reached by pre-3.0 server.
+ */
+ if (pqGetInt(&originalMsgLen, sizeof(int32), conn))
+ return PGRES_POLLING_READING;
+
+ runningMsgLength = originalMsgLen;
+
+ /* Block until each of the fields in the packet is read */
+ if (pqGetInt(&minServerVersion, sizeof(int32), conn) ||
+ pqGetInt(&maxServerVersion, sizeof(int32), conn) ||
+ pqGetInt(¶m_list_len, sizeof(int32), conn))
+ {
+ return PGRES_POLLING_READING;
+ }
+
+ /* 4 bytes each for msgLength, min, max, and length of list of param names */
+ runningMsgLength -= sizeof(int32) * 4;
+
+ /* Create empty buffer */
+ buf = createPQExpBuffer();
+ for (i = 0; i < param_list_len; ++i)
+ {
+ if (pqGets(buf, conn))
+ {
+ available = conn->inEnd - conn->inCursor;
+ if (available < runningMsgLength)
+ {
+ /* Enlarge buffer if required */
+ if (pqCheckInBufferSpace(conn->inCursor + (size_t)runningMsgLength, conn))
+ goto error_return;
+
+ return PGRES_POLLING_READING;
+ }
+ else
+ {
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("encountered error while attempting "
+ "to read parameter name from connection\n"));
+ /* Free buffer */
+ destroyPQExpBuffer(buf);
+
+ goto error_return;
+ }
+ }
+
+ /* Read string successfully, decrement message length to reflect this */
+ runningMsgLength -= buf->len + 1; /* +1 for NULL */
+
+ simple_string_list_member(&startup_parameters, buf->data);
+
+ /* pqGets() resets the buffer, so buffer is clean for next iteration */
+ }
+
+ /* Free buffer */
+ destroyPQExpBuffer(buf);
+
+ /* Check for extraneous data in packet, +1 to account for message type char */
+ if (conn->inCursor != conn->inStart + 1 + originalMsgLen)
+ {
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("Extraneous data in ServerProtocolMessage packet\n"));
+ goto error_return;
+ }
+
+ /* Mark read data consumed */
+ conn->inStart = conn->inCursor;
+
+ /*
+ * Emit error if FE is older than min supported by BE.
+ *
+ * This check will be effective once the minimum BE version is >= 3.0;
+ * otherwise, older FEs will be turned away when parsing startup packet.
+ */
+ if (PG_PROTOCOL_MAJOR(conn->pversion) < PG_PROTOCOL_MAJOR(minServerVersion) ||
+ (PG_PROTOCOL_MAJOR(conn->pversion) == PG_PROTOCOL_MAJOR(minServerVersion) &&
+ PG_PROTOCOL_MINOR(conn->pversion) < PG_PROTOCOL_MINOR(minServerVersion)))
+ {
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("unsupported frontend protocol %u.%u: "
+ "server supports %u.%u to %u.%u"),
+ PG_PROTOCOL_MAJOR(conn->pversion),
+ PG_PROTOCOL_MINOR(conn->pversion),
+ PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST),
+ PG_PROTOCOL_MINOR(PG_PROTOCOL_EARLIEST),
+ PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST),
+ PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST));
+ goto error_return;
+ }
+
+ /* Set flag to enable parameter check if newer FE is connecting to older BE
+ * In this case, check if the non-optional parameters sent
+ * in StartupMsg by FE were accepted by the BE.
+ *
+ * If the FE version is the same as BE or it falls in the [min, max]
+ * range of BE, all the parameters sent by FE should be accepted by BE,
+ * so skip the accepted parameter check.
+ */
+ else if (PG_PROTOCOL_MAJOR(conn->pversion) > PG_PROTOCOL_MAJOR(maxServerVersion) ||
+ (PG_PROTOCOL_MAJOR(conn->pversion) == PG_PROTOCOL_MAJOR(maxServerVersion) &&
+ PG_PROTOCOL_MINOR(conn->pversion) > PG_PROTOCOL_MINOR(maxServerVersion)))
+ {
+ server_is_older = true;
+ }
+
+ /*
+ * Check whether all required parameters sent by newer FE were accepted by older BE.
+ * Do not error out if optional parameters were rejected by the BE.
+ */
+ while (server_is_older && (rejected_param = simple_string_list_not_touched(&startup_parameters)) != NULL)
+ {
+ /*
+ * Terminate connection if the rejected parameter is not optional.
+ * This is not encoding-aware, which is okay because it is all on client-side
+ * optional_parameter_prefix must be a proper prefix of the rejected paramteter's name.
+ */
+ proper_prefix_len = strlen(optional_parameter_prefix);
+ if (strlen(rejected_param) <= proper_prefix_len ||
+ strncmp(rejected_param, optional_parameter_prefix, proper_prefix_len) != 0)
+ {
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("Non-optional startup parameter \'%s\'"
+ "was rejected by server\n"),
+ rejected_param);
+ goto error_return;
+
+ }
+
+ /* Mark member as visited */
+ simple_string_list_member(&startup_parameters, rejected_param);
+ }
+
+ conn->status = CONNECTION_AWAITING_RESPONSE;
+ goto keep_going;
+ }
+
case CONNECTION_AUTH_OK:
{
/*
@@ -2921,8 +3105,9 @@ keep_going: /* We will come back to here until there is
if (res->resultStatus != PGRES_FATAL_ERROR)
appendPQExpBufferStr(&conn->errorMessage,
libpq_gettext("unexpected message from server during startup\n"));
- else if (conn->send_appname &&
- (conn->appname || conn->fbappname))
+ else if ((conn->send_appname &&
+ (conn->appname || conn->fbappname)) ||
+ (conn->send_startup_params && startup_parameters.head))
{
/*
* If we tried to send application_name, check to see
@@ -2938,10 +3123,23 @@ keep_going: /* We will come back to here until there is
sqlstate = PQresultErrorField(res, PG_DIAG_SQLSTATE);
if (sqlstate &&
- strcmp(sqlstate, ERRCODE_APPNAME_UNKNOWN) == 0)
+ strcmp(sqlstate, ERRCODE_PARAMETER_UNKNOWN) == 0)
{
PQclear(res);
- conn->send_appname = false;
+
+ if (conn->send_startup_params && startup_parameters.head)
+ {
+ conn->send_startup_params = false;
+ /*
+ * Sending optional parameters is the only FE feature of pgwire v3.1;
+ * downgrade to v3.0 if this failed.
+ */
+ conn->pversion = PG_PROTOCOL(3, 0);
+ }
+
+ if (conn->send_appname && (conn->appname || conn->fbappname))
+ conn->send_appname = false;
+
/* Must drop the old connection */
pqDropConnection(conn, true);
conn->status = CONNECTION_NEEDED;
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index a484fe80a1..7f5c74b11f 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -13,6 +13,7 @@
*-------------------------------------------------------------------------
*/
#include "postgres_fe.h"
+#include "fe_utils/simple_list.h"
#include <ctype.h>
#include <fcntl.h>
@@ -54,7 +55,8 @@ static int getReadyForQuery(PGconn *conn);
static void reportErrorPosition(PQExpBuffer msg, const char *query,
int loc, int encoding);
static int build_startup_packet(const PGconn *conn, char *packet,
- const PQEnvironmentOption *options);
+ const PQEnvironmentOption *options,
+ SimpleStringList *startup_parameters);
/*
@@ -2116,15 +2118,16 @@ pqFunctionCall3(PGconn *conn, Oid fnid,
*/
char *
pqBuildStartupPacket3(PGconn *conn, int *packetlen,
- const PQEnvironmentOption *options)
+ const PQEnvironmentOption *options,
+ SimpleStringList *startup_parameters)
{
char *startpacket;
- *packetlen = build_startup_packet(conn, NULL, options);
+ *packetlen = build_startup_packet(conn, NULL, options, startup_parameters);
startpacket = (char *) malloc(*packetlen);
if (!startpacket)
return NULL;
- *packetlen = build_startup_packet(conn, startpacket, options);
+ *packetlen = build_startup_packet(conn, startpacket, options, startup_parameters);
return startpacket;
}
@@ -2139,11 +2142,15 @@ pqBuildStartupPacket3(PGconn *conn, int *packetlen,
*/
static int
build_startup_packet(const PGconn *conn, char *packet,
- const PQEnvironmentOption *options)
+ const PQEnvironmentOption *options,
+ SimpleStringList *startup_parameters)
{
int packet_len = 0;
const PQEnvironmentOption *next_eo;
const char *val;
+ int nCells = 0;
+ char *name, *value;
+ SimpleStringListCell *pCurr;
/* Protocol version comes first. */
if (packet)
@@ -2195,6 +2202,31 @@ build_startup_packet(const PGconn *conn, char *packet,
}
}
+ if (conn->send_startup_params && startup_parameters->head)
+ {
+ for (pCurr = startup_parameters->head; pCurr != NULL; pCurr = pCurr->next, ++nCells)
+ {
+ if ((nCells % 2) == 0)
+ name = pCurr->val;
+ else
+ {
+ value = pCurr->val;
+ ADD_STARTUP_OPTION(name, value);
+
+ /* Mark value consumed */
+ pCurr->touched = true;
+ }
+ }
+
+ if ((nCells % 2) != 0)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("at least one parameter specified in"
+ "StartupMessage does not have a value\n"));
+ pqSaveErrorResult(conn);
+ }
+ }
+
/* Add trailing terminator */
if (packet)
packet[packet_len] = '\0';
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 1d915e7915..d04f48544c 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -58,6 +58,7 @@ typedef enum
CONNECTION_MADE, /* Connection OK; waiting to send. */
CONNECTION_AWAITING_RESPONSE, /* Waiting for a response from the
* postmaster. */
+ CONNECTION_NEGOTIATING, /* Negotiating pgwire protocol between FE/BE */
CONNECTION_AUTH_OK, /* Received authentication; waiting for
* backend startup. */
CONNECTION_SETENV, /* Negotiating environment. */
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 42913604e3..4a1839186f 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -43,6 +43,12 @@
/* include stuff found in fe only */
#include "pqexpbuffer.h"
+/*
+ * Simple string list data structure to track
+ * startup parameters that were accepted.
+ */
+#include "fe_utils/simple_list.h"
+
#ifdef ENABLE_GSS
#if defined(HAVE_GSSAPI_H)
#include <gssapi.h>
@@ -414,6 +420,7 @@ struct pg_conn
PGSetenvStatusType setenv_state; /* for 2.0 protocol only */
const PQEnvironmentOption *next_eo;
bool send_appname; /* okay to send application_name? */
+ bool send_startup_params; /* okay to send startup parameters? */
/* Miscellaneous stuff */
int be_pid; /* PID of backend --- needed for cancels */
@@ -598,7 +605,8 @@ extern PGresult *pqFunctionCall2(PGconn *conn, Oid fnid,
/* === in fe-protocol3.c === */
extern char *pqBuildStartupPacket3(PGconn *conn, int *packetlen,
- const PQEnvironmentOption *options);
+ const PQEnvironmentOption *options,
+ SimpleStringList *parameters);
extern void pqParseInput3(PGconn *conn);
extern int pqGetErrorNotice3(PGconn *conn, bool isError);
extern void pqBuildErrorMessage3(PQExpBuffer msg, const PGresult *res,
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 686c7369f6..a440ca303f 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -234,7 +234,7 @@ sub mkvcbuild
$libpq->UseDef('src/interfaces/libpq/libpqdll.def');
$libpq->ReplaceFile('src/interfaces/libpq/libpqrc.c',
'src/interfaces/libpq/libpq.rc');
- $libpq->AddReference($libpgport);
+ $libpq->AddReference($libpgcommon, $libpgfeutils, $libpgport);
# The OBJS scraper doesn't know about ifdefs, so remove fe-secure-openssl.c
# and sha2_openssl.c if building without OpenSSL, and remove sha2.c if
--
2.13.2.windows.1
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-10-13 18:16 Robert Haas <robertmhaas@gmail.com>
parent: Badrul Chowdhury <bachow@microsoft.com>
0 siblings, 1 reply; 24+ messages in thread
From: Robert Haas @ 2017-10-13 18:16 UTC (permalink / raw)
To: Badrul Chowdhury <bachow@microsoft.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
On Fri, Oct 6, 2017 at 5:07 PM, Badrul Chowdhury <bachow@microsoft.com> wrote:
> I added a mechanism to fall back to v3.0 if the BE fails to start when FE initiates a connection with v3.1 (with optional startup parameters). This completely eliminates the need to backpatch older servers, ie newer FE can connect to older BE. Please let me know what you think.
Well, I think it needs a good bit of cleanup before we can really get
to the substance of the patch.
+ fe_utils \
interfaces \
backend/replication/libpqwalreceiver \
backend/replication/pgoutput \
- fe_utils \
Useless change, omit.
+ if (whereToSendOutput != DestRemote ||
+ PG_PROTOCOL_MAJOR(FrontendProtocol) < 3)
+ return -1;
+
+ int sendStatus = 0;
Won't compile on older compilers. We generally aim for C89
compliance, with a few exceptions for newer features.
Also, why initialize sendStatus and then overwrite the value in the
very next line of code?
Also, the PG_PROTOCOL_MAJOR check here seems to be redundant with the
one in the caller.
+ /* NegotiateServerProtocol packet structure
+ *
+ * [ 'Y' | msgLength | min_version | max_version | param_list_len
| list of param names ]
+ */
+
Please pgindent your patches. I suspect you'll find this gets garbled.
Is there really any reason to separate NegotiateServerProtocol and
ServerProtocolVersion into separate functions?
-libpq = -L$(libpq_builddir) -lpq
+libpq = -L$(libpq_builddir) -lpq -L$(top_builddir)/src/common
-lpgcommon -L$(top_builddir)/src/fe_utils -lpgfeutils
+ $libpq->AddReference($libpgcommon, $libpgfeutils, $libpgport);
I haven't done any research to try to figure out why you did this, but
I don't think these are likely to be acceptable changes.
SendServerProtocolVersionMessage should be adjusted to use the new
facilities added by commit 1de09ad8eb1fa673ee7899d6dfbb2b49ba204818.
- /* Check we can handle the protocol the frontend is using. */
-
- if (PG_PROTOCOL_MAJOR(proto) < PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST) ||
- PG_PROTOCOL_MAJOR(proto) > PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) ||
- (PG_PROTOCOL_MAJOR(proto) == PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) &&
- PG_PROTOCOL_MINOR(proto) > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST)))
- ereport(FATAL,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("unsupported frontend protocol %u.%u: server supports %
u.0 to %u.%u",
- PG_PROTOCOL_MAJOR(proto), PG_PROTOCOL_MINOR(proto),
- PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST),
- PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST),
- PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST))));
The way you've arranged things here looks like it'll cause us to
accept connections even for protocol versions 4.x or higher; I don't
think we want that. I suggest checking the major version number at
this point in the code; then, the code path for version 3+ needs no
additional check and the code path for version 2 can enforce 2.0.
+bool
+is_optional(const char *guc_name)
+{
+ const char *optionalPrefix = "_pq_";
+ bool isOptional = false;
+
+ /* "_pq_" must be a proper prefix of the guc name in all encodings */
+ if (guc_name_compare(guc_name, optionalPrefix) == 1 &&
+ strstr(guc_name, optionalPrefix))
+ isOptional = true;
+
+ return isOptional;
+}
This seems like very strange coding for all kinds of reasons. Why is
guc_name_compare() used to do the comparison and strstr() then used
afterwards? Why do we need two tests instead of just one, and why
should one of them be case-sensitive and the other not? Why not just
use strncmp? Why write bool var = false; if (blah blah) var = true;
return var; instead of just return blah blah? Why the comment about
encodings - that doesn't seem particularly relevant here? Why
redeclare the prefix here instead of having a common definition
someplace that can be used by both the frontend and the backend,
probably a header file #define?
Also, this really doesn't belong in guc.c at all. We should be
separating out these options in ProcessStartupPacket() just as we do
for existing protocol-level options. When we actually have some
options, I think they should be segregated into a separate list
hanging off of the port, instead of letting them get mixed into
port->guc_options, but for right now we don't have any yet, so a bunch
of this complexity can go away.
+ ListCell *gucopts = list_head(port->guc_options);
+ while (gucopts)
+ {
+ char *name;
+
+ /* First comes key, which we need. */
+ name = lfirst(gucopts);
+ gucopts = lnext(gucopts);
+
+ /* Then comes value, which we don't need. */
+ gucopts = lnext(gucopts);
+
+ pq_sendstring(&buf, name);
+ }
This is another coding rule violation because the declaration of
gucopts follows executable statements.
- SimpleStringList roles = {NULL, NULL};
+ SimpleStringList roles = {NULL, NULL, NULL};
I don't think it's a good idea to change SimpleStringList like this --
it's used in lots of places already. If we were going to do it, a
bool needs to be set to false, not NULL.
+ /* Cannot append to immutable list */
+ if (list->is_immutable)
+ return;
Even if I were inclined to support changing the SimpleStringList
abstraction, this seems like super-confusing behavior -- just don't
append, and don't warn the user that nothing happened in any way?
Ugh.
+override CPPFLAGS := -DFRONTEND $(CPPFLAGS) -fPIC
+override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS) -fPIC
Seems unrelated to anything this patch is about.
+#include "postgres.h"
We never include "postgres.h" in other header files.
+#include "libpq/libpq-be.h"
+
+extern int NegotiateServerProtocol(Port *port);
+extern int SendServerProtocolVersionMessage(Port *port);
These changes aren't needed. The functions aren't called from outside
the file where they are defined, so just make them static and
prototype them in that file. That avoids sucking in additional
headers into this file.
+ /*
+ * Block until message length is read.
+ *
+ * No need to account for 2.x fixed-length message
because this state cannot
+ * be reached by pre-3.0 server.
+ */
Wrong formatting. pgindent will fix it.
+ {
+ return PGRES_POLLING_READING;
+ }
Superfluous braces.
+ {
+ server_is_older = true;
+ }
And here.
+ runningMsgLength -= buf->len + 1; /* +1 for NULL */
NUL or \0, not NULL. You're talking about the byte, not the pointer value.
In general, I think it might be a good idea for the client to send a
3.0 connection request unless the user requests some feature that
requires use of a >3.0 protocol version -- and right now there are no
such features. It's a little hard to predict what we might want to do
with minor protocol versions in the future so maybe at some point
there will be a good reason for us to request the newest protocol
version we can get (e.g. if we make some protocol change that improves
performance). Right now, though, there's a big advantage to not
requesting anything beyond 3.0 unless we need it -- it works with
existing servers. So I suggest that for right now we just make the
server side changes here to 3.x,x>0 protocol versions and accept
_pq_.whatever parameters, and leave all of these libpq changes out
completely. Some future patch might need those changes but this one
doesn't.
--
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-10-18 23:35 Badrul Chowdhury <bachow@microsoft.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 24+ messages in thread
From: Badrul Chowdhury @ 2017-10-18 23:35 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
Hi Robert,
Thanks very much for your quick response. PFA the patch containing the BE changes for pgwire v3.1, correctly formatted using pgindent this time 😊
A few salient points:
>> SendServerProtocolVersionMessage should be adjusted to use the new
>> facilities added by commit 1de09ad8eb1fa673ee7899d6dfbb2b49ba204818.
The new functionality is for sending 64bit ints. I think 32bits is sufficient for the information we want to pass around in the protocol negotiation phase, so I left this part unchanged.
>> Also, this really doesn't belong in guc.c at all. We should be separating out
>> these options in ProcessStartupPacket() just as we do for existing protocol-
>> level options. When we actually have some options, I think they should be
>> segregated into a separate list hanging off of the port, instead of letting them
>> get mixed into
>> port->guc_options, but for right now we don't have any yet, so a bunch
>> of this complexity can go away.
You are right, it is more elegant to make this a part of the port struct; I made the necessary changes in the patch.
Thanks,
Badrul
>> -----Original Message-----
>> From: Robert Haas [mailto:robertmhaas@gmail.com]
>> Sent: Friday, October 13, 2017 11:16 AM
>> To: Badrul Chowdhury <bachow@microsoft.com>
>> Cc: Tom Lane <tgl@sss.pgh.pa.us>; Satyanarayana Narlapuram
>> <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer
>> <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus
>> Hagander <magnus@hagander.net>; PostgreSQL-development <pgsql-
>> hackers@postgresql.org>
>> Subject: Re: [HACKERS] Re: protocol version negotiation (Re: Libpq
>> PGRES_COPY_BOTH - version compatibility)
>>
>> On Fri, Oct 6, 2017 at 5:07 PM, Badrul Chowdhury <bachow@microsoft.com>
>> wrote:
>> > I added a mechanism to fall back to v3.0 if the BE fails to start when FE
>> initiates a connection with v3.1 (with optional startup parameters). This
>> completely eliminates the need to backpatch older servers, ie newer FE can
>> connect to older BE. Please let me know what you think.
>>
>> Well, I think it needs a good bit of cleanup before we can really get to the
>> substance of the patch.
>>
>> + fe_utils \
>> interfaces \
>> backend/replication/libpqwalreceiver \
>> backend/replication/pgoutput \
>> - fe_utils \
>>
>> Useless change, omit.
>>
>> + if (whereToSendOutput != DestRemote ||
>> + PG_PROTOCOL_MAJOR(FrontendProtocol) < 3)
>> + return -1;
>> +
>> + int sendStatus = 0;
>>
>> Won't compile on older compilers. We generally aim for C89 compliance, with
>> a few exceptions for newer features.
>>
>> Also, why initialize sendStatus and then overwrite the value in the very next
>> line of code?
>>
>> Also, the PG_PROTOCOL_MAJOR check here seems to be redundant with the
>> one in the caller.
>>
>> + /* NegotiateServerProtocol packet structure
>> + *
>> + * [ 'Y' | msgLength | min_version | max_version | param_list_len
>> | list of param names ]
>> + */
>> +
>>
>> Please pgindent your patches. I suspect you'll find this gets garbled.
>>
>> Is there really any reason to separate NegotiateServerProtocol and
>> ServerProtocolVersion into separate functions?
>>
>> -libpq = -L$(libpq_builddir) -lpq
>> +libpq = -L$(libpq_builddir) -lpq -L$(top_builddir)/src/common
>> -lpgcommon -L$(top_builddir)/src/fe_utils -lpgfeutils
>> + $libpq->AddReference($libpgcommon, $libpgfeutils, $libpgport);
>>
>> I haven't done any research to try to figure out why you did this, but I don't
>> think these are likely to be acceptable changes.
>>
>> SendServerProtocolVersionMessage should be adjusted to use the new
>> facilities added by commit 1de09ad8eb1fa673ee7899d6dfbb2b49ba204818.
>>
>> - /* Check we can handle the protocol the frontend is using. */
>> -
>> - if (PG_PROTOCOL_MAJOR(proto) <
>> PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST) ||
>> - PG_PROTOCOL_MAJOR(proto) >
>> PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) ||
>> - (PG_PROTOCOL_MAJOR(proto) ==
>> PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) &&
>> - PG_PROTOCOL_MINOR(proto) >
>> PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST)))
>> - ereport(FATAL,
>> - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>> - errmsg("unsupported frontend protocol %u.%u: server supports %
>> u.0 to %u.%u",
>> - PG_PROTOCOL_MAJOR(proto), PG_PROTOCOL_MINOR(proto),
>> - PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST),
>> - PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST),
>> - PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST))));
>>
>> The way you've arranged things here looks like it'll cause us to accept
>> connections even for protocol versions 4.x or higher; I don't think we want
>> that. I suggest checking the major version number at this point in the code;
>> then, the code path for version 3+ needs no additional check and the code
>> path for version 2 can enforce 2.0.
>>
>> +bool
>> +is_optional(const char *guc_name)
>> +{
>> + const char *optionalPrefix = "_pq_";
>> + bool isOptional = false;
>> +
>> + /* "_pq_" must be a proper prefix of the guc name in all encodings */
>> + if (guc_name_compare(guc_name, optionalPrefix) == 1 &&
>> + strstr(guc_name, optionalPrefix))
>> + isOptional = true;
>> +
>> + return isOptional;
>> +}
>>
>> This seems like very strange coding for all kinds of reasons. Why is
>> guc_name_compare() used to do the comparison and strstr() then used
>> afterwards? Why do we need two tests instead of just one, and why should
>> one of them be case-sensitive and the other not? Why not just use strncmp?
>> Why write bool var = false; if (blah blah) var = true; return var; instead of just
>> return blah blah? Why the comment about encodings - that doesn't seem
>> particularly relevant here? Why redeclare the prefix here instead of having a
>> common definition someplace that can be used by both the frontend and the
>> backend, probably a header file #define?
>>
>> Also, this really doesn't belong in guc.c at all. We should be separating out
>> these options in ProcessStartupPacket() just as we do for existing protocol-
>> level options. When we actually have some options, I think they should be
>> segregated into a separate list hanging off of the port, instead of letting them
>> get mixed into
>> port->guc_options, but for right now we don't have any yet, so a bunch
>> of this complexity can go away.
>>
>> + ListCell *gucopts = list_head(port->guc_options);
>> + while (gucopts)
>> + {
>> + char *name;
>> +
>> + /* First comes key, which we need. */
>> + name = lfirst(gucopts);
>> + gucopts = lnext(gucopts);
>> +
>> + /* Then comes value, which we don't need. */
>> + gucopts = lnext(gucopts);
>> +
>> + pq_sendstring(&buf, name);
>> + }
>>
>> This is another coding rule violation because the declaration of gucopts
>> follows executable statements.
>>
>> - SimpleStringList roles = {NULL, NULL};
>> + SimpleStringList roles = {NULL, NULL, NULL};
>>
>> I don't think it's a good idea to change SimpleStringList like this -- it's used in
>> lots of places already. If we were going to do it, a bool needs to be set to
>> false, not NULL.
>>
>> + /* Cannot append to immutable list */
>> + if (list->is_immutable)
>> + return;
>>
>> Even if I were inclined to support changing the SimpleStringList abstraction,
>> this seems like super-confusing behavior -- just don't append, and don't warn
>> the user that nothing happened in any way?
>> Ugh.
>>
>> +override CPPFLAGS := -DFRONTEND $(CPPFLAGS) -fPIC override CPPFLAGS :=
>> +-DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS) -fPIC
>>
>> Seems unrelated to anything this patch is about.
>>
>> +#include "postgres.h"
>>
>> We never include "postgres.h" in other header files.
>>
>> +#include "libpq/libpq-be.h"
>> +
>> +extern int NegotiateServerProtocol(Port *port); extern int
>> +SendServerProtocolVersionMessage(Port *port);
>>
>> These changes aren't needed. The functions aren't called from outside the file
>> where they are defined, so just make them static and prototype them in that
>> file. That avoids sucking in additional headers into this file.
>>
>> + /*
>> + * Block until message length is read.
>> + *
>> + * No need to account for 2.x fixed-length message
>> because this state cannot
>> + * be reached by pre-3.0 server.
>> + */
>>
>> Wrong formatting. pgindent will fix it.
>>
>> + {
>> + return PGRES_POLLING_READING;
>> + }
>>
>> Superfluous braces.
>>
>> + {
>> + server_is_older = true;
>> + }
>>
>> And here.
>>
>> + runningMsgLength -= buf->len + 1; /* +1 for NULL */
>>
>> NUL or \0, not NULL. You're talking about the byte, not the pointer value.
>>
>> In general, I think it might be a good idea for the client to send a
>> 3.0 connection request unless the user requests some feature that requires
>> use of a >3.0 protocol version -- and right now there are no such features. It's
>> a little hard to predict what we might want to do with minor protocol versions
>> in the future so maybe at some point there will be a good reason for us to
>> request the newest protocol version we can get (e.g. if we make some
>> protocol change that improves performance). Right now, though, there's a big
>> advantage to not requesting anything beyond 3.0 unless we need it -- it works
>> with existing servers. So I suggest that for right now we just make the server
>> side changes here to 3.x,x>0 protocol versions and accept _pq_.whatever
>> parameters, and leave all of these libpq changes out completely. Some future
>> patch might need those changes but this one doesn't.
>>
>> --
>> Robert Haas
>> EnterpriseDB:
>> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.ent
>> erprisedb.com&data=02%7C01%7Cbachow%40microsoft.com%7Ce68db1491
>> 10b426ceb4e08d51266842c%7C72f988bf86f141af91ab2d7cd011db47%7C1
>> %7C0%7C636435153876276748&sdata=1Y%2FLhfYfN9Km8PxKAN7ghF1siYUt
>> hXoIY0LGxQNywk8%3D&reserved=0
>> 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
Attachments:
[application/octet-stream] pgwire_be.patch (5.0K, ../../BN6PR21MB0772A3E76F9FC9D60DA08341D14D0@BN6PR21MB0772.namprd21.prod.outlook.com/2-pgwire_be.patch)
download | inline diff:
From a5ef6cc738123b6ad2032a20ab0a8cc983cf2f0e Mon Sep 17 00:00:00 2001
From: Badrul Chowdhury <bachow@microsoft.com>
Date: Wed, 18 Oct 2017 15:26:33 -0700
Subject: [PATCH] BE changes for pgwire v3.1.
---
src/backend/postmaster/postmaster.c | 80 +++++++++++++++++++++++++++++++++++--
src/include/libpq/libpq-be.h | 1 +
2 files changed, 77 insertions(+), 4 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 95180b2ef5..1eafdea87b 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -387,6 +387,7 @@ static DNSServiceRef bonjour_sdref = NULL;
/*
* postmaster.c - function prototypes
*/
+static int NegotiateServerProtocol(Port *port);
static void CloseServerPorts(int status, Datum arg);
static void unlink_external_pid_file(int status, Datum arg);
static void getInstallationPaths(const char *argv0);
@@ -1368,6 +1369,48 @@ PostmasterMain(int argc, char *argv[])
abort(); /* not reached */
}
+static int
+NegotiateServerProtocol(Port *port)
+{
+ StringInfoData buf;
+ char *name;
+ ListCell *optional_params;
+
+ /*
+ * NegotiateServerProtocol packet structure
+ *
+ * [ 'Y' | msgLength | min_version | max_version | param_list_len | list
+ * of param names ]
+ */
+
+ /* PG message type */
+ pq_beginmessage(&buf, 'Y');
+
+ /* Protocol version numbers */
+ pq_sendint(&buf, PG_PROTOCOL_EARLIEST, sizeof(int32)); /* min */
+ pq_sendint(&buf, PG_PROTOCOL_LATEST, sizeof(int32)); /* max */
+
+ /* Length of parameter list; parameter list consists of (key, value) pairs */
+ pq_sendint(&buf, list_length(port->optional_parameters) / 2, sizeof(int32));
+
+ optional_params = list_head(port->optional_parameters);
+ while (optional_params)
+ {
+ /* First comes key, which we need. */
+ name = lfirst(optional_params);
+ optional_params = lnext(optional_params);
+
+ /* Then comes value, which we don't need. */
+ optional_params = lnext(optional_params);
+
+ pq_sendstring(&buf, name);
+ }
+
+ pq_endmessage(&buf);
+
+ /* Ensure that the message buffer is flushed */
+ pq_flush();
+}
/*
* on_proc_exit callback to close server's listen sockets
@@ -2050,12 +2093,10 @@ retry1:
*/
FrontendProtocol = proto;
- /* Check we can handle the protocol the frontend is using. */
+ /* Check we can handle the major protocol the frontend is using. */
if (PG_PROTOCOL_MAJOR(proto) < PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST) ||
- PG_PROTOCOL_MAJOR(proto) > PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) ||
- (PG_PROTOCOL_MAJOR(proto) == PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) &&
- PG_PROTOCOL_MINOR(proto) > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST)))
+ PG_PROTOCOL_MAJOR(proto) > PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST))
ereport(FATAL,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("unsupported frontend protocol %u.%u: server supports %u.0 to %u.%u",
@@ -2126,6 +2167,13 @@ retry1:
valptr),
errhint("Valid values are: \"false\", 0, \"true\", 1, \"database\".")));
}
+ else if (strlen(nameptr) > 4 && strncmp(nameptr, "_pq_", 4) == 0)
+ {
+ port->optional_parameters = lappend(port->optional_parameters,
+ pstrdup(nameptr));
+ port->optional_parameters = lappend(port->optional_parameters,
+ pstrdup(valptr));
+ }
else
{
/* Assume it's a generic GUC option */
@@ -2145,9 +2193,33 @@ retry1:
ereport(FATAL,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("invalid startup packet layout: expected terminator as last byte")));
+
+ /*
+ * Need to negotiate pgwire protocol if FE version is not the same
+ * as BE version and FE version is not 3.0
+ */
+ if (FrontendProtocol != PG_PROTOCOL_LATEST
+ && FrontendProtocol != PG_PROTOCOL(3, 0))
+ {
+ /* Negotiate parameters after all the error-checking is done */
+ if (NegotiateServerProtocol(port))
+ return STATUS_ERROR;
+ }
}
else
{
+ /* Check we can handle the minor protocol the frontend is using. */
+
+ if (PG_PROTOCOL_MAJOR(proto) == PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) &&
+ PG_PROTOCOL_MINOR(proto) > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST))
+ ereport(FATAL,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("unsupported frontend protocol %u.%u: server supports %u.0 to %u.%u",
+ PG_PROTOCOL_MAJOR(proto), PG_PROTOCOL_MINOR(proto),
+ PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST),
+ PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST),
+ PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST))));
+
/*
* Get the parameters from the old-style, fixed-width-fields startup
* packet as C strings. The packet destination was cleared first so a
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 7bde744d51..8b2ef62580 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -137,6 +137,7 @@ typedef struct Port
char *user_name;
char *cmdline_options;
List *guc_options;
+ List *optional_parameters;
/*
* Information that needs to be held during the authentication cycle.
--
2.13.2.windows.1
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-10-27 12:55 Robert Haas <robertmhaas@gmail.com>
parent: Badrul Chowdhury <bachow@microsoft.com>
0 siblings, 1 reply; 24+ messages in thread
From: Robert Haas @ 2017-10-27 12:55 UTC (permalink / raw)
To: Badrul Chowdhury <bachow@microsoft.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
On Thu, Oct 19, 2017 at 1:35 AM, Badrul Chowdhury <bachow@microsoft.com> wrote:
> The new functionality is for sending 64bit ints. I think 32bits is sufficient for the information we want to pass around in the protocol negotiation phase, so I left this part unchanged.
No, it isn't. That commit didn't add any new functionality, but it
changed the preferred interfaces for assembling protocol messages.
Your patch was still using the old ones.
Attached is an updated patch. I've made a few modifications:
- I wrote documentation. For future reference, that's really the job
of the patch submitter.
- I changed the message type byte for the new message from 'Y' to 'v'.
'Y' is apparently used by logical replication as a "type message", but
'v' is not currently used for anything. It's also somewhat mnemonic.
- I deleted the minimum protocol version from the new message. I know
there were a few votes for including it, but I think it's probably
useless. The client should always request the newest version it can
support; if that's not new enough for the server, then we're dead
anyway and we might as well just handle that via ERROR. Moreover, it
seems questionable whether we'd ever deprecate 3.0 support in the
server anyway, or if we do, it'll probably be because 4.0 has been
stable for a decade or so. Desupporting 3.0 while continuing to
support 3.x,x>0 seems like a remote outcome (and, like I say, if it
does happen, ERROR is a good-enough response). If there's some use
case for having a client request an older protocol version than the
newest one it can support, then this change could be revisited, or we
can just handle that by retrying the whole connection attempt.
- I changed the test for whether to send NegotiateProtocolVersion to
send it only when the client requests a version too new for the
server. I think that if the client requests a version older than what
the server could support, the server should just silently use the
older version. That's arguable. You could argue that the message
should be sent anyway (at least to post-3.0 clients) as a way of
reporting what happened with _pq_.<whatever> parameters, but I have
two counter-arguments. Number one, maybe clients just shouldn't send
_pq_.<whatever> parameters that the protocol version they're using
doesn't support, or if they do, be prepared for them to have no
effect. Number two, this is exactly the sort of thing that we can
change in future minor protocol versions if we wish. For example, we
could define protocol version 3.1 or 3.43 or whatever as always
sending a NegotiateProtocolVersion message. There's no need for the
code to make 3.0 compatible with future versions to decide what
choices those future versions might make.
- Made the prefix matching check for "_pq_." rather than "_pq_". I
think we're imagining extensions like _pq_.magic_fairy_dust, not
_pq_magic_fairy_dust.
- I got rid of the optional_parameters thing in Port and just passed a
list of unrecognized parameters around directly. Once some parameters
are recognized, e.g. _pq_.magic_fairy_dust = 1, we'll probably have
dedicated fields in the Port for them (i.e. int magic_fairy_dust)
rather than digging them out of some list. Moreover, with your
design, we'd end up having to make NegotiateServerProtocol exclude
things that are actually recognized, which would be annoying.
- I got rid of the pq_flush(). There's no need for this because we're
always going to send another protocol message (ErrorResponse or
AuthenticationSomething) afterwards.
- I renamed NegotiateServerProtocol to SendNegotiateProtocolVersion
and moved it to what I think is a more sensible place in the file.
- I removed the error check for 2.x, x != 0. I may have advocated for
this before, but on reflection, I don't see much advantage in
rejecting things that work today.
- I fixed the above-mentioned failure to use the new interface for
assembling the NegotiateProtocolVersion message.
- I did some work on the comments.
Barring objections, I plan to commit this and back-patch it all the
way. Of course, this is not a bug fix, but Tom previously proposed
back-patching it and I think that's a good idea, because if we don't,
it will be a very long time before servers with this code become
commonplace in the wild. Back-patching doesn't completely fix that
problem because not everybody applies upgrades and some people may be
running EOL versions, but it will still help.
Also attached is a patch I used for testing purposes, some version of
which we might eventually use when we actually introduce version 3.1
of the protocol. It bumps the protocol version that libpq uses from
3.0 to 3.1 without changing what the server thinks the latest protocol
version is, so the server always replies with a
NegotiateProtocolVersion message. It also teaches libpq to ignore the
NegotiateProtocolVersion message. With that patch applied, make
check-world passes, which seems to show that the server-side changes
are not totally broken. More testing would be great, of course.
Some thoughts on the future:
- libpq should grow an option to force a specific protocol version.
Andres already proposed one to force 2.0, but now we probably want to
generalize that to also allow forcing a particular minor version.
That seems useful for testing, if nothing else, and might let us add
TAP tests that this stuff works as intended.
- Possibly we should commit the portion of the testing patch which
ignores NegotiateProtocolVersion to v11, maybe also adding a
connection status function that lets users inquire about whether a
NegotiateProtocolVersion message was received and, if so, what
parameters it reported as unrecognized and what minor version it
reported the server as speaking. The existing PQprotocolVersion
interface looks inadequate, as it seems to return only the major
version.
- On further reflection, I think the reconnect functionality you
proposed previously is probably a good idea. It won't be necessary
with servers that have been patched to send NegotiateProtocolVersion,
but there may be quite a few that haven't for a long time to come, and
although an automated reconnect is a little annoying, it's a lot less
annoying than an outright connection failure. So that part of your
patch should probably be resubmitted when and if we bump the version
to 3.1.
--
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
Attachments:
[application/octet-stream] pgwire-be-rmh.patch (9.5K, ../../CA+TgmoYLgvr8ezrapQaBLJEXt3j3RdAJv=jxAY=W8BK=1t4DKw@mail.gmail.com/2-pgwire-be-rmh.patch)
download | inline diff:
From 85e04f460f378cdb53da137e82ba62e8a83833a5 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Fri, 27 Oct 2017 14:20:32 +0200
Subject: [PATCH 1/2] Minor protocol versions + extension parameters.
---
doc/src/sgml/protocol.sgml | 115 ++++++++++++++++++++++++++++++++----
src/backend/postmaster/postmaster.c | 58 ++++++++++++++++--
2 files changed, 157 insertions(+), 16 deletions(-)
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 15108baf71..f0816c3b16 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -22,10 +22,18 @@
<productname>PostgreSQL</productname> 7.4 and later. For descriptions
of the earlier protocol versions, see previous releases of the
<productname>PostgreSQL</productname> documentation. A single server
- can support multiple protocol versions. The initial
- startup-request message tells the server which protocol version the
- client is attempting to use, and then the server follows that protocol
- if it is able.
+ can support multiple protocol versions. The initial startup-request
+ message tells the server which protocol version the client is attempting to
+ use. If the major version requested by the client is not supported by
+ the server, the connection will be rejected (for example, this would occur
+ if the client requested protocol version 4.0, which does not exist as of
+ this writing). If the minor version requested by the client is not
+ supported by the server (e.g. the client requests version 3.1, but the
+ server supports only 3.0), the server may either reject the connection or
+ may respond with a NegotiateProtocolVersion message containing the highest
+ minor protocol version which it supports. The client may then choose either
+ to continue with the connection using the specified protocol version or
+ to abort the connection.
</para>
<para>
@@ -406,6 +414,19 @@
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>NegotiateProtocolVersion</term>
+ <listitem>
+ <para>
+ The server does not support the minor protocol version requested
+ by the client, but does support an earlier version of the protocol;
+ this message indicates the highest supported minor version.
+ This message will be followed by an ErrorResponse or a message
+ indicating the success or failure of authentication.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
@@ -420,8 +441,10 @@
for further messages from the server. In this phase a backend process
is being started, and the frontend is just an interested bystander.
It is still possible for the startup attempt
- to fail (ErrorResponse), but in the normal case the backend will send
- some ParameterStatus messages, BackendKeyData, and finally ReadyForQuery.
+ to fail (ErrorResponse) or the server to decline support for the requested
+ minor protocol version (NegotiateProtocolVersion), but in the normal case
+ the backend will send some ParameterStatus messages, BackendKeyData, and
+ finally ReadyForQuery.
</para>
<para>
@@ -4699,6 +4722,74 @@ GSSResponse (F)
</listitem>
</varlistentry>
+<varlistentry>
+<term>
+NegotiateProtocolVersion (B)
+</term>
+<listitem>
+<para>
+
+<variablelist>
+<varlistentry>
+<term>
+ Byte1('v')
+</term>
+<listitem>
+<para>
+ Identifies the message as a protocol version negotiation
+ message.
+</para>
+</listitem>
+</varlistentry>
+<varlistentry>
+<term>
+ Int32
+</term>
+<listitem>
+<para>
+ Length of message contents in bytes, including self.
+</para>
+</listitem>
+</varlistentry>
+<varlistentry>
+<term>
+ Int32
+</term>
+<listitem>
+<para>
+ Newest minor protocol version supported by the server
+ for the major protocol version requested by the client.
+</para>
+</listitem>
+</varlistentry>
+<varlistentry>
+<term>
+ Int32
+</term>
+<listitem>
+<para>
+ Number of protocol options not recognized by the server.
+</para>
+</listitem>
+</varlistentry>
+</variablelist>
+ Then, for protocol option not recognized by the server, there
+ is the following:
+<variablelist>
+<varlistentry>
+<term>
+ String
+</term>
+<listitem>
+<para>
+ The option name.
+</para>
+</listitem>
+</varlistentry>
+</variablelist>
+</para>
+</listitem>
+</varlistentry>
<varlistentry>
<term>
@@ -5654,11 +5745,13 @@ StartupMessage (F)
</varlistentry>
</variablelist>
- In addition to the above, any run-time parameter that can be
- set at backend start time might be listed. Such settings
- will be applied during backend start (after parsing the
- command-line arguments if any). The values will act as
- session defaults.
+ In addition to the above, others parameter may be listed.
+ Parameter names beginning with <literal>_pq_.</literal> are
+ reserved for use as protocol extensions, while others are
+ treated as run-time parameters to be set at backend start
+ time. Such settings will be applied during backend start
+ (after parsing the command-line arguments if any) and will
+ act as session defaults.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 2b2b993e2c..3963e97386 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -101,6 +101,7 @@
#include "lib/ilist.h"
#include "libpq/auth.h"
#include "libpq/libpq.h"
+#include "libpq/pqformat.h"
#include "libpq/pqsignal.h"
#include "miscadmin.h"
#include "pg_getopt.h"
@@ -412,6 +413,7 @@ static void ExitPostmaster(int status) pg_attribute_noreturn();
static int ServerLoop(void);
static int BackendStartup(Port *port);
static int ProcessStartupPacket(Port *port, bool SSLdone);
+static void SendNegotiateProtocolVersion(List *unrecognized_protocol_options);
static void processCancelRequest(Port *port, void *pkt);
static int initMasks(fd_set *rmask);
static void report_fork_failure_to_client(Port *port, int errnum);
@@ -2052,12 +2054,9 @@ retry1:
*/
FrontendProtocol = proto;
- /* Check we can handle the protocol the frontend is using. */
-
+ /* Check that the major protocol version is in range. */
if (PG_PROTOCOL_MAJOR(proto) < PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST) ||
- PG_PROTOCOL_MAJOR(proto) > PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) ||
- (PG_PROTOCOL_MAJOR(proto) == PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) &&
- PG_PROTOCOL_MINOR(proto) > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST)))
+ PG_PROTOCOL_MAJOR(proto) > PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST))
ereport(FATAL,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("unsupported frontend protocol %u.%u: server supports %u.0 to %u.%u",
@@ -2079,6 +2078,7 @@ retry1:
if (PG_PROTOCOL_MAJOR(proto) >= 3)
{
int32 offset = sizeof(ProtocolVersion);
+ List *unrecognized_protocol_options = NIL;
/*
* Scan packet body for name/option pairs. We can assume any string
@@ -2128,6 +2128,16 @@ retry1:
valptr),
errhint("Valid values are: \"false\", 0, \"true\", 1, \"database\".")));
}
+ else if (strncmp(nameptr, "_pq_.", 5) == 0)
+ {
+ /*
+ * Any option beginning with _pq_. is reserved for use as a
+ * protocol-level option, but at present no such options are
+ * defined.
+ */
+ unrecognized_protocol_options =
+ lappend(unrecognized_protocol_options, pstrdup(nameptr));
+ }
else
{
/* Assume it's a generic GUC option */
@@ -2147,6 +2157,16 @@ retry1:
ereport(FATAL,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("invalid startup packet layout: expected terminator as last byte")));
+
+
+ /*
+ * If necessary, tell the client that we don't speak the minor
+ * protocol version they have requested, and tell them about the
+ * newest version we do speak and any protocol options we didn't
+ * recognize.
+ */
+ if (PG_PROTOCOL_MINOR(proto) > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST))
+ SendNegotiateProtocolVersion(unrecognized_protocol_options);
}
else
{
@@ -2260,6 +2280,34 @@ retry1:
return STATUS_OK;
}
+/*
+ * Send a NegotiateProtocolVersion to the client. This lets the client know
+ * that they have requested a newer minor protocol version than we are able
+ * to speak. We'll speak the highest version we know about; the client can,
+ * of course, abandon the connection if that's a problem.
+ *
+ * We also include in the response a list of protocol options we didn't
+ * understand. This allows clients to include optional parameters that might
+ * be present either in newer protocol versions or third-party protocol
+ * extensions without fear of having to reconnect if those options are not
+ * understood, while at the same time making certain that the client is aware
+ * of which options were actually accepted.
+ */
+static void
+SendNegotiateProtocolVersion(List *unrecognized_protocol_options)
+{
+ StringInfoData buf;
+ ListCell *lc;
+
+ pq_beginmessage(&buf, 'v'); /* NegotiateProtocolVersion */
+ pq_sendint32(&buf, PG_PROTOCOL_LATEST);
+ pq_sendint32(&buf, list_length(unrecognized_protocol_options));
+ foreach(lc, unrecognized_protocol_options)
+ pq_sendstring(&buf, lfirst(lc));
+ pq_endmessage(&buf);
+
+ /* no need to flush, some other message will follow */
+}
/*
* The client has sent a cancel request packet, not a normal
--
2.13.5 (Apple Git-94)
[application/octet-stream] pgwire-libpq-test.patch (2.4K, ../../CA+TgmoYLgvr8ezrapQaBLJEXt3j3RdAJv=jxAY=W8BK=1t4DKw@mail.gmail.com/3-pgwire-libpq-test.patch)
download | inline diff:
From 4d3d56f19dd4673daa7510cfd0d463802f9d77f2 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Fri, 27 Oct 2017 14:21:16 +0200
Subject: [PATCH 2/2] libpq test framework
---
src/interfaces/libpq/fe-connect.c | 38 +++++++++++++++++++++++++++++++++-----
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 6bcf60a712..2c859d7617 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1798,11 +1798,11 @@ connectDBStart(PGconn *conn)
#endif
/*
- * Set up to try to connect, with protocol 3.0 as the first attempt.
+ * Set up to try to connect, with protocol 3.1 as the first attempt.
*/
conn->whichhost = 0;
conn->addr_cur = conn->connhost[0].addrlist;
- conn->pversion = PG_PROTOCOL(3, 0);
+ conn->pversion = PG_PROTOCOL(3, 1);
conn->send_appname = true;
conn->status = CONNECTION_NEEDED;
@@ -2653,13 +2653,13 @@ keep_going: /* We will come back to here until there is
* request or an error here. Anything else probably means
* it's not Postgres on the other end at all.
*/
- if (!(beresp == 'R' || beresp == 'E'))
+ if (!(beresp == 'R' || beresp == 'E' || beresp == 'v'))
{
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext(
"expected authentication request from "
- "server, but received %c\n"),
- beresp);
+ "server, but received %d\n"),
+ (int) beresp);
goto error_return;
}
@@ -2813,6 +2813,34 @@ keep_going: /* We will come back to here until there is
goto error_return;
}
+ /*
+ * Parse and discard any NegotiateProtocolVersion message.
+ */
+ if (beresp == 'v')
+ {
+ int server_max_version;
+ int num_unrecognized_parameters;
+
+ if (pqGetInt(&server_max_version, 4, conn))
+ return PGRES_POLLING_READING;
+ if (pqGetInt(&num_unrecognized_parameters, 4, conn))
+ return PGRES_POLLING_READING;
+ if (num_unrecognized_parameters > 0)
+ {
+ PQExpBufferData buf;
+ int i;
+
+ initPQExpBuffer(&buf);
+ for (i = 0; i < num_unrecognized_parameters; ++i)
+ (void) pqGets(&buf, conn);
+ termPQExpBuffer(&buf);
+ }
+
+ conn->inStart = conn->inCursor;
+
+ goto keep_going;
+ }
+
/* It is an authentication request. */
conn->auth_req_received = true;
--
2.13.5 (Apple Git-94)
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-10-31 01:19 Badrul Chowdhury <bachow@microsoft.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 24+ messages in thread
From: Badrul Chowdhury @ 2017-10-31 01:19 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
Hi Robert,
Thank you for the comprehensive review! We are very much in the early stages of contributing to the PG community and we clearly have lots to learn, but we look forward to becoming proficient and active members of the pg community.
Regarding the patch, I have tested it extensively by hand and it works great.
Some comments on the future direction:
>> Some thoughts on the future:
>>
>> - libpq should grow an option to force a specific protocol version.
>> Andres already proposed one to force 2.0, but now we probably want to
>> generalize that to also allow forcing a particular minor version.
>> That seems useful for testing, if nothing else, and might let us add TAP tests
>> that this stuff works as intended.
>>
>> - Possibly we should commit the portion of the testing patch which ignores
>> NegotiateProtocolVersion to v11, maybe also adding a connection status
>> function that lets users inquire about whether a NegotiateProtocolVersion
>> message was received and, if so, what parameters it reported as unrecognized
>> and what minor version it
>> reported the server as speaking. The existing PQprotocolVersion
>> interface looks inadequate, as it seems to return only the major version.
I think these changes are a good idea; I will initiate a design discussion on these targeting the 2018-01 commitfest on a separate thread.
>> - On further reflection, I think the reconnect functionality you proposed
>> previously is probably a good idea. It won't be necessary with servers that
>> have been patched to send NegotiateProtocolVersion, but there may be quite
>> a few that haven't for a long time to come, and although an automated
>> reconnect is a little annoying, it's a lot less annoying than an outright
>> connection failure. So that part of your patch should probably be resubmitted
>> when and if we bump the version to 3.1.
I will preserve the FE changes in my local branch so that we have it ready when a decision has been made regarding the bumping of the pgwire version.
Again, thanks very much for your feedback- I am in a much better position to make future contributions to the community explicitly because of it.
Regards,
Badrul
>> -----Original Message-----
>> From: Robert Haas [mailto:robertmhaas@gmail.com]
>> Sent: Friday, October 27, 2017 5:56 AM
>> To: Badrul Chowdhury <bachow@microsoft.com>
>> Cc: Tom Lane <tgl@sss.pgh.pa.us>; Satyanarayana Narlapuram
>> <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer
>> <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus
>> Hagander <magnus@hagander.net>; PostgreSQL-development <pgsql-
>> hackers@postgresql.org>
>> Subject: Re: [HACKERS] Re: protocol version negotiation (Re: Libpq
>> PGRES_COPY_BOTH - version compatibility)
>>
>> On Thu, Oct 19, 2017 at 1:35 AM, Badrul Chowdhury
>> <bachow@microsoft.com> wrote:
>> > The new functionality is for sending 64bit ints. I think 32bits is sufficient for
>> the information we want to pass around in the protocol negotiation phase, so
>> I left this part unchanged.
>>
>> No, it isn't. That commit didn't add any new functionality, but it changed the
>> preferred interfaces for assembling protocol messages.
>> Your patch was still using the old ones.
>>
>> Attached is an updated patch. I've made a few modifications:
>>
>> - I wrote documentation. For future reference, that's really the job of the
>> patch submitter.
>>
>> - I changed the message type byte for the new message from 'Y' to 'v'.
>> 'Y' is apparently used by logical replication as a "type message", but 'v' is not
>> currently used for anything. It's also somewhat mnemonic.
>>
>> - I deleted the minimum protocol version from the new message. I know there
>> were a few votes for including it, but I think it's probably useless. The client
>> should always request the newest version it can support; if that's not new
>> enough for the server, then we're dead anyway and we might as well just
>> handle that via ERROR. Moreover, it seems questionable whether we'd ever
>> deprecate 3.0 support in the server anyway, or if we do, it'll probably be
>> because 4.0 has been stable for a decade or so. Desupporting 3.0 while
>> continuing to support 3.x,x>0 seems like a remote outcome (and, like I say, if it
>> does happen, ERROR is a good-enough response). If there's some use case for
>> having a client request an older protocol version than the newest one it can
>> support, then this change could be revisited, or we can just handle that by
>> retrying the whole connection attempt.
>>
>> - I changed the test for whether to send NegotiateProtocolVersion to send it
>> only when the client requests a version too new for the server. I think that if
>> the client requests a version older than what the server could support, the
>> server should just silently use the older version. That's arguable. You could
>> argue that the message should be sent anyway (at least to post-3.0 clients) as
>> a way of reporting what happened with _pq_.<whatever> parameters, but I
>> have two counter-arguments. Number one, maybe clients just shouldn't send
>> _pq_.<whatever> parameters that the protocol version they're using doesn't
>> support, or if they do, be prepared for them to have no effect. Number two,
>> this is exactly the sort of thing that we can change in future minor protocol
>> versions if we wish. For example, we could define protocol version 3.1 or 3.43
>> or whatever as always sending a NegotiateProtocolVersion message. There's
>> no need for the code to make 3.0 compatible with future versions to decide
>> what choices those future versions might make.
>>
>> - Made the prefix matching check for "_pq_." rather than "_pq_". I think
>> we're imagining extensions like _pq_.magic_fairy_dust, not
>> _pq_magic_fairy_dust.
>>
>> - I got rid of the optional_parameters thing in Port and just passed a list of
>> unrecognized parameters around directly. Once some parameters are
>> recognized, e.g. _pq_.magic_fairy_dust = 1, we'll probably have dedicated
>> fields in the Port for them (i.e. int magic_fairy_dust) rather than digging them
>> out of some list. Moreover, with your design, we'd end up having to make
>> NegotiateServerProtocol exclude things that are actually recognized, which
>> would be annoying.
>>
>> - I got rid of the pq_flush(). There's no need for this because we're always
>> going to send another protocol message (ErrorResponse or
>> AuthenticationSomething) afterwards.
>>
>> - I renamed NegotiateServerProtocol to SendNegotiateProtocolVersion and
>> moved it to what I think is a more sensible place in the file.
>>
>> - I removed the error check for 2.x, x != 0. I may have advocated for this
>> before, but on reflection, I don't see much advantage in rejecting things that
>> work today.
>>
>> - I fixed the above-mentioned failure to use the new interface for assembling
>> the NegotiateProtocolVersion message.
>>
>> - I did some work on the comments.
>>
>> Barring objections, I plan to commit this and back-patch it all the way. Of
>> course, this is not a bug fix, but Tom previously proposed back-patching it and
>> I think that's a good idea, because if we don't, it will be a very long time
>> before servers with this code become commonplace in the wild. Back-
>> patching doesn't completely fix that problem because not everybody applies
>> upgrades and some people may be running EOL versions, but it will still help.
>>
>> Also attached is a patch I used for testing purposes, some version of which we
>> might eventually use when we actually introduce version 3.1 of the protocol.
>> It bumps the protocol version that libpq uses from
>> 3.0 to 3.1 without changing what the server thinks the latest protocol version
>> is, so the server always replies with a NegotiateProtocolVersion message. It
>> also teaches libpq to ignore the NegotiateProtocolVersion message. With that
>> patch applied, make check-world passes, which seems to show that the server-
>> side changes are not totally broken. More testing would be great, of course.
>>
>> Some thoughts on the future:
>>
>> - libpq should grow an option to force a specific protocol version.
>> Andres already proposed one to force 2.0, but now we probably want to
>> generalize that to also allow forcing a particular minor version.
>> That seems useful for testing, if nothing else, and might let us add TAP tests
>> that this stuff works as intended.
>>
>> - Possibly we should commit the portion of the testing patch which ignores
>> NegotiateProtocolVersion to v11, maybe also adding a connection status
>> function that lets users inquire about whether a NegotiateProtocolVersion
>> message was received and, if so, what parameters it reported as unrecognized
>> and what minor version it
>> reported the server as speaking. The existing PQprotocolVersion
>> interface looks inadequate, as it seems to return only the major version.
>>
>> - On further reflection, I think the reconnect functionality you proposed
>> previously is probably a good idea. It won't be necessary with servers that
>> have been patched to send NegotiateProtocolVersion, but there may be quite
>> a few that haven't for a long time to come, and although an automated
>> reconnect is a little annoying, it's a lot less annoying than an outright
>> connection failure. So that part of your patch should probably be resubmitted
>> when and if we bump the version to 3.1.
>>
>> --
>> Robert Haas
>> EnterpriseDB:
>> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.ent
>> erprisedb.com&data=02%7C01%7Cbachow%40microsoft.com%7Cf4348d608f
>> 8f421e33a608d51d3a08ef%7C72f988bf86f141af91ab2d7cd011db47%7C1%7
>> C0%7C636447057460416468&sdata=tyWDa%2B5cmxBEEIs5Btnm3PEl7SZF5a
>> 0ifhRhoD0QNig%3D&reserved=0
>> 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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-11-15 21:11 Robert Haas <robertmhaas@gmail.com>
parent: Badrul Chowdhury <bachow@microsoft.com>
0 siblings, 1 reply; 24+ messages in thread
From: Robert Haas @ 2017-11-15 21:11 UTC (permalink / raw)
To: Badrul Chowdhury <bachow@microsoft.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
On Mon, Oct 30, 2017 at 9:19 PM, Badrul Chowdhury <bachow@microsoft.com> wrote:
> Thank you for the comprehensive review! We are very much in the early stages of contributing to the PG community and we clearly have lots to learn, but we look forward to becoming proficient and active members of the pg community.
>
> Regarding the patch, I have tested it extensively by hand and it works great.
I spent a little more time looking at this patch today. I think that
the patch should actually send NegotiateProtocolVersion when *either*
the requested version is differs from the latest one we support *or*
an unsupported protocol option is present. Otherwise, you only find
out about unsupported protocol options if you also request a newer
minor version, which isn't good, because it makes it hard to add new
protocol options *without* bumping the protocol version.
Here's an updated version with that change and a proposed commit message.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
[application/octet-stream] pgwire-be-rmh-v2.patch (11.1K, ../../CA+TgmoZCis1TCcsMa8eu8wgT0K71fL_HQL+ZNtOEHwrxh804ow@mail.gmail.com/2-pgwire-be-rmh-v2.patch)
download | inline diff:
commit e63395bb924d480fa8184be957df54b3975229c7
Author: Robert Haas <rhaas@postgresql.org>
Date: Wed Nov 15 15:11:21 2017 -0500
Provide for forward compatibility with future minor protocol versions.
Previously, any attempt to request a 3.x protocol version other than
3.0 would lead to a hard connection failure, which made the minor
protocol version really no different from the major protocol version
and precluded gentle protocol version breaks. Instead, when the
client requests a 3.x protocol version where x is greater than 0, send
the new NegotiateProtocolVersion message to convey that we support
only 3.0. This makes it possible to introduce new minor protocol
versions without requiring a connection retry when the server is
older.
In addition, if the startup packet includes name/value pairs where
the name starts with "_pq_.", assume that those are protocol options,
not GUCs. Include those we don't support (i.e. all of them, at
present) in the NegotiateProtocolVersion message so that the client
knows they were not understood. This makes it possible for the
client to request previously-unsupported features without bumping
the protocol version at all; the client can tell from the server's
response whether the option was understood.
It will take some time before servers that support these new
facilities become common in the wild; to speed things up and make
things easier for a future 3.1 protocol version, back-patch to all
supported releases.
Robert Haas and Badrul Chowdhury
Discussion: http://postgr.es/m/BN6PR21MB0772FFA0CBD298B76017744CD1730@BN6PR21MB0772.namprd21.prod.outlook.com
Discussion: http://postgr.es/m/30788.1498672033@sss.pgh.pa.us
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 6d4dcf83ac..4fd7420f85 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -22,10 +22,18 @@
<productname>PostgreSQL</productname> 7.4 and later. For descriptions
of the earlier protocol versions, see previous releases of the
<productname>PostgreSQL</productname> documentation. A single server
- can support multiple protocol versions. The initial
- startup-request message tells the server which protocol version the
- client is attempting to use, and then the server follows that protocol
- if it is able.
+ can support multiple protocol versions. The initial startup-request
+ message tells the server which protocol version the client is attempting to
+ use. If the major version requested by the client is not supported by
+ the server, the connection will be rejected (for example, this would occur
+ if the client requested protocol version 4.0, which does not exist as of
+ this writing). If the minor version requested by the client is not
+ supported by the server (e.g. the client requests version 3.1, but the
+ server supports only 3.0), the server may either reject the connection or
+ may respond with a NegotiateProtocolVersion message containing the highest
+ minor protocol version which it supports. The client may then choose either
+ to continue with the connection using the specified protocol version or
+ to abort the connection.
</para>
<para>
@@ -406,6 +414,21 @@
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>NegotiateProtocolVersion</term>
+ <listitem>
+ <para>
+ The server does not support the minor protocol version requested
+ by the client, but does support an earlier version of the protocol;
+ this message indicates the highest supported minor version. This
+ message will also be sent if the client requested unsupported protocol
+ options (i.e. beginning with <literal>_pq_.</literal>) in the
+ startup packet. This message will be followed by an ErrorResponse or
+ a message indicating the success or failure of authentication.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
@@ -420,8 +443,10 @@
for further messages from the server. In this phase a backend process
is being started, and the frontend is just an interested bystander.
It is still possible for the startup attempt
- to fail (ErrorResponse), but in the normal case the backend will send
- some ParameterStatus messages, BackendKeyData, and finally ReadyForQuery.
+ to fail (ErrorResponse) or the server to decline support for the requested
+ minor protocol version (NegotiateProtocolVersion), but in the normal case
+ the backend will send some ParameterStatus messages, BackendKeyData, and
+ finally ReadyForQuery.
</para>
<para>
@@ -4704,6 +4729,74 @@ GSSResponse (F)
</listitem>
</varlistentry>
+<varlistentry>
+<term>
+NegotiateProtocolVersion (B)
+</term>
+<listitem>
+<para>
+
+<variablelist>
+<varlistentry>
+<term>
+ Byte1('v')
+</term>
+<listitem>
+<para>
+ Identifies the message as a protocol version negotiation
+ message.
+</para>
+</listitem>
+</varlistentry>
+<varlistentry>
+<term>
+ Int32
+</term>
+<listitem>
+<para>
+ Length of message contents in bytes, including self.
+</para>
+</listitem>
+</varlistentry>
+<varlistentry>
+<term>
+ Int32
+</term>
+<listitem>
+<para>
+ Newest minor protocol version supported by the server
+ for the major protocol version requested by the client.
+</para>
+</listitem>
+</varlistentry>
+<varlistentry>
+<term>
+ Int32
+</term>
+<listitem>
+<para>
+ Number of protocol options not recognized by the server.
+</para>
+</listitem>
+</varlistentry>
+</variablelist>
+ Then, for protocol option not recognized by the server, there
+ is the following:
+<variablelist>
+<varlistentry>
+<term>
+ String
+</term>
+<listitem>
+<para>
+ The option name.
+</para>
+</listitem>
+</varlistentry>
+</variablelist>
+</para>
+</listitem>
+</varlistentry>
<varlistentry>
<term>
@@ -5659,11 +5752,13 @@ StartupMessage (F)
</varlistentry>
</variablelist>
- In addition to the above, any run-time parameter that can be
- set at backend start time might be listed. Such settings
- will be applied during backend start (after parsing the
- command-line arguments if any). The values will act as
- session defaults.
+ In addition to the above, others parameter may be listed.
+ Parameter names beginning with <literal>_pq_.</literal> are
+ reserved for use as protocol extensions, while others are
+ treated as run-time parameters to be set at backend start
+ time. Such settings will be applied during backend start
+ (after parsing the command-line arguments if any) and will
+ act as session defaults.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 9906a85bc0..a3d4917173 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -101,6 +101,7 @@
#include "lib/ilist.h"
#include "libpq/auth.h"
#include "libpq/libpq.h"
+#include "libpq/pqformat.h"
#include "libpq/pqsignal.h"
#include "miscadmin.h"
#include "pg_getopt.h"
@@ -412,6 +413,7 @@ static void ExitPostmaster(int status) pg_attribute_noreturn();
static int ServerLoop(void);
static int BackendStartup(Port *port);
static int ProcessStartupPacket(Port *port, bool SSLdone);
+static void SendNegotiateProtocolVersion(List *unrecognized_protocol_options);
static void processCancelRequest(Port *port, void *pkt);
static int initMasks(fd_set *rmask);
static void report_fork_failure_to_client(Port *port, int errnum);
@@ -2052,12 +2054,9 @@ retry1:
*/
FrontendProtocol = proto;
- /* Check we can handle the protocol the frontend is using. */
-
+ /* Check that the major protocol version is in range. */
if (PG_PROTOCOL_MAJOR(proto) < PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST) ||
- PG_PROTOCOL_MAJOR(proto) > PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) ||
- (PG_PROTOCOL_MAJOR(proto) == PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) &&
- PG_PROTOCOL_MINOR(proto) > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST)))
+ PG_PROTOCOL_MAJOR(proto) > PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST))
ereport(FATAL,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("unsupported frontend protocol %u.%u: server supports %u.0 to %u.%u",
@@ -2079,6 +2078,7 @@ retry1:
if (PG_PROTOCOL_MAJOR(proto) >= 3)
{
int32 offset = sizeof(ProtocolVersion);
+ List *unrecognized_protocol_options = NIL;
/*
* Scan packet body for name/option pairs. We can assume any string
@@ -2128,6 +2128,16 @@ retry1:
valptr),
errhint("Valid values are: \"false\", 0, \"true\", 1, \"database\".")));
}
+ else if (strncmp(nameptr, "_pq_.", 5) == 0)
+ {
+ /*
+ * Any option beginning with _pq_. is reserved for use as a
+ * protocol-level option, but at present no such options are
+ * defined.
+ */
+ unrecognized_protocol_options =
+ lappend(unrecognized_protocol_options, pstrdup(nameptr));
+ }
else
{
/* Assume it's a generic GUC option */
@@ -2147,6 +2157,16 @@ retry1:
ereport(FATAL,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("invalid startup packet layout: expected terminator as last byte")));
+
+ /*
+ * If the client requested a newer protocol version or if the client
+ * requested any protocol options we didn't recognize, let them know
+ * the newest minor protocol version we do support and the names of any
+ * unrecognized options.
+ */
+ if (PG_PROTOCOL_MINOR(proto) > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST) ||
+ unrecognized_protocol_options != NIL)
+ SendNegotiateProtocolVersion(unrecognized_protocol_options);
}
else
{
@@ -2260,6 +2280,34 @@ retry1:
return STATUS_OK;
}
+/*
+ * Send a NegotiateProtocolVersion to the client. This lets the client know
+ * that they have requested a newer minor protocol version than we are able
+ * to speak. We'll speak the highest version we know about; the client can,
+ * of course, abandon the connection if that's a problem.
+ *
+ * We also include in the response a list of protocol options we didn't
+ * understand. This allows clients to include optional parameters that might
+ * be present either in newer protocol versions or third-party protocol
+ * extensions without fear of having to reconnect if those options are not
+ * understood, while at the same time making certain that the client is aware
+ * of which options were actually accepted.
+ */
+static void
+SendNegotiateProtocolVersion(List *unrecognized_protocol_options)
+{
+ StringInfoData buf;
+ ListCell *lc;
+
+ pq_beginmessage(&buf, 'v'); /* NegotiateProtocolVersion */
+ pq_sendint32(&buf, PG_PROTOCOL_LATEST);
+ pq_sendint32(&buf, list_length(unrecognized_protocol_options));
+ foreach(lc, unrecognized_protocol_options)
+ pq_sendstring(&buf, lfirst(lc));
+ pq_endmessage(&buf);
+
+ /* no need to flush, some other message will follow */
+}
/*
* The client has sent a cancel request packet, not a normal
^ permalink raw reply [nested|flat] 24+ messages in thread
* RE: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-11-20 00:49 Badrul Chowdhury <bachow@microsoft.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 24+ messages in thread
From: Badrul Chowdhury @ 2017-11-20 00:49 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
>> I spent a little more time looking at this patch today. I think that the patch
>> should actually send NegotiateProtocolVersion when *either* the requested
>> version is differs from the latest one we support *or* an unsupported protocol
>> option is present. Otherwise, you only find out about unsupported protocol
>> options if you also request a newer minor version, which isn't good, because it
>> makes it hard to add new protocol options *without* bumping the protocol
>> version.
It makes sense from a maintainability point of view.
>> Here's an updated version with that change and a proposed commit message.
I have tested the new patch and it works great. The comments look good as well.
Thanks,
Badrul
>> -----Original Message-----
>> From: Robert Haas [mailto:robertmhaas@gmail.com]
>> Sent: Wednesday, November 15, 2017 1:12 PM
>> To: Badrul Chowdhury <bachow@microsoft.com>
>> Cc: Tom Lane <tgl@sss.pgh.pa.us>; Satyanarayana Narlapuram
>> <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer
>> <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus
>> Hagander <magnus@hagander.net>; PostgreSQL-development <pgsql-
>> hackers@postgresql.org>
>> Subject: Re: [HACKERS] Re: protocol version negotiation (Re: Libpq
>> PGRES_COPY_BOTH - version compatibility)
>>
>> On Mon, Oct 30, 2017 at 9:19 PM, Badrul Chowdhury
>> <bachow@microsoft.com> wrote:
>> > Thank you for the comprehensive review! We are very much in the early
>> stages of contributing to the PG community and we clearly have lots to learn,
>> but we look forward to becoming proficient and active members of the pg
>> community.
>> >
>> > Regarding the patch, I have tested it extensively by hand and it works great.
>>
>> I spent a little more time looking at this patch today. I think that the patch
>> should actually send NegotiateProtocolVersion when *either* the requested
>> version is differs from the latest one we support *or* an unsupported protocol
>> option is present. Otherwise, you only find out about unsupported protocol
>> options if you also request a newer minor version, which isn't good, because it
>> makes it hard to add new protocol options *without* bumping the protocol
>> version.
>>
>> Here's an updated version with that change and a proposed commit message.
>>
>> --
>> Robert Haas
>> EnterpriseDB:
>> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.ent
>> erprisedb.com&data=02%7C01%7Cbachow%40microsoft.com%7Ce37b69223
>> a144d1e5b7408d52c6d8171%7C72f988bf86f141af91ab2d7cd011db47%7C1
>> %7C0%7C636463771208784375&sdata=1%2FDylQIfS2rI2RwIVyZnDCUbzRQJe
>> V4YM8J496QkpiQ%3D&reserved=0
>> The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
@ 2017-11-21 19:47 Robert Haas <robertmhaas@gmail.com>
parent: Badrul Chowdhury <bachow@microsoft.com>
0 siblings, 0 replies; 24+ messages in thread
From: Robert Haas @ 2017-11-21 19:47 UTC (permalink / raw)
To: Badrul Chowdhury <bachow@microsoft.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; pgsql-hackers
On Sun, Nov 19, 2017 at 7:49 PM, Badrul Chowdhury <bachow@microsoft.com> wrote:
>>> I spent a little more time looking at this patch today. I think that the patch
>>> should actually send NegotiateProtocolVersion when *either* the requested
>>> version is differs from the latest one we support *or* an unsupported protocol
>>> option is present. Otherwise, you only find out about unsupported protocol
>>> options if you also request a newer minor version, which isn't good, because it
>>> makes it hard to add new protocol options *without* bumping the protocol
>>> version.
>
> It makes sense from a maintainability point of view.
>
>>> Here's an updated version with that change and a proposed commit message.
>
> I have tested the new patch and it works great. The comments look good as well.
Committed and back-patched to all supported branches.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 24+ messages in thread
end of thread, other threads:[~2017-11-21 19:47 UTC | newest]
Thread overview: 24+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2017-06-28 17:24 protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility) Robert Haas <robertmhaas@gmail.com>
2017-06-28 17:47 ` Tom Lane <tgl@sss.pgh.pa.us>
2017-06-28 19:01 ` Robert Haas <robertmhaas@gmail.com>
2017-06-29 01:44 ` Craig Ringer <craig@2ndquadrant.com>
2017-06-29 01:55 ` Craig Ringer <craig@2ndquadrant.com>
2017-06-29 02:27 ` Tom Lane <tgl@sss.pgh.pa.us>
2017-06-29 04:23 ` Craig Ringer <craig@2ndquadrant.com>
2017-06-29 04:24 ` Craig Ringer <craig@2ndquadrant.com>
2017-06-29 12:18 ` Robert Haas <robertmhaas@gmail.com>
2017-06-29 23:29 ` Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>
2017-07-02 22:44 ` Robert Haas <robertmhaas@gmail.com>
2017-06-30 02:49 ` Craig Ringer <craig@2ndquadrant.com>
2017-10-04 01:35 Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility) Badrul Chowdhury <bachow@microsoft.com>
2017-10-04 01:46 ` Tom Lane <tgl@sss.pgh.pa.us>
2017-10-04 11:54 ` Robert Haas <robertmhaas@gmail.com>
2017-10-04 23:30 ` Badrul Chowdhury <bachow@microsoft.com>
2017-10-06 21:07 ` Badrul Chowdhury <bachow@microsoft.com>
2017-10-13 18:16 ` Robert Haas <robertmhaas@gmail.com>
2017-10-18 23:35 ` Badrul Chowdhury <bachow@microsoft.com>
2017-10-27 12:55 ` Robert Haas <robertmhaas@gmail.com>
2017-10-31 01:19 ` Badrul Chowdhury <bachow@microsoft.com>
2017-11-15 21:11 ` Robert Haas <robertmhaas@gmail.com>
2017-11-20 00:49 ` Badrul Chowdhury <bachow@microsoft.com>
2017-11-21 19:47 ` Robert Haas <robertmhaas@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox