Re: Request for comment on setting binary format output per session

On Mon, 9 Oct 2023 at 22:25, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> We absolutely would
> need to update the documentation, and clients (like psql) really should
> be updated.

+1

> > I think one
> > could conclude on these facts either that (a) client_encoding is fine
> > and the problems with controlling behavior using that kind of
> > mechanism are mostly theoretical or
>
> I'm not clear on the exact rules for a protocol version bump and why a
> GUC helps us avoid one. If we have a binary_formats GUC, the client
> would need to know the server version and check that it's >=17 before
> sending the "SET binary_formats='...'" commmand, right?

I agree that we'd probably still want to do a protocol minor version
bump. FYI there is another thread trying to introduce protocol change
which needs a minor version bump. Patch number 0003 in that patchset
is meant to actually make libpq handle minor version increases
correctly. If we need a version bump than that would be useful[1].

> What's the
> difference between that and making it an explicit protocol message that
> only >=17 understand?

Honestly I think the main difference is the need to introduce this
explicit protocol message. If we do, I think it might be best to have
this be a way of setting a GUC at the Protocol level, and expand the
GucContext enum to have a way to disallow setting it from SQL (e.g.
PGC_PROTOCOL), while still allowing PgBouncer (or other poolers) to
change the GUC as part of the connection handoff, in a way that's
similar to what's being done for client_encoding now. We might even
want to make client_encoding PGC_PROTOCOL too (eventually).

Actually, for connection poolers there's other reasons to want to set
GUC values at the protocol level instead of SQL. Because the value of
the ParameterStatus response is sadly not a valid SQL string... That's
why in PgBouncer we have to re-quote the value [2], which is a problem
for any GUC_LIST_QUOTE type, which search_path is. This GUC_LIST_QUOTE
logic is actually not completely correct in PgBouncer and only handles
"" (empty search_path), because for search_path that's the only
reasonable problematic case that people might hit (e.g. truncating to
NAMELEN is another problem, but elements in search_path should already
be at most NAMELEN). But still it would be good not to have to worry
about that. And being able to send the value in ParameterStatus back
verbatim to the server would be quite helpful for PgBouncer.

[1]: https://www.postgresql.org/message-id/flat/CAFj8pRAX48WH5Y6BbqnZbUSzmtEaQZ22rY_6cYw%3DE9QkoVvL0A%40mail.gmail.com#643c91f84ae33b316c0fed64e19c8e49
[2]: https://github.com/pgbouncer/pgbouncer/blob/60708022d5b934fa53c51849b9f02d87a7881b11/src/varcache.c#L172-L183