Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 18 Dec 2025 15:35:07 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Peter Eisentraut <peter@eisentraut.org>
Cc: Dharin Shah <dharinshah95@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Fwd: [PATCH] Add zstd compression for TOAST using extended
 header format
Message-ID: <aUOgmypuun0rc2xO@paquier.xyz>
References: 
 <CAOj6k6dy2CRVA6Lsb5N59zE-7KNVKt=oYwWyg8ULK8zOOY8e7A@mail.gmail.com>
 <CAOj6k6dkseVvZzmEAWvBd6twZsCU0DbN+qeM7CoDuMM3r9doiw@mail.gmail.com>
 <CAOj6k6eSsQ_PTUTxW0upn6wp7tzbvQwqkQco-=+majwX8p6JJg@mail.gmail.com>
 <4c70b0f6-5aba-464c-b145-464a620c1222@eisentraut.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="poFDZ5OKkNBF+pHp"
Content-Disposition: inline
In-Reply-To: <4c70b0f6-5aba-464c-b145-464a620c1222@eisentraut.org>
Archived-At: 
 <https://www.postgresql.org/message-id/aUOgmypuun0rc2xO%40paquier.xyz>
Precedence: bulk


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

On Wed, Dec 17, 2025 at 04:11:38PM +0100, Peter Eisentraut wrote:
> On 16.12.25 11:51, Dharin Shah wrote:
> > - Zstd only applies to external TOAST, not inline compression. The 2-bit
> > limit in va_tcinfo stays as-is for inline data, where pglz/lz4 work fine
> > anyway. Zstd's wins show up on larger values.
>=20
> This is a very complicated patch.  To motivate it, you should show some
> detailed performance measurements that show these wins.

Yes, this is expected for any patch posted.  Zstd is an improved
version of lz4, acting as a sort of industry standard these days, and
any byte sequences I have looked at points that zstd leads kind of
always to a better compression ratio for less or equivalent CPU cost
compared to LZ4.  Not saying that numbers are not required, they are.
But I strongly suspect numbers among these lines.

FWIW, it's not a complicated patch, it is a large mechanical patch
that enforces a bunch of TOAST code paths to do what it wants.  If we
are going to do something about that and agree on something, I think
that we should just use a new vartag_external for this matter
(spoiler: I think we should use a new vartag_external value), but
keep the toast structure at 16 bytes all the time, leaving alone the
extra bit in the existing varatt_external structure so as there is no
impact for heap relations if zstd is used, as long as the TOAST value
is 32 bits.  The patch introduces a new vartag_external with
VARTAG_ONDISK_EXTENDED, so while it leads to a better compatibility,
it also means that all zstd entries have to pay an extra amount of
space in the main relation as an effect of a different
default_toast_compression.  The difficulty is not in the
implementation, it would be on agreeing on what folks would be OK
with in terms if vartag and varatt structures, and that's one of the
oldest areas of the PG code, that has complications and assumptions of
its own.

The test implementation looks wrong to me.  Why is there any need for
an extra test module test_toast_ext?  You could just reuse the same
structure as compression_lz4.sql, but adapted to zstd.  That's why a
split with compression.sql has been done in 74a3fc36f314, FYI.

You should also aim at splitting the patch more to make it easier to
review: one of the sticky point of this area of the code is to untie
completely DefaultCompressionId, its GUC and the TOAST code.  The GUC
default_toast_compression accepts by design only 4 values.  This needs
to go further, and should be refactored as a piece of its own.

And also, I would prefer if the 32-bit value issue is tackled first,
but that's a digression here, for a different thread.  :)=20
--
Michael

--poFDZ5OKkNBF+pHp
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmlDoJsACgkQnvQgOdby
QH1zdA//S3P5T02bCetbVjjpat0THm29qesi5TgTuwAvufR18XymrUIGHc4EWlEJ
mdnk6FDLwb7jSmhOHVRZXqBn2Q2veQH/nJMyrxYQjYJ3e1wR5DWHqilUQQ4h+0hb
nUqW1ZfPV0WzaV7lefhK6bUG0+HCs3i0yhjMVXEVgMZMFxt87MfN6qQ+F1SqHTef
sZsBVsq9U9u74Me2WBWG4sEGgkWDWXOcVBUs9kAaAUV/5JhBPz50Kv5rm35Ne6hT
ycvYs//RqhCZ9zIXqAQB0UCfUGb0Zd5ahndNNW6ewcP0liEfWXRgYxNPAJTBC75r
b/4s/jZSw6Z3vBc/G0WNtMaXy4DO0/0qBOEwUsENoX+Fr0RLMGhub3To5WH3clXK
Q0MUbrRTpheRo4/oVXC/oIzjAXqbKXsfB9BZCEQW2dQmK1qpBPA1D83Pgnsu8w7W
bY6/zf/l2cyAwl1OkGxOZXphy2HE0lTnwfcqyfha6PlLLuBhlah4UMii3Wk7IWsV
f/1Z0N0iZLpj3pfJpWYoFfR90F4UFYc8/PxRq0W9BQ2HJ2EtyyunHM8lqO3loVdv
LbddC73mOLjB8l9yC8f1uuD8jeyTKFy3Td0YYw81mLqgCDuTzTEN/1dbrYuLs+p3
IOsVjtjbr/LrHmoRussAvyfcm3hGPR/zzYL+GvEhUUbcrDe06qo=
=b7Z3
-----END PGP SIGNATURE-----

--poFDZ5OKkNBF+pHp--