Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 14 Oct 2025 14:40:59 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: gkokolatos@pm.me, Justin Pryzby <pryzby@telsasoft.com>,
	Rachel Heaton <rachelmheaton@gmail.com>, Greg Stark <stark@mit.edu>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Add LZ4 compression in pg_dump
Message-ID: <aO3iaxTJ2LylgcLu@paquier.xyz>
References: <Yj6rXhLLW3h1vYt0@paquier.xyz>
 <20220326061441.GH28503@telsasoft.com>
 <YkK05K+0tjwmhLre@paquier.xyz>
 <yTrMijGhpyxYVVeJuJPM5NandUq_5Qq647cqNuUpVkXSfbYPDj0VLfRK72jB2HWoeDgRIYIbjG7YbBrUuzv8fRF9Y5vALIrM9BGIgQLh8vM=@pm.me>
 <YkPwmndklVKG4PE+@paquier.xyz>
 <bZeDkb5raMHiji1UIjvXjVIwc5qCHcdAb5h3CXXp85D8Mjwrm4kIGPh8UVbc2sJlfKgx2KsDHSrF35JAr-kFW5GJOx8YQlLKLv5vmAYLEjQ=@pm.me>
 <YkUTTK7dS6Zyeg9r@paquier.xyz>
 <syL-7HopUnzsy_Trvi6DON6tGwrcpujr-LhXo26AJ8AoP1fiGW66H33bMkPYU7tlu5HoqaSTt5FBeI6Ew1RLxnvTKy4-YZtBUJPgFMw3bQM=@pm.me>
 <YkucudXyfOwPOY5n@paquier.xyz>
 <25345.1760289877@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="oBmG+nYaUzkwoSlO"
Content-Disposition: inline
In-Reply-To: <25345.1760289877@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/aO3iaxTJ2LylgcLu%40paquier.xyz>
Precedence: bulk


--oBmG+nYaUzkwoSlO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sun, Oct 12, 2025 at 01:24:37PM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> At the end I am finishing with the attached.  I also saw an overlap
>> with the addition of --jobs for the directory format vs not using the
>> option, so I have removed the case where --jobs was not used in the
>> directory format.
>=20
> (This patch became commit 98fe74218.)
>=20
> I am wondering if you remember why this bit:
>=20
> +        # Give coverage for manually compressed blob.toc files during
> +        # restore.
> +        compress_cmd =3D> {
> +            program =3D> $ENV{'GZIP_PROGRAM'},
> +            args    =3D> [ '-f', "$tempdir/compression_gzip_dir/blobs.to=
c", ],
> +        },
>
> was set up to manually compress blobs.toc but not the main TOC in the
> toc.dat file.  It turns out that Gzip_read is broken for the case
> of a zero-length read request [1], but we never reach that case
> unless toc.dat is compressed.  We don't cover the getc_func member
> of the compression stream API, either.

Using extra commands to emulate the manual compressions of the dump
elements is an idea that comes originally from Georgios, and we
included in the first version of the patch posted on the original
thread:=20
https://www.postgresql.org/message-id/faUNEOpts9vunEaLnmxmG-DldLSg_ql137OC3=
JYDmgrOMHm1RvvWY2IdBkv_CRxm5spCCb_OmKNk2T03TMm0fBEWveFF9wA1WizPuAgB7Ss=3D@p=
rotonmail.com

If I recall correctly, it's something that the two of us have
discussed offline because Georgios had sent the patch to the lists.
He has argued about these additions in favor of coverage, because he
had noticed a bunch of edge cases we never reached with only the tests
in core while coding compression support.  Regarding toc.dat
specifically, I think that we have just failed to consider it as a
"valid" case, as far as I can see.  So I see no reason to not adding
coverage where toc.dat is manuall compressed, and you are giving a
good reason to add this part in the test.

> I thought for a bit about proposing that we compress toc.dat but not
> blobs.toc, but that loses coverage in another way: the gets_func API
> turns out to be used only while reading blobs.toc.  So we need to
> compress both files manually if we want full coverage.
>
> I think this change won't lose coverage, because there are other tests
> in 002_pg_dump.pl that exercise directory format without extra
> compression of anything.
>=20
> Thoughts?

It looks like it should be OK to just add a "manual" compression of
toc.dat in compression_gzip_dir.  That sounds OK to me here.  As far
as I can see on HEAD at 1c05fe11abb6, the problem has been fixed in
1f8062dd9668, without a test added for toc.dat.  Having coverage would
be nice.  I would also expand this concept to compression_zstd_dir and
compression_lz4_dir, but I guess that you are implying that already to
provide coverage for all the compression methods supported by pg_dump?=20
--
Michael

--oBmG+nYaUzkwoSlO
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmjt4msACgkQnvQgOdby
QH0lzQ//ddpjOLvU2IeOOjwq3WPAex3flOxCJBHplcZw4HRqGy2P1kXM+1Aaiy+A
XIiJzoFOA78FYLrTK8BRbMe9RHyJEz8lJUc39KNTxNH6j7pdL6STub/ABCujSU/V
9lKX89Jq6jB6GslJ6AKdaif00Ugi1pNgcqWMqMt1/EdF4Dgt4h5aYhvsBmjCoaFD
OtWkDY98LcwaxltxoMHAHOjjfpUqsCge4urMSYEsOCXKgGBsW4p8/3F8zTcqNL2i
Tx5pMBib1JUFU1kwULm2OML+/6NJs37otGeMm9jmISyOIYLmEEAq7naHE8yZNkfM
owg1f5hxa8CWNwPonYsDTed2uIx9+eOaAL7s8haK10uMpO56Dgh2yyYKG233kftz
/kUncq3mYGrvSfuHFhu5qtYriOUVi+Bkua+4wcxPI8He0FzfHZlhJuWABvsAepl9
iWlnyj9cZ8rZyahcuSZOP63hz/WCaeiPd1N9oqBxSKTDjgMtSXNY2wVURvQHYukP
Z4Rd1b+T7rhT3lxfRge/IMypRp7UcGnlIeXamI95Ylxb92tttDUwQnvH/nx4fFKU
i2KvTE3GKCBKfzsTSNEmnhoYU5B+BFsHja7Lzp6PeRR4hkMw/bRP0PPLhYOKXC+H
MZau8L2iorQ5lKlQp2ezftPrE9Gvx2aRxk6aAQfD19B1ibM9TX0=
=Bx1T
-----END PGP SIGNATURE-----

--oBmG+nYaUzkwoSlO--