Re: pglz compression performance, take two - Mailing list pgsql-hackers

From Andres Freund
Subject Re: pglz compression performance, take two
Date
Msg-id 20230207201832.cb42m4hknobq5e2t@awork3.anarazel.de
Whole thread Raw
In response to Re: pglz compression performance, take two  (Andrey Borodin <amborodin86@gmail.com>)
Responses Re: pglz compression performance, take two
List 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@gmail.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
sp0x7ffd35782f68
 
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



pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: A bug in make_outerjoininfo
Next
From: Andres Freund
Date:
Subject: Re: SLRUs in the main buffer pool - Page Header definitions