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

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.

> --
> Michael

--
Nikhil Veldanda