Re: ZSTD TOAST compression, and an extensible compression method encoding - Mailing list pgsql-hackers

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.
While studying the patch series further, I noticed a few technical points
regarding error handling, extensibility, and edge-case slicing that might be
worth looking into:
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)?
    Size hdrsize = VARTAG_IS_ONDISK_LONG(toast_ext_data.tag) ?
                   (VARHDRSZ_COMPRESSED_LONG - VARHDRSZ) :
                   (VARHDRSZ_COMPRESSED - VARHDRSZ);
    slicelength += hdrsize;
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?
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()?
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.
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.
To protect against corrupted or truncated streams, should we also verify that
the decompressed size matches the recorded external size?
    if (ZSTD_isError(rawsize) || rawsize != (size_t) VARDATA_COMPRESSED_GET_EXTSIZE(value))
        ereport(ERROR, ...);
Thanks again for driving this work!