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