Re: Commitfest 2021-11 Patch Triage - Part 2

Greetings,

* Daniel Gustafsson (daniel@yesql.se) wrote:
> > On 15 Nov 2021, at 15:32, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Tue, Nov 9, 2021 at 9:12 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> >> 2773: libpq compression
> >> =======================
> >> This patch intended to provide libpq connection compression to "replace SSL
> >> compression" which was doomed when the patch was written, and have since been
> >> removed altogether.  The initial approach didn't get much traction but there
> >> was significant discussion and work, which has since fizzled out.  The patch
> >> has been updated but there hasn't been meaningful review the past months, the
> >> last comments seem to imply there being a fair amount of questionmarks left in
> >> here.  Robert, having been very involved in this do you have any thoughts on
> >> where we are and where to go (if at all IYO)?
> >
> > It's been a while since I looked at that patch, and my recollections
> > are somewhat fuzzy.
>
> No worries, thanks for sharing!
>
> > However, I think that my main feeling was that the
> > quality of the patch didn't seem to be that close to what I felt would
> > be required for a commit. I think at that point there were still
> > significant questions about the details of the approach as well as
> > code-quality and performance concerns. It may have gotten better since
> > last I looked.
>
> Context switching into this thread is hard, so it's not clear how much the
> patch has moved since you reviewed, or how the current patchset relates to the
> earlier one as there seems to have been a shift at a recent(ish) point in time.
> The last message (which is unreadable HTML soup in the archives) talks about
> implementing lz4 with "Looking forward to post an update soon."

Not sure where the best place to drop this comment is, but the patch
seems to be doing exactly what I was hoping it didn't- it's just taking
the ultimate results and compressing them and sending them on to the
client, at least that's what it looks like in a quick look.

What about when that data has already been compressed and stored
compressed and not manipulated at all?  The initial post in that thread
talks about returning data from 'large JSON columns'.  With this
approach, we would likely end up taking compressed json/jsonb data,
decompressing it, and then re-compressing it to send to the client.

That seems ... less than ideal.

I get that just compressing the entire stream is simpler and easier and
such, but it's surely cheaper and more efficient to not decompress and
then recompress data that's already compressed.  Finding a way to pass
through data that's already compressed when stored as-is while also
supporting compression of everything else (in a sensible way- wouldn't
make sense to just compress each attribute independently since a 4 byte
integer isn't going to get smaller with compression) definitely
complicates the overall idea but perhaps would be possible to do.

> Given the discussions had and the doubts over approach I wonder if the best
> course of action with the CF entry in question is to close it, hash out the
> what and how on -hackers and reopen a new entry when there is a patch which.
> We are not doing ourselves, or the CF process, any favors by moving an entry
> along which isn't all that close to committable.
>
> Thoughts (addressing everyone involved/interested)?

Yeah, agreed with this in general.

Thanks,

Stephen