From: Justin Pryzby <pryzby@telsasoft.com>
To: gkokolatos@pm.me
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: pgsql-hackers@lists.postgresql.org, Rachel Heaton <rachelmheaton@gmail.com>
Subject: Re: Add LZ4 compression in pg_dump
Date: Thu, 26 Jan 2023 12:22:45 -0600
Message-ID: <20230126182245.GR22427@telsasoft.com> (raw)
In-Reply-To: <Y9JpuLGng5zLO4Mx@paquier.xyz> <4ZDVII-_kusch7xndcrTUt7OPSuQWQGX7n8rqp625Hdv6DijBjWZ38gjhxLSzPu8M_Dka-50eeKCiCU_dOdawwpv06dZtwGroYE9dHlIBLc=@pm.me>
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 will be 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, gkokolatos@pm.me, michael@paquier.xyz, tomas.vondra@enterprisedb.com, rachelmheaton@gmail.com
Subject: Re: Add LZ4 compression in pg_dump
In-Reply-To: <20230126182245.GR22427@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox