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

Hi, Nikhil

On Fri, 25 Sep 2026 at 02:21, Nikhil Kumar Veldanda <veldanda(dot)nikhilkumar17(at)gmail(dot)com> wrote:
> On Fri, Sep 25, 2026 at 1:04 AM Michael Paquier <michael(at)paquier(dot)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.