Re: libpq compression (part 3) - Mailing list pgsql-hackers

On Tue, May 14, 2024 at 12:30 PM Jacob Burroughs
<jburroughs@instructure.com> wrote:
> I've withdrawn this patch from the commitfest.  I had been waiting for
> some resolution on "Add new protocol message to change GUCs for usage
> with future protocol-only GUCs" before I rebased/refactored this one,
> because this would be introducing the first protocol extension so far,
> and that discussion appeared to be working out some meaningful issues
> on how GUCs and protocol parameters should interact.  If you think it
> is worthwhile to proceed here though, I am very happy to do so. (I
> would love to see this feature actually make it into postgres; it
> would almost certainly be a big efficiency and cost savings win for
> how my company deploys postgres internally :) )

I don't think you should wait for that to be resolved; IMHO, this
patch needs to inform that discussion more than the other way around.

> > 2. I still think it needs to be more clear how the interface is
> > supposed to work. I do not want to spend time reviewing a patch to see
> > whether it works without understanding how it is intended to work --
> > and I also think that reviewing the patch in detail before we've got
> > the user interface right makes a whole lot of sense.
>
> Regarding the interface, what I had originally gone for was the idea
> that the naming of the options was from the perspective of the side
> you were setting them on.  Therefore, when setting `libpq_compression`
> as a server-level GUC, `compress` would control if the server would
> compress (send compressed data) with the given algorithm, and
> `decompress` would control if the the server would decompress (receive
> compressed data) with the given algorithm.  And likewise on the client
> side, when setting `compression` as a connection config option,
> `compress` would control if the *client* would compress (send
> compressed data) with the given algorithm, and `decompress` would
> control if the the *client* would decompress (receive compressed data)
> with the given algorithm.  So for a client to pick what to send, it
> would choose from the intersection of its own `compress=true` and the
> server's `decompress=true` algorithms sent in the `ParameterStatus`
> message with `libpq_compression`.  And likewise on the server side, it
> would choose from the intersection of the server's `compress=true`
> algorithms and the client's `decompress=true` algorithms sent in the
> `_pq_.libpq_compression` startup option.  If you have a better
> suggestion I am very open to it though.

Well, in my last response before the thread died, I complained that
libpq_compression=gzip:compress=off was confusing, and I stand by
that, because "compress" is used both in the name of the parameter and
as an option within the value of that parameter. I think there's more
than one acceptable way to resolve that problem, but I think leaving
it like that is unacceptable.

Even more broadly, I think there have been a couple of versions of
this patch now where I read the documentation and couldn't understand
how the feature was supposed to work, and I'm not really willing to
spend time trying to review a complex patch for conformity with a
design that I can't understand in the first place. I don't want to
pretend like I'm the smartest person on this mailing list, and in fact
I know that I'm definitely not, but I think I'm smart enough and
experienced enough with PostgreSQL that if I look at the description
of a parameter and say "I don't understand how the heck this is
supposed to work", probably a lot of users are going to have the same
reaction. That lack of understanding on my part my come either from
the explanation of the parameter not being as good as it needs to be,
or from the design itself not being as good as it needs to be, or from
some combination of the two, but whichever is the case, IMHO you or
somebody else has got to figure out how to fix it.

I do also admit that there is a possibility that everything is totally
fine and I've just been kinda dumb on the days when I've looked at the
patch. If a chorus of other hackers shows up and gives me a few whacks
with the cluestick and after that I look at the proposed options and
go "oh, yeah, these totally make sense, I was just being stupid," fair
enough! But right now that's not where I'm at. I don't want you to
explain to me how it works; I want you to change it in some way so
that when I or some end user looks at it, they go "I don't need an
explanation of how that works because it's extremely clear to me
already," or at least "hmm, this is a bit complicated but after a
quick glance at the documentation it makes sense".

I would really like to see this patch go forward, but IMHO these UI
questions are blockers.

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