Date: Thu, 26 Jan 2023 00:28:46 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: gkokolatos@pm.me, Tomas Vondra <tomas.vondra@enterprisedb.com>,
	pgsql-hackers@lists.postgresql.org,
	Rachel Heaton <rachelmheaton@gmail.com>
Subject: Re: Add LZ4 compression in pg_dump
Message-ID: <20230126062846.GO22427@telsasoft.com>
References: <7zu6GYFi72jByVk2SEe_nd8r1DHhwA35vSntAE9sAfiMX9Lfk6bwimkwmu7GDgYaEvL27SE4qF1uK4_LHWQM4H-SXOwkMAI-GV8PKGmED_E=@pm.me>
 <d8cbc586-a8e3-1662-c158-9022d5c8de83@enterprisedb.com>
 <ce49a4f6-f7cc-28d7-3761-ddee7ef732af@enterprisedb.com>
 <MMLKPM28EzV5-NC4Jd3YdWCmPlUvbpbyqrugtklWTHZwF24g5x2IJ1J1wTICSESs8CaIRdj-56eDIqC4cTpahm55PmTVRik2D77X0ZKGeVo=@pm.me>
 <20230123180014.GX13860@telsasoft.com>
 <YiPrsNILgul-hbeyD6tfNaRUK5JZEhaZE36dLe1YJvKN_i0Tm2GuHd246MqcfNGFsjSY-rNo9n54MFCrRiRHRDkYyjgE4e29V-diAN2EcNU=@pm.me>
 <20230125014203.GL13860@telsasoft.com>
 <-myGeDiZ-ePDRyi18026Iu2HTTfmwfjWaJn4OWJ2F3oglZUfJTOQ7FARPW7gYxE4vK3b0HpwYoiHR45nfOWReRvvJZfCBfJzPP5iJyn4muk=@pm.me>
 <20230125180020.GF22427@telsasoft.com>
 <Y9IUZzQsusY3VuyJ@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y9IUZzQsusY3VuyJ@paquier.xyz>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: <https://www.postgresql.org/message-id/20230126062846.GO22427%40telsasoft.com>
Precedence: bulk

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