On Tue, May 14, 2024 at 12:30 PM Jacob Burroughs <jburrou...@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
