| From: | Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com> |
|---|---|
| To: | Jelte Fennema-Nio <postgres(at)jeltef(dot)nl> |
| Cc: | Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Robert Haas <robertmhaas(at)gmail(dot)com> |
| Subject: | Re: libpq: Bump protocol version to version 3.2 at least until the first/second beta |
| Date: | 2026-01-30 18:06:30 |
| Message-ID: | CAOYmi+mXccgHiWppypxYgxHNr+Qs+1VJ+UEPJAHQHgj2U38NPw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Sat, Jan 24, 2026 at 2:10 AM Jelte Fennema-Nio <postgres(at)jeltef(dot)nl> wrote:
> A few thoughts on the docs:
> 1. I like the version table split. I think it might be better to do
> the same for protocol extensions too. Primarily because I think the
> tiny empty cell at the start of each row looks weird (see screenshot),
I figured that would be a sticking point. Docbook really does not want
to help me out here.
> but also just for consistency.
As long as the consistency provides *clarity*, I'm fine with it; I'm
just not sure that it does. I'll post a v6 with some screenshots.
> 2. In my v5 I created a dedicated section header for protocol
> extensions instead of including it in the extension. I think that's
> slightly nicer, primarily so you can link to that section from the
> StartupMessage docs (including the introductory paragraph), instead of
> having to link to the table.
Agreed, will fix.
> 3. In the table in my v5 I use "extension name" as a column instead of
> "parameter name" so I did not have to include the "_pq_." prefix. I'm
> going a bit back and forth between which I like better.
I feel more strongly about including `_pq_.` in the full parameter
name, because the namespace isn't immediately clear otherwise. Adding
a note in the paragraph above doesn't immediately broadcast that every
single _pq_ parameter is reserved for future protocol work.
(Also, I wonder if we should eventually move the "currently recognized
application-level parameter" table out of StartupMessage, and the
prefix inclusion is even more important then. I'd feel better about
doing that if the docs were a little easier to navigate; there was a
Discord conversation along those lines semi-recently. Something for
another thread.)
> A few thoughts on the implementation:
> 1. If you like the randomization I did in my v5-0003
From a quick skim, I do like it, thank you! My favorite part is the
ability to send multiple grease parameters. I think the minor version
randomization is probably a weaker aspect of the patch, because the
difference in difficulty between "==" and ">=" in a misbehaving server
is much less than our new maintenance cost for randomizing it.
> but don't want
> to commit it yet.
I won't focus on that just yet. I'd like opinions from other people,
and at least one other maintainer, on a proposed
`max_protocol_version=grease` for production use.
> Then I think it would be good to change the reserved
> protocol extension to rename to _pq_.test_protocol_negotiation_0000.
> So that if we commit it later we don't have both
> _pq_.test_protocol_negotiation and _pq_.test_protocol_negotiation_XXXX
> reserved, but only have the one with a suffix reserved.
I think we can safely include the shorter one in a future suffix
reservation, just by moving where the "wildcard" is, so I'm not too
worried about that at the moment. But let me know if you feel strongly
about it.
> 2. It might be nice to also error duplicate keys in NegotiateProtocolVersion.
I'm skeptical unless
- we change the protocol itself to disallow duplicate parameters, in
which case we don't have to grease it; or
- you know of a specific reason a duplicate key in NPV could cause
interoperability problems?
(For greenfield protocol design, I'm right there with you.)
Thanks,
--Jacob
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Greg Burd | 2026-01-30 18:22:56 | Re: Refactor how we form HeapTuples for CatalogTuple(Insert|Update) |
| Previous Message | Álvaro Herrera | 2026-01-30 17:42:27 | Re: Set 1s WaitLatch timeout if standby limit has expired in ResolveRecoveryConflictWithBufferPin |