Re: Compress ReorderBuffer spill files using LZ4

Hi Julien,

Thanks for the last patch version and sorry for the delay. Here's a
quick review, I plan to do a bit of additional testing, hopefully
sometime this week.

Attached is v4, which is your v3 rebased to current master, with a
couple "review" commits, adding comments to relevant places, which I
find easier to follow. There's also a "pgindent" commit at the end,
showing some formatting issues, adding structs to typedefs, etc. This
needs to be applied to the earlier patches, I didn't want to have too
many pgindent commits.

v4-0002-review.patch
--------------------

1) I don't think we need to rename ReorderBufferDiskChange to
ReorderBufferDiskHeader. It seems we could easily just add a field to
ReorderBufferDiskChange, and keep using that, no? It's still just a
wrapper for ReorderBufferChange, even if it's compressed.

2) Likewise, I don't see the point in moving ReorderBufferDiskChange to
a different file. It seems like something that should remain private to
reorderbuffer.c. IMHO the correct thing would be to move the various
ReorderBuffer* funct from reorderbuffer_compress.c to reorderbuffer.c.
The compression is something the ReorderBuffer is responsible for, so
reorderbuffer.c is the right place for that.

3) That means the reorderbuffer_compress.c would have only the actual
compression code. But wouldn't it be better to have one file for each
compression algorithm, similar to src/fe_utils/astreamer_{pglz,lz4,...}?
Just a suggestion, though. Not sure.

4) logical_decoding_spill_compression moved a bit, next to the variables
for other logical_decoding GUCs. It's also missing the definition in a
header file, so I get a compiler warning, so add it to reorderbuffer.h.

5) Won't the code in ReorderBufferCompress() do palloc/pfree for each
piece of data we compress? That seems it might be pretty expensive, at
least for records that happen to be large (>8kB), because oversized
chunks are not cached in the context. IMHO this should use a single
buffer, similarly to what astreamer_lz4_compressor_content does.

6) I'm really confused by having "streaming" and "regular" compression.
I know lz4 supports different modes, but I'd expect only one of them
being really suitable for this, so why support both? But even more
importantly, ReorderBufferCompress() sets the strategy using

#define lz4_CanDoStreamingCompression(s) \
(s < (LZ4_RING_BUFFER_SIZE / 2))

But it does that with "s" being the "change size", and that can vary
wildly. Doesn't that means the strategy will change withing a single
file? Does that even make sense?

v4-0004-review.patch
--------------------

7) I'm not sure the "Fix spill_bytes counter" is actually a fix. Even
now (i.e. without compression) it tracks the size of transactions, not
the bytes written to disk exactly, because it doesn't include the bytes
for the size field, so maybe we should not change that ...

v4-0006-review.patch
--------------------

8) It seems a bit strange that we add lz4 first, and only later pglz.
IMHO it should be the other way around, as pglz is the default algorithm
we can rely to have, while lz4 etc. are optional.

One question I have is whether it might be better to compress stuff at a
lower layer - not in reorderbuffer.c, but for the whole file. But maybe
there are reasons why that would be difficult, I haven't tried that.

regards

--
Tomas Vondra