On Tue, 5 Sept 2023 at 15:04, Aleksander Alekseev <aleksan...@timescale.com> wrote: > > Hi, > > I noticed that the patch needs review and decided to take a look.
Thanks for reviewing! > 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. I've updated the various comments with improved wording. > * `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. Fixed, it now uses `int` instead, as does XLogReadLength(). > * 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 I use READ_OP and WRITE_OP mostly to make sure that each operation's code is clear. Manually expanding the macro would allow the handling of each variant to have different structure code, and that would allow for more coding errors. I think it's extra important to make sure the code isn't wrong because this concerns WAL (de)serialization, and one copy is (in my opinion) easier to check for errors than 3 copies. I've had my share of issues in copy-edited code, so I rather like keep the template around as long as I don't need to modify the underlying code. > * `if (read < 0)` in DecodeXLogRecord() is noop since `read` is unsigned Yes, thanks for noticing. I've been working with Rust recently, where unsigned size is `usize` and `size` is signed. The issue has been fixed in the attached patch with 'int' types instead. Kind regards, Matthias van de Meent
v2-0001-Reduce-overhead-of-small-block-data-in-xlog-recor.patch
Description: Binary data