Re: pglz compression performance, take two

From: Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>
To: Ian Lawrence Barwick <barwick(at)gmail(dot)com>, Andrey Borodin <x4mmm(at)yandex-team(dot)ru>
Cc: Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pglz compression performance, take two
Date: 2022-11-27 16:02:18
Message-ID: bcf3e6ee-44a1-5615-bb3d-0eb9531b6e8c@enterprisedb.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachment Content-Type Size
0001-Reorganize-pglz-compression-code-v6.patch text/x-patch 20.1 KB
0002-review-v6.patch text/x-patch 2.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2022-11-27 16:08:58 Re: pglz compression performance, take two
Previous Message Justin Pryzby 2022-11-27 15:40:53 Re: Add tracking of backend memory allocated to pg_stat_activity