On Fri, Sep 25, 2026 at 5:35 AM wenhui qiu <qiuwenhuifx@gmail.com> wrote:
>
> Hi Nikhil,
>
>
> Thanks for the updated patch set! Looking at patch 4/4, I noticed a minor issue in zstd_compress_datum() regarding
thecheck for incompressible data:
>
> /* data is incompressible so just free the memory and return NULL */
> if (len > (size_t) valsize)
> {
> pfree(tmp);
> return NULL;
> }
> Should this condition be >= instead of >? if (len >= (size_t) valsize)
> Reasons:
> If len == valsize, the compressed payload alone is already the same size as the uncompressed data. Once
VARHDRSZ_COMPRESSED_LONG(9 bytes) is added, the compressed datum is strictly larger than the original. There is no
compressionbenefit here.
> If len == valsize, zstd_compress_datum() currently proceeds to call SET_VARSIZE_COMPRESSED() and returns tmp, only
forthe caller toast_compress_datum() to immediately reject it via if (VARSIZE(tmp) < valsize - 2) and pfree(tmp).
Freeingit early and returning NULL saves redundant operations.
The check in the compressors is deliberately loose. The real decision
is made once for all methods in toast_compress_datum(), which knows
about the header and the alignment margin and rejects anything that
does not save more than 2 bytes with the header included. So for zstd
every result from len == valsize - 11 upwards is thrown away there.
Changing > to >= in the compressor catches one of those twelve
lengths; the other eleven still take the round trip. To actually
avoid it, each compressor would need a copy of the caller's threshold,
which is what keeping the policy in one place is meant to avoid.
The saving would also be one store and one comparison in a case that
does not happen in practice: zstd's frame overhead means
incompressible input comes out strictly longer than valsize.
> This is also consistent with lz4_compress_datum() in the same file:
>
> /*
> * If the compressed size is greater than or equal to the raw data size,
> * then data is incompressible so just free the memory and return NULL.
> */
> if (len >= valsize)
> {
> pfree(tmp);
> return NULL;
> }
lz4_compress_datum() in master has "if (len > valsize)", unchanged
since bbe0a81db69 added it. The zstd check is a copy of it, so the
two are consistent as they stand.
--
Nikhil Veldanda
