On 11/27/22 17:02, Tomas Vondra wrote:
> Hi,
>
> I took a look at the v6 patch, with the intention to get it committed. I
> have a couple minor comments:
>
> 1) For PGLZ_HISTORY_SIZE it uses literal 0x0fff, with the explanation:
>
> /* to avoid compare in iteration */
>
> which I think means intent to use this value as a bit mask, but then the
> only place using PGLZ_HISTORY_SIZE does
>
> if (hist_next == PGLZ_HISTORY_SIZE) ...
>
> i.e. a comparison. Maybe I misunderstand the comment, though.
>
>
> 2) PGLZ_HistEntry was modified and replaces links (pointers) with
> indexes, but the comments still talk about "links", so maybe that needs
> to be changed. Also, I wonder why next_id is int16 while hist_idx is
> uint16 (and also why id vs. idx)?
>
> 3) minor formatting of comments
>
> 4) the comment in pglz_find_match about traversing the history seems too
> early - it's before handling invalid entries and cleanup, but it does
> not talk about that at all, and the actual while loop is after that.
>
> Attached is v6 in 0001 (verbatim), with the review comments in 0002.
>
BTW I've switched this to WoA, but the comments should be trivial to
resolve and to get it committed.
Also, I still see roughly 15-20% improvement on some compression-heavy
tests, as reported before. Which is nice.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
