Re: pglz compression performance, take two

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