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

On Wed, 12 Jun 2024 at 19:53, Robert Haas <robertmhaas@gmail.com> wrote:
> 0001 looks like a bug fix that can (and probably should) be committed
> and back-patched.

I moved this patch to its own thread, together with a bunch of other
fixes to the libpq tracing logic:
https://www.postgresql.org/message-id/flat/CAGECzQSoPHtZ4xe0raJ6FYSEiPPS%2BYWXBhOGo%2BY1YecLgknF3g%40mail.gmail.com

I left the patch numbering intact though.

> I agree with 0002 except for the change from PG_PROTOCOL_MINOR(proto)
> > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST) to proto > PG_PROTOCOL_LATEST.
> I prefer that test the way it is; I think the intent is clearer with
> the existing code.

Great to hear! I reverted that change to the check back to what it was now.

> > Patch 3: Similar to 1 & 2 in that it has no actual effect yet. But
> > after bumping the version this would be a user visible API change, so
> > I expect it requires a bit more discussion.
>
> I don't know if this is the right idea or not. An alternative would be
> to leave this alone and add PQprotocolMinorVersion().

I considered that, but that makes the API a lot more annoying to use
for end users when they want to compare to a version, especially when
they want to include the major version too.

PQprotocolVersion() >= 30001

vs

PQprotocolVersion() > 3 || (PQprotocolVersion() == 3 &&
PQprotocolVersionMinor() >= 1)

Given that PQprotocolVersion currently has no practical use, because
it always returns 3 in practice. I personally think that changing the
behaviour of API in the way I suggested is the best option here.

> > Patch 4: Adds a new connection option, but initially all parameters
> > that it takes have the same effect.
>
> Generally seems OK, but:

Fixed

> > Patch 5: Starts using the new connection option from Patch 4
>
> Not sure yet whether I like this approach.

Could you explain a bit more what you don't like about it?

> > Patch 6: Libpq changes to start handling NegotiateProtocolVersion in a
> > more complex way. (nothing changes in practice yet)
>
> + * NegotiateProtocolVersion message. So we only want to send a
>
> only->don't
>
> + * protocol version by default. Since either of those would cause a
>
> "default. Since" => "default, since"

Fixed

> I have some difficulty understanding how these calculations would
> produce different answers.

Oops, copy paste mistake, fixed

> + libpq_append_conn_error(conn, "invalid NegotiateProtocolVersion
> message, server version is newer than client version");
> + libpq_append_conn_error(conn, "invalid NegotiateProtocolVersion
> message, negative protocol parameter count");
> + libpq_append_conn_error(conn, "unexpected NegotiateProtocolVersion
> message, server supports requested features");
>
> These messages don't seem good.

Fair enough. I changed them a bit now, do you think they are good now?

> I don't think I completely understand what's going on in this patch
> yet. I'm not sure that it can be committed on its own, and I think it
> might need more polishing, including on comments and the commit
> message.

Yeah, I think it probably makes sense to combine this with 0008,
because there is so much interaction between the two. For now I
haven't done that yet though.

> > Patch 7: Bump the protocol version to 3.2 (see commit message for why
> > not bumping to 3.1)
>
> Good commit message. The point is arguable, so putting forth your best
> argument is important.

Just to clarify: do you agree with the point now, after that argument?

> > Patch 8: The main change: Infrastructure and protocol support to be
> > able to add protocol parameters
> > Patch 9: Adds a report_parameters protocol extension as a POC for the
> > changes in the previous patch.
>
> My general impression on first looking at these patches is that a lot
> of the ideas make sense but that they don't seem very close to being
> committable.

I totally agree that there's definitely significant work/discussion
that needs to happen before these are committable. Patch 8 was
basically my first implementation of my interpretation of our
in-person conversation at PGConf.dev. I mainly meant these last two
patches as an initial start for further discussion, and to see if this
was indeed the direction in which to progress. Sorry if I didn't make
that clear.

> It's not very clear how these new messages integrate into the overall
> protocol flow. The documentation makes the negative statement that
> they can't be used as part of the extended query protocol, but that
> just begs the question of where they can be used. I think there should
> be an update to protocol-flow.html here. For example, consider the
> "Simple Query" section of that page, which begins "A simple query
> cycle is initiated by the frontend sending a Query message to the
> backend." It goes on to describe what happens afterward. A similar
> discussion seems to be needed here, or maybe two of them,

I changed the second paragraph a bit to hopefully clarify this. Is it
indeed clearer like this?

> The patch touches src/interfaces/libpq (which is good) but does not
> update the libpq documentation (which is bad).

I definitely did update the libpq documentation for all the new public
C APIs, but it seems I forgot to add docs for the report_parameters
connection option, so I fixed that now.

Note: 0008 doesn't add any publicly facing libpq changes, so I don't
think updating the libpq docs make sense for that patch.

> The documentation for NegotiateProtocolParameter is almost identical
> to the documentation for SetProtocolParameterComplete. I would have
> expected the former to include a field giving guidance about values
> that might be legal in the future

Ugh, it seems I implemented the client side for that only half-baked
and didn't include it in the docs in the message specification (only
in the the report_parameters one). This is fixed now.

> and the latter to include an error
> message, rather than just an error indicator.

I did consider an error message (and I think that is what we discussed
in-person too). But during implementation a WARNING together with a
simple error indicator seemed nicer since that could hook into the
existing infrastructure for reporting warnings (both server and client
side). e.g. you can now provide detail/context/errorcode in the
warning, without having to add all those to the
SetProtocolParameterComplete message. I don't feel super strongly
either way though, so If you prefer the error message to be part of
the SetProtocolParameterComplete message then I'm happy to change
that.

> I wonder whether we could define 3.2 to report on all supported
> protocol parameters even if they weren't in the startup message, to
> avoid having to jam a lot of stuff we don't really care about into the
> startup message.

I think that's a good idea. I'll try to look into doing that soonish.