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

Hi, Nikhil

On Fri, 25 Sep 2026 at 02:21, Nikhil Kumar Veldanda <veldanda.nikhilkumar17@gmail.com> wrote:
> On Fri, Sep 25, 2026 at 1:04 AM Michael Paquier <michael@paquier.xyz> wrote:
>>
>> On Thu, Sep 24, 2026 at 04:00:29PM -0700, Nikhil Kumar Veldanda wrote:
>> > 0001 removes toast_compress_header and the TOAST_COMPRESS_* macros in
>> > favor of the varatt.h accessors and
>> > toast_compress_set_size_and_method().
>>
>> In this one, I got mixed feelings about the addition of the new inline
>> routine in toast_internals.h while we have already a set of two getter
>> routines in varatt.h that interact with va_tcinfo, so I have moved an
>> equivalent to varatt.h.  An extra benefit of this move is that we
>> don't need to include toast_compression.h in toast_internals.h
>> anymore.  Applied the result.  Cool cleanup, thanks for that.
>
> Thanks.  That is a better home for it, and it sets the pattern for the
> long form: the format patch now adds
> VARDATA_COMPRESSED_SET_TCINFO_LONG() next to
> VARDATA_COMPRESSED_SET_TCINFO() in varatt.h, and
> toast_compress_datum() picks one or the other based on
> toast_compression_id_needs_cmid_byte(). The series does not touch
> toast_internals.h at all anymore.  One small change to the routine you
> committed: its assertion on the method becomes "cmethod <
> VARLENA_COMPRESS_METHOD_LONG", since from that patch on the value 3 is
> a flag for the long form, not a method.
>
>> > 0002 adds toast_pointer_build().
>>
>> +static varlena *
>> +toast_pointer_build(vartag_external tag, const void *fixed, Size fixedsize)
>> +{
>> +    varlena    *result;
>> +
>> +    Assert(tag == VARTAG_ONDISK_OID || tag == VARTAG_ONDISK_OID8);
>> +    Assert(VARTAG_SIZE(tag) == fixedsize);
>> +
>> +    result = (varlena *) palloc(VARHDRSZ_EXTERNAL + VARTAG_SIZE(tag));
>> +    SET_VARTAG_EXTERNAL(result, tag);
>> +    memcpy(VARDATA_EXTERNAL(result), fixed, fixedsize);
>> +
>> +    return result;
>> +}
>>
>> In this one, is there a need for fixedsize at all?  We can guess the
>> size of the target based on the vartag and VARTAG_SIZE(), without
>> needing an assertion.  If this brings any kind of extra protection, I
>> may have missed it..
>
> Only a cross-check that the struct the caller filled is the one the
> tag announces; without it, a mismatch would read four bytes past the
> caller's local.  With both callers sitting right next to the struct
> they fill, that is not worth a parameter.  Removed, and the size now
> comes from VARTAG_SIZE().  In the format patch the position of the
> method byte is derived from it as well, so the helper's inputs are
> down to the tag, the struct and the method.
>
>> Rename *fixed to *ptr?  Should this use a VARATT_IS_EXTERNAL_ONDISK()
>> in the first assertion instead of declaring the two vartags?
>>
>
> Both done.  VARATT_IS_EXTERNAL_ONDISK() wants a datum rather than a
> tag, so the assertion moves after SET_VARTAG_EXTERNAL() and checks the
> result.  A nice side effect is that the format patch does not need to
> touch that assertion when it adds the long tags.
>
>> > 0003 adds compress_method to toast_external_data and makes the three
>> > callers read it from there.
>>
>>                 /* Compressed attributes should have a valid compression method */
>> -               cmid = VARATT_EXTINFO_GET_COMPRESS_METHOD(toast_ext_data.extinfo);
>> +               cmid = toast_ext_data.compress_method;
>>
>> It looks like we could remove VARATT_EXTINFO_GET_COMPRESS_METHOD().
>> That's kind of tempting..
>
> Done in that patch.  Once the callers read toast_external_data, the
> only user left was toast_external_info_get() itself, which now shifts
> the bits directly, and keeping the function around would only invite
> code to bypass the struct.  The format patch was removing it anyway,
> so this just moves it earlier; the comment on
> VARATT_EXTINFO_GET_EXTSIZE() points at compress_method instead.
>
> v4 attached, rebased on e27f3b2cad7 and renumbered now that the first
> patch is in:
>
> 0001 adds toast_pointer_build().
> 0002 adds compress_method to toast_external_data and removes
> VARATT_EXTINFO_GET_COMPRESS_METHOD().
> 0003 is the format change.
> 0004 is zstd.
>

In the v4-0001 commit message, it says no behavior change.  However, it seems
an Assert introduced after setting vartag.

Since the access/detoast.h include the access/toast_compression.h, should we remove
the access/toast_compression.h in files that already include access/toash.h?

    $ grep -rl '#include[[:space:]]*[<"]access/detoast\.h[>"]' . \
      | xargs grep -l '#include[[:space:]]*[<"]access/toast_compression\.h[>"]'
    ./src/backend/access/brin/brin_tuple.c
    ./src/backend/access/common/toast_compression.c
    ./src/backend/access/common/detoast.c
    ./src/backend/access/common/toast_internals.c
    ./src/backend/utils/adt/varlena.c
    ./contrib/amcheck/verify_heapam.c

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.