Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs - Mailing list pgsql-hackers

On Mon, Jun 24, 2024 at 9:19 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> > > 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.

Mmm, I was thinking of defining the new function to return the major
and minor version in one number, while the existing function could
continue to work as now. I see your point too, but I'd like to hear
some other opinions before we decide, because I think it's unclear
what is actually best here. And also: surely this is not the hill to
die on. If it makes others a lot happier to do it one way or the
other, I'm severely disinclined to spend energy arguing with those
people. And, on the basis of previous experience, this is exactly the
sort of question about which opinions sometimes run quite strongly. I
would much rather swim with the current than get what I would myself
prefer.

> > > 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?

I don't dislike it; I'm just not sure whether it is the best approach
to testing the remainder of the patch series. Perhaps the commit
message could explain more why this approach was chosen and what value
we get from it.

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

I'll try to re-review the patch set when I have a bit more time than I
do at this exact moment.

> > > 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?

Well, here again, I would like to know what other people think. It
doesn't intrinsically matter to me that much what we do here, but it
matters to me a lot that extensive recriminations don't ensue
afterwards.

> 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.

My general programming experience has been that any time I decide to
include an error flag rather than an error message, I end up
regretting it. It's possible that this case is, for some reason, an
exception to that principle, but I feel like we need to nail down
exactly what the protocol flow *and* the libpq API for these new
messages is going to be before I can really have an intelligent
opinion about that.

--
Robert Haas
EDB: http://www.enterprisedb.com