Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs - Mailing list pgsql-hackers

On Thu, 6 Jun 2024 at 18:01, Robert Haas <robertmhaas@gmail.com> wrote:
> As I see it, the issue here is whether the default value would ever be
> different from the latest value. If not, then using blank to mean the
> latest seems fine, but if so, then I'd expect blank to mean the
> default version and latest to mean the latest version.

Alright, that's fair. And we already seem to follow that pattern:
There's currently no connection option that has a default that's not
the empty string, but still accepts the empty string as an argument.

> > I'll look into adding min_protocol_version to the patchset soonish.
> > Some review of the existing code in the first few patches would
> > definitely be appreciated.
>
> Yeah, I understand, and I do want to do that, but keep in mind I've
> already spent considerable time on this patch set, way more than most
> others, and if I want to get it committed I'm nowhere close to being
> done. It's probably multiple weeks of additional work for me, and I
> think I've probably already spent close to a week on this, and I only
> work ~48 weeks a year, and there are ~300 patches in the CommitFest.

I very much appreciate the time you spent on this patchset so far. I
mainly thought that instead of only discussing the more complex parts
of the patchset, it would be nice to also actually move forward a
little bit too. And the first 3 patches in this patchset are very
small and imho straightforward improvements.

To be clear, I'm not saying that should be all on you. I think those
first three patches can be reviewed by pretty much anyone.

> Plus, right now there is no possibility of actually committing
> anything until after we branch.

Totally fair, but even a LGTM on one of the patches would be quite nice.

> And, respectfully, I feel like there
> has to be some give and take here. I've been trying to give this patch
> set higher priority because it's in an area that I know something
> about and have opinions about and also because I can tell that you're
> kind of frustrated and I don't want you to leave the development
> community.

Thank you for giving it a higher priority, it's definitely appreciated
and noticed.

> But, at the same time, I don't think you've done much to
> help me get my patches committed, and while you have done some review
> of other people's patches, it doesn't seem to often be the kind of
> detailed, line-by-line review that is needed to get most patches
> committed. So I'm curious how you expect this system to scale.

Of course there's always the possibility to review more. But I don't
really agree with this summary of my review activity. I did see your
patches related to the incremental backup stuff. They looked
interesting, but at the time from an outside perspective it didn't
seem like those threads needed my reviews to progress (a bunch of
people more knowledgable on the topic were already responding). So I
spent my time mainly on threads where I felt I could add something
useful, and often that was more on the design front than the exact
code. Honestly that's what triggered this whole patchset in the first
place: Adding infrastructure for protocol changes so that the several
other threads that try to introduce protocol changes can actually move
forward, instead of being in limbo forever.

Regarding line-by-line reviews, imho I definitely do that for the
smaller patches I tend to review (even if they are part of a bigger
patchset). But the bigger ones I don't think line-by-line reviews are
super helpful at the start, so I generally comment more on the design
in those cases.