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