Re: ZSTD TOAST compression, and an extensible compression method encoding

On Mon, Sep 28, 2026 at 3:01 AM wenhui qiu <qiuwenhuifx(at)gmail(dot)com> wrote:
>
> HI Nikhil
>>
>>
>> Thanks for the clarification! You are completely right about the caller threshold
>> policy in toast_compress_datum() and the zstd frame overhead, and my apologies
>> for misquoting lz4_compress_datum(). Keeping the compressors loose and
>> centralizing the storage policy makes total sense.

No problem, thanks for looking again.

>> 1. Potential slice-fetch length mismatch in detoast.c (Patch 3/4)
>> In detoast.c, toast_fetch_datum_slice() accounts for the compressed header
>> overhead with:
>> if (VARATT_EXTERNAL_IS_COMPRESSED(toast_pointer) && slicelength > 0)
>> slicelength = slicelength + sizeof(int32);
>> For plain compressed datums (pglz/lz4), the header in chunk 0 is va_tcinfo
>> (4 bytes). But for the long-form encoding introduced in patch 3, toast_save_datum()
>> stores va_tcinfo (4 bytes) PLUS va_cmid (1 byte), totaling 5 bytes
>> (VARHDRSZ_COMPRESSED_LONG - VARHDRSZ).
>> Currently, detoast_attr_slice() fetches the entire external datum for lz4 and zstd
>> so the length is clamped at attrsize, but if toast_fetch_datum_slice() is used
>> to fetch a true slice of a long-form external datum, hardcoding sizeof(int32)
>> would result in fetching 1 byte short of the payload.
>> Should this instead be computed based on VARTAG_IS_ONDISK_LONG(toast_pointer)?

Yes. It can't happen today because only pglz fetches a true prefix
and pglz never uses the long form, but the code shouldn't depend on
that. Fixed in the format patch: the header size now follows the tag.

>> 2. Assertion failure during amcheck on corrupted on-disk datums (Patch 3)
>> In detoast.h, toast_external_info_get() has:
>> if (VARTAG_IS_ONDISK_LONG(toast_ext_data->tag))
>> {
>> uint8 cmid;
>> Assert(toast_ext_data->compress_method == VARLENA_COMPRESS_METHOD_LONG);
>> memcpy(&cmid, ptr + fixedsize, sizeof(cmid));
>> toast_ext_data->compress_method = (ToastCompressionId) cmid;
>> }
>> Because amcheck calls toast_external_info_get() to verify disk tuples, if an on-disk
>> datum is corrupted such that the tag is LONG but the va_extinfo bits were corrupted
>> to something else, an assert-enabled build will crash with AssertionFailed instead of
>> letting amcheck catch and report the corruption.
>> In contrast, VARDATA_COMPRESSED_GET_COMPRESS_METHOD() in varatt.h uses a non-asserting
>> "if (method == VARLENA_COMPRESS_METHOD_LONG)". Perhaps toast_external_info_get()
>> should do the same and set compress_method to TOAST_INVALID_COMPRESSION_ID on mismatch?

Yes. I reproduced it by clearing the method bits of a long pointer on
disk, the kind of damage src/bin/pg_amcheck/t/004_verify_heapam.pl
injects to check that amcheck reports corruption without crashing. An
assert-enabled build then hits TRAP: failed Assert(...) in
verify_heapam(); a production build doesn't crash, but reads the
method byte anyway and reports nothing. Fixed as you suggest: a
mismatch now decodes as TOAST_INVALID_COMPRESSION_ID, and amcheck
reports "toast value 16399 has invalid compression method id 3", the
same way it already reports a plain pointer whose bits hold 0b11.

>> 3. Hardcoded method enum in toast_save_datum() (Patch 3 & 4)
>> In toast_internals.c:
>> Assert(cmid == TOAST_PGLZ_COMPRESSION_ID ||
>> cmid == TOAST_LZ4_COMPRESSION_ID ||
>> cmid == TOAST_ZSTD_COMPRESSION_ID);
>> if (toast_compression_id_needs_cmid_byte(cmid))
>> Since the goal of patch 3 was to allow extensible compression methods without
>> hardcoding IDs across toast internals, should this assert simply be:
>> Assert(cmid != TOAST_INVALID_COMPRESSION_ID);
>> which matches the asserts in toast_compress_datum() and toast_pointer_build()?

I'd keep the list. Master already lists pglz and lz4 there. Unlike
in toast_compress_datum(), where cmid comes from the switch just
above, here it is read from the datum, so the list also catches values
that are not methods at all, which "!= INVALID" would let through. A
new method missing from it fails its first test, so it can't go stale.
The format patch is about deciding the pointer form in one place, not
about never naming methods; the dispatch switches and amcheck name
them too.

>> 4. Defensive underflow guard for VARSIZE in zstd_decompress_datum() (Patch 4)
>> In zstd_decompress_datum():
>> rawsize = ZSTD_decompress(VARDATA(result),
>> VARDATA_COMPRESSED_GET_EXTSIZE(value),
>> (const char *) value + VARHDRSZ_COMPRESSED_LONG,
>> VARSIZE(value) - VARHDRSZ_COMPRESSED_LONG);
>> Unlike LZ4_decompress_safe() where the compressed size argument is a signed int
>> (which immediately returns error if negative), ZSTD_decompress() takes size_t
>> (unsigned). If a corrupted on-disk datum has VARSIZE(value) < VARHDRSZ_COMPRESSED_LONG,
>> this underflows to a massive unsigned value, which could cause ZSTD_decompress()
>> to read out of bounds. Adding a check for VARSIZE(value) < VARHDRSZ_COMPRESSED_LONG
>> before calling ZSTD_decompress() would be safer.

Agreed, added to both zstd decompressors. pglz and lz4 get away with
it because they take a signed size.

>> 5. Decompressed size verification in zstd_decompress_datum() (Patch 4)
>> ZSTD_decompress() only returns an error if dstCapacity is too small; if the frame
>> decompresses to fewer bytes than dstCapacity (the recorded extsize), it returns the
>> smaller size without error.

Not quite. A zstd datum records its uncompressed size twice: in our
header (va_tcinfo) and in the zstd frame header, which ZSTD_compress()
always fills in. zstd checks its own copy: a truncated frame, or one
that decodes to a different length than it records, is an error.
Checked against libzstd 1.5.7, a frame cut by one byte fails with "Src
size is incorrect", and one whose recorded size I raised fails with
"Data corruption detected".

>> To protect against corrupted or truncated streams, should we also verify that
>> the decompressed size matches the recorded external size?

Yes, because zstd never sees our copy of the size: we only pass it as
dstCapacity, and a destination larger than needed is not an error. So
if our header claims more than the frame holds, zstd returns the real
data without complaint, and the mismatch goes unnoticed.

That has visible effects. In a manual test (not part of the patch), I
raised the size in the header of an inline 3450-byte zstd datum from
3450 to 3460 in the heap file, leaving the frame alone. Without the
check, the value reads back as the correct 3450 bytes, yet "v = <the
original text>" returns false: texteq() compares lengths first, via
toast_raw_datum_size(), which for a compressed datum takes the size
from our header without decompressing. With the check, the read fails
with "compressed zstd data is corrupt" instead. pglz already does the
same, through check_complete; lz4 doesn't. The slice path can't check
this, since it stops before the end by design.

v7 attached

--
Nikhil Veldanda