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.
> 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!
>
Best regards
