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;
}
