Horton

Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

Jelte Fennema-Nio <me@jeltef.nl>

View thread
From: Jelte Fennema-Nio <me@jeltef.nl>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Jacob Burroughs <jburroughs@instructure.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Dave Cramer <davecramer@gmail.com>, "Andrey M. Borodin" <x4mmm@yandex-team.ru>, Pavel Stehule <pavel.stehule@gmail.com>, Jeff Davis <pgsql@j-davis.com>, Peter Eisentraut <peter@eisentraut.org>
Date: 2024-01-10T22:53:50Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. libpq: Add min/max_protocol_version connection options

  2. libpq: Handle NegotiateProtocolVersion message differently

  3. Add PQfullProtocolVersion() to surface the precise protocol version.

  4. Do not hardcode PG_PROTOCOL_LATEST in NegotiateProtocolVersion

  5. libpq: Handle NegotiateProtocolVersion message

  6. Provide for forward compatibility with future minor protocol versions.

On Wed, 10 Jan 2024 at 22:12, Robert Haas <robertmhaas@gmail.com> wrote:
> Could you explain why you think that the protocol version bump is necessary?

Patch 0006 adds a new protocol message to the protocol. Somehow the
client needs to be told that the server understands that message.
Using the protocol version seems like the best/simplest/cleanest way
to do that to me.

In theory we could add a dedicated protocol extension parameter (e.g.
_pq_.enable_protocol_level_set) that the client would need to set to
true before it would be able to use ParameterSet. But that just sounds
like introducing unnecessary complexity to me.

Bumping the protocol version carries exactly the same level of risk as
adding new protocol extension parameters. Both will always allow old
clients to connect to the newer server. And both also allow a new
client to connect to an old server just fine as well, as long as that
server has ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed (which was
introduced in PG11.0 and was backpatched to all then supported PG
versions).

> > It now limits the possible GUCs that can be changed to PGC_USERSET and
> > PGC_SUSET. If desired, we could limit it even further by using an
> > allowlist of reasonable GUCs or set a flag on GUCs that can be
> > "upgraded" . Things that seem at least reasonable to me are "role",
> > "session_authorization", "client_encoding".
>
> I don't know whether that limit helps anything or not, and you haven't
> really said why you imposed it.

The main reason I did this is to make sure that the required context
can only be hardened, not softened. e.g. it would be very bad if
PGC_POSTMASTER GUCs could suddenly be changed with a protocol message.
So it was more meant as fixing a bug, than really reducing the number
of GUCs this has an impact on significantly.

> I'm still afraid that
> trying to allow this kind of nail-down for a broad range of GUCs (even
> if not all) is going to be messy. But I'm also plenty willing to
> listen to contrary opinions. I hear yours, but I wonder what others
> think? Tom particularly.

I wouldn't mind heavily reducing the GUCs that can be nailed-down like
this. For my usecase (connection pooling) I only really care about it
being possible to nail-down session_authorization.

Honestly, I care more about patch 0010 than patch 0008. Patch 0008
simply seemed like the easiest way to demonstrate the ParameterSet
message.