Horton

Thread

Commits

  1. Fix bug in archive streamer with LZ4 decompression

  1. Decompression bug in astreamer_lz4

    Mikhail Gribkov <youzhick@gmail.com> — 2025-06-25T20:07:32Z

    Hi hackers,

While developing my extension, I faced some problems in using
pg_basebackup/pg_verifybackup on a cluster with additional custom files
saved. The extension saves its own format files, which may have a variety
of sizes and usually quite low compression potential.

The same situation can be reproduced using randomly filled files. Let’s
imagine the extension saves its own statistics in pg_stat:
dd if=/dev/urandom of=$PGDATA/pg_stat/junk bs=1M count=16

Now we’ll run the server and make an lz4-packed backup:
$PGINSTALL/bin/pg_basebackup --no-sync -cfast --target
server:/tmp/server-backup -Xfetch --compress server-lz4

Now we’ll check the backup:
$PGINSTALL/bin/pg_verifybackup -n /tmp/server-backup

The verification process will fail. The exact error may vary, but the most
common version looks like this:
pg_verifybackup: error: checksum mismatch for file "pg_stat/junk" in
archive "base.tar.lz4"
pg_verifybackup: error: "base/4/3431" is present in the manifest but not on
disk
pg_verifybackup: error: "base/5/2620" is present in the manifest but not on
disk
pg_verifybackup: error: "base/1/2617" is present in the manifest but not on
disk
pg_verifybackup: error: "base/4/3576" is present in the manifest but not on
disk
......

Using gzip or zstd compressions in the same setup doesn’t lead to any
problems. Moreover, the lz4 archive itself is fine and it can be
successfully unpacked via cli lz4 unpacker. The problem thus is in reading,
and some investigation done by me and my colleagues lead us to
astreamer_lz4.c file.
Looks like astreamer_lz4_decompressor_content() function is initializing
_out-parameters incorrectly:
next_out = (uint8 *) mystreamer->base.bbs_buffer.data;
avail_out = mystreamer->base.bbs_buffer.maxlen;

, while should be:
next_out = (uint8 *) mystreamer->base.bbs_buffer.data +
mystreamer->bytes_written;
avail_out = mystreamer->base.bbs_buffer.maxlen - mystreamer->bytes_written;

And this would be consistent with what  the gzip version of the function
(astreamer_gzip_decompressor_content()) does.

In the vast majority of cases the mystreamer->bytes_written value is zero
at the moment of next_out/avail_out initialization, thus the problem does
not appear. In fact we were totally unable to reproduce non-zero initial
value of mystreamer->bytes_written without poor-compressible custom format
files.I’m not that deep in the compression algorithms to say exactly why is
that, and I realize that very few users will really face the problem, but
the problem is definitely there.

Attached is a simple patch implementing the above fix. The issue disappears
with it.
What do you think?

--
 best regards,
    Mikhail A. Gribkov

  2. Re: Decompression bug in astreamer_lz4

    Michael Paquier <michael@paquier.xyz> — 2025-06-25T23:29:37Z

    On Wed, Jun 25, 2025 at 11:07:32PM +0300, Mikhail Gribkov wrote:
> The same situation can be reproduced using randomly filled files. Let’s
> imagine the extension saves its own statistics in pg_stat:
> dd if=/dev/urandom of=$PGDATA/pg_stat/junk bs=1M count=16

Original, I like that.  It's not hard to imagine some extension code
that decides to dump at the root of the data folder some file using
one of the shutdown callbacks at the root of the data directory.  Not
seeing the problem would then be a matter of luck.  So yes, that's a
legit bug IMO.

> In the vast majority of cases the mystreamer->bytes_written value is zero
> at the moment of next_out/avail_out initialization, thus the problem does
> not appear. In fact we were totally unable to reproduce non-zero initial
> value of mystreamer->bytes_written without poor-compressible custom format
> files.I’m not that deep in the compression algorithms to say exactly why is
> that, and I realize that very few users will really face the problem, but
> the problem is definitely there.

That's not surprising, noted.

> Attached is a simple patch implementing the above fix. The issue disappears
> with it.

It seems like you have spent some time digging into all that, thanks
for all that.

> What do you think?

I think that it would be nice to have some test coverage for such
cases in pg_verifybackup for both the client side and the server side
of backups compressed, relying on the backend to generate some random
data dumped into a file at the root of a data folder.  You could just
plug in that into one of the scripts, before taking a backup, and
check all the compression methods we need to support.  See for example
brin.sql where with the combination of string_agg() and fipshash(), as
one reference.  So we could just reuse something like that.

I am detecting a threshold close to 512kB to be able to reproduce the
problem, so we don't need a file that large to see the failure, making
such tests faster with less data required as long as the file is
written once in a data folder used by all the backups taken:
$ rm $PGDATA/junk && dd if=/dev/urandom of=$PGDATA/junk bs=1k count=512
$ rm -r /tmp/client-backup-lz4/*
$ pg_basebackup --format=tar --no-sync -cfast \
   -Xfetch --compress client-lz4 -D /tmp/client-backup-lz4
$ pg_verifybackup -n /tmp/client-backup-lz4/
pg_verifybackup: error: checksum mismatch for file "junk" in archive "base.tar.lz4"
--
Michael

  3. Re: Decompression bug in astreamer_lz4

    Mikhail Gribkov <youzhick@gmail.com> — 2025-06-27T12:28:18Z

    Hi Michael,

> I think that it would be nice to have some test coverage for such
>   cases in pg_verifybackup for both the client side and the server side
>   of backups compressed, relying on the backend to generate some random
>   data dumped into a file at the root of a data folder.

Yes, good point. Added checks to both client and server untar tests.
I also noticed that the problem showup depends on compression level. For
example in 008_untar.pl and 010_client_untar.pl tests it only showed up
with compression levels 0 and 1.
It looks to me that we do not have any chances to check all possible
combinations of vital parameters, but at least we'll check several cases
that we know make a difference.

> I am detecting a threshold close to 512kB to be able to reproduce the
>   problem, so we don't need a file that large to see the failure, making
> such tests faster with less data required

I have raised the file size up to 640kB to make it a bit more reliable as
far as we are not sure about all lz4-internals that trigger the problem. It
still does not make the test data unreasonably large.

Attached is the updated version of the patch with test additions.

--
 best regards,
    Mikhail A. Gribkov


>

  4. Re: Decompression bug in astreamer_lz4

    Michael Paquier <michael@paquier.xyz> — 2025-07-02T05:05:13Z

    On Fri, Jun 27, 2025 at 03:28:18PM +0300, Mikhail Gribkov wrote:
> Attached is the updated version of the patch with test additions.

Thanks for the updated versions.  A bit more variance with the LZ4
levels, even if we cannot track everything, does not sound like a bad
thing to me for the server-side and client-side [de]compressions.  I
have reused what you have suggested, adjusting the tests in the
back-branches based on the fact that int->bytea casts don't exist up
to v17 and that the command structures were a bit different in test
008.

And fixed down to v15.
--
Michael