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

[wenhui qiu]

Hi Nikhil,


Thanks for the updated patch set! Looking at patch 4/4, I noticed a minor
issue in zstd_compress_datum() regarding the check 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
compression benefit here.
If len == valsize, zstd_compress_datum() currently proceeds to call
SET_VARSIZE_COMPRESSED() and returns tmp, only for the caller
toast_compress_datum() to immediately reject it via if (VARSIZE(tmp) <
valsize - 2) and pfree(tmp). Freeing it early and returning NULL saves
redundant operations.
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;
 }