Re: Add LZ4 compression in pg_dump

On Thu, Jan 26, 2023 at 02:49:27PM +0900, Michael Paquier wrote:
> On Wed, Jan 25, 2023 at 12:00:20PM -0600, Justin Pryzby 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.
>
> Right. The impacts the possibility of looking at the header data,
> which is useful with pg_restore -l for example.

It's not just header data - it's schema and (I think) everything other
than table data.

> > The coverage report disagrees with me, though...
> > https://coverage.postgresql.org/src/bin/pg_dump/pg_backup_archiver.c.gcov.html#3901
>
> Isn't that one of the tests like compression_gzip_plain?

I'm not sure what you mean. Plain dump is restored with psql and not
with pg_restore.

My line number was wrong:
https://coverage.postgresql.org/src/bin/pg_dump/pg_backup_archiver.c.gcov.html#390

What test would hit that code without rebuilding ?

394 : #ifndef HAVE_LIBZ
395 : if (AH->compression_spec.algorithm == PG_COMPRESSION_GZIP &&

> Thoughts?
> #ifndef HAVE_LIBZ
> if (AH->compression_spec.algorithm == PG_COMPRESSION_GZIP)
> - pg_fatal("archive is compressed, but this installation does not support compression");
> + pg_log_warning("archive is compressed, but this installation does not support compression -- no data will be available");

Your patch is fine for now, but these errors should eventually specify
*which* compression algorithm is unavailable. I think that should be a
part of the 001 patch, ideally in a way that minimizes the number of
places which need to be updated when adding an algorithm.

--
Justin