On Tue, 5 Sept 2023 at 15:04, Aleksander Alekseev
<aleksander@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
