Hi,
I noticed that the patch needs review and decided to take a look.
> No meaningful savings in the pgbench workload, mostly due to xlog
> record length MAXALIGNs currently not being favorable in the pgbench
> workload. But, record sizes have dropped by 1 or 2 bytes in several
> cases, as can be seen at the bottom of this mail.
This may not sound a lot but still is valuable IMO if we consider the
reduction in terms of percentages of overall saved disk throughput,
network traffic, etc, not in absolute values per one record. Even if
1-2 bytes are not a bottleneck that can be seen on benchmarks (or the
performance improvement is not that impressive), it's some amount of
money paid on cloud. Considering the fact that the patch is not that
complicated I see no reason not to apply the optimization as long as
it doesn't cause degradations.
I also agree with Matthias' arguments above regarding the lack of
one-size-fits-all variable encoding and the overall desire to keep the
focus. E.g. the code can be refactored if and when we discover that
different subsystems ended up using the same encoding.
All in all the patch looks good to me, but I have a couple of nitpicks:
* The comment for XLogSizeClass seems to be somewhat truncated as if
Ctr+S was not pressed before creating the patch. I also suggest
double-checking the grammar.
* `Size written = -1;` in XLogWriteLength() can lead to compiler
warnings some day considering the fact that Size / size_t are
unsigned. Also this assignment doesn't seem to serve any particular
purpose. So I suggest removing it.
* I don't see much value in using the WRITE_OP macro in
XLogWriteLength(). The code is read more often than it's written and I
wouldn't call this code particularly readable (although it's shorter).
* XLogReadLength() - ditto
* `if (read < 0)` in DecodeXLogRecord() is noop since `read` is unsigned
--
Best regards,
Aleksander Alekseev
