Re: Add LZ4 compression in pg_dump - Mailing list pgsql-hackers
From | Justin Pryzby |
---|---|
Subject | Re: Add LZ4 compression in pg_dump |
Date | |
Msg-id | 20230126182245.GR22427@telsasoft.com Whole thread Raw |
In response to | Re: Add LZ4 compression in pg_dump (gkokolatos@pm.me) |
Responses |
Re: Add LZ4 compression in pg_dump
Re: Add LZ4 compression in pg_dump |
List | pgsql-hackers |
On Wed, Jan 25, 2023 at 07:57:18PM +0000, gkokolatos@pm.me wrote: > On Wednesday, January 25th, 2023 at 7:00 PM, Justin Pryzby <pryzby@telsasoft.com> wrote: > > While looking at this, I realized that commit 5e73a6048 introduced a > > regression: > > > > @@ -3740,19 +3762,24 @@ ReadHead(ArchiveHandle *AH) > > > > - if (AH->compression != 0) > > > > - pg_log_warning("archive is compressed, but this installation does not support compression -- no data will be available"); > > + if (AH->compression_spec.algorithm == PG_COMPRESSION_GZIP) > > > > + pg_fatal("archive is compressed, but this installation does not support compression"); > > > > Before, it was possible to restore non-data chunks of a dump file, even > > if the current build didn't support its compression. But that's now > > impossible - and it makes the code we're discussing in RestoreArchive() > > unreachable. On Thu, Jan 26, 2023 at 08:53:28PM +0900, Michael Paquier wrote: > On Thu, Jan 26, 2023 at 11:24:47AM +0000, gkokolatos@pm.me wrote: > > I gave this a little bit of thought. I think that ReadHead should not > > emit a warning, or at least not this warning as it is slightly misleading. > > It implies that it will automatically turn off data restoration, which is > > false. Further ahead, the code will fail with a conflicting error message > > stating that the compression is not available. > > > > Instead, it would be cleaner both for the user and the maintainer to > > move the check in RestoreArchive and make it the sole responsible for > > this logic. > > - pg_fatal("cannot restore from compressed archive (compression not supported in this installation)"); > + pg_fatal("cannot restore data from compressed archive (compression not supported in this installation)"); > Hmm. I don't mind changing this part as you suggest. > > -#ifndef HAVE_LIBZ > - if (AH->compression_spec.algorithm == PG_COMPRESSION_GZIP) > - pg_fatal("archive is compressed, but this installation does not support compression"); > -#endif > However I think that we'd better keep the warning, as it can offer a > hint when using pg_restore -l not built with compression support if > looking at a dump that has been compressed. Yeah. But the original log_warning text was better, and should be restored: - if (AH->compression != 0) - pg_log_warning("archive is compressed, but this installation does not support compression -- no data willbe available"); That commit also added this to pg-dump.c: + case PG_COMPRESSION_ZSTD: + pg_fatal("compression with %s is not yet supported", "ZSTD"); + break; + case PG_COMPRESSION_LZ4: + pg_fatal("compression with %s is not yet supported", "LZ4"); + break; In 002, that could be simplified by re-using the supports_compression() function. (And maybe the same in WriteDataToArchive()?) -- Justin
pgsql-hackers by date: