Re: pglz compression performance, take two

From: Andres Freund <andres(at)anarazel(dot)de>
To: Andrey Borodin <amborodin86(at)gmail(dot)com>
Cc: Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, Ian Lawrence Barwick <barwick(at)gmail(dot)com>, Andrey Borodin <x4mmm(at)yandex-team(dot)ru>, 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: 2023-02-07 20:18:32
Message-ID: 20230207201832.cb42m4hknobq5e2t@awork3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

On 2023-02-05 10:36:39 -0800, Andrey Borodin wrote:
> On Fri, Jan 6, 2023 at 10:02 PM Andrey Borodin <amborodin86(at)gmail(dot)com> wrote:
> >
> > Hello! Please find attached v8.
>
> I got some interesting feedback from some patch users.
> There was an oversight that frequently yielded results that are 1,2 or
> 3 bytes longer than expected.
> Looking closer I found that the correctness of the last 3-byte tail is
> checked in two places. PFA fix for this. Previously compressed data
> was correct, however in some cases few bytes longer than the result of
> current pglz implementation.

This version fails on cfbot, due to address sanitizer:

https://cirrus-ci.com/task/4921632586727424
https://api.cirrus-ci.com/v1/artifact/task/4921632586727424/log/src/test/regress/log/initdb.log

performing post-bootstrap initialization ... =================================================================
==15991==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61e000002ee0 at pc 0x558e1b847b16 bp 0x7ffd35782f70 sp 0x7ffd35782f68
READ of size 1 at 0x61e000002ee0 thread T0
#0 0x558e1b847b15 in pglz_hist_add /tmp/cirrus-ci-build/src/common/pg_lzcompress.c:310
#1 0x558e1b847b15 in pglz_compress /tmp/cirrus-ci-build/src/common/pg_lzcompress.c:680
#2 0x558e1aa86ef0 in pglz_compress_datum /tmp/cirrus-ci-build/src/backend/access/common/toast_compression.c:65
#3 0x558e1aa87af2 in toast_compress_datum /tmp/cirrus-ci-build/src/backend/access/common/toast_internals.c:68
#4 0x558e1ac22989 in toast_tuple_try_compression /tmp/cirrus-ci-build/src/backend/access/table/toast_helper.c:234
#5 0x558e1ab6af24 in heap_toast_insert_or_update /tmp/cirrus-ci-build/src/backend/access/heap/heaptoast.c:197
#6 0x558e1ab4a2a6 in heap_update /tmp/cirrus-ci-build/src/backend/access/heap/heapam.c:3533
...

Independent of this failure, I'm worried about the cost/benefit analysis of a
pglz change that changes this much at once. It's quite hard to review.

Andres

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2023-02-07 20:20:19 Re: SLRUs in the main buffer pool - Page Header definitions
Previous Message Tom Lane 2023-02-07 20:15:53 Re: A bug in make_outerjoininfo