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

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

I took another look at 0002 today:

+ if (proto > PG_PROTOCOL_LATEST)
+ FrontendProtocol = PG_PROTOCOL_LATEST;

A few lines before this, we set FrontendProto = proto. Then we have
one error check, then this. How about instead changing the earlier
assignment to FrontendProto = Max(proto, PG_PROTOCOL_LATEST), or an
if-statement with similar effect? I don't think it practically matters
because send_message_to_frontend() looks well-equipped to handle a
garbage value in FrontendProto, but it seems cleaner to set the value
once and not change it than to change it and then almost immediately
change it again. Also, if we do that, we could fix the comment e.g.
"Set FrontendProtocol now so that ereport() knows what format to send
if we fail during startup, but limit the value to the newest protocol
version we actually support." And if we don't do that, then this new
if-statement needs a comment of its own, and it should explain why we
didn't choose to merge this with the earlier assignment.

- if (PG_PROTOCOL_MINOR(proto) >
PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST) ||
- unrecognized_protocol_options != NIL)
+ if (PG_PROTOCOL_MINOR(proto) >
PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST)
+ || unrecognized_protocol_options != NIL)

This hunk can be dropped.

- pq_sendint32(&buf, PG_PROTOCOL_LATEST);
+ pq_sendint32(&buf, FrontendProtocol);

This looks good, but the function header comment needs to be
corrected, e.g. "This lets the client know that they have either
requested a newer minor protocol version than we are able to speak, or
at least one protocol option that we don't understand, or possibly
both. FrontendProtocol has already been set to the version requested
by the client or the highest version we know how to speak, whichever
is older. If the highest version that we know how to speak is too old
for the client, it can abandon the connection."

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

I respect that, but I don't want to get flamed for doing something
that might be controversial without anybody else endorsing it. I'll
commit this if it gets some support, but not otherwise. I'm willing to
commit a patch adding a new function even if nobody else votes, but
not this.

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

Not entirely. The documentation of the environment variable gets the
name of the environment variable wrong.

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

Is this something that you still intend to do? It looks to me like
this would change some things as early as 0006.

Other things about 0006:

+ * Since some old servers and poolers don't support the
+ * NegotiateProtocolVersion message. So we don't want to send a

You could say "Some old servers ... message, so we don't" or "Some old
servers ... message. So, we don't" or "Since some old servers ...
message, we don't" but it's not quite grammatical the way it is.

+ * ask for the latest protocol version we support by
default too.

How about "default to the latest protocol version we support"?

+ for (const pg_protocol_parameter *param =
KnownProtocolParameters; param->name; param++)
+ {
+ const char *value = *(char **) ((char *) conn
+ param->conn_connection_string_value_offset);
+
+ if (value && value[0])
+ {
+ needs_new_protocol_features = true;
+ break;
+ }
+ }

I would personally prefer this if it were written with explicit tests,
like param->name != NULL and value[0] != '\0' but I realize that's not
everyone's preferred style.

I guess I'm also a little unsure whether we need to deal with both
NULL and "" here. Are they semantically different? Is it reasonable to
try to prevent NULL from occurring in the first place? Or maybe it's
fine the way it is. Not sure.

I also happened to notice that 0009 has a typo: report_paramters.

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