Thanks Michael for feedback.
On Wed, Sep 23, 2026 at 11:12 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> +static varlena *
> +toast_pointer_build(vartag_external tag, const void *fixed, Size fixedsize)
>
> Hmm, why not. It is a pattern in two places. You raise it to 4, one
> for each vartag.
Right, that is what made a helper worthwhile: with the long tags it
would otherwise be four copies of the same three lines.
>
> How about splitting 0001 into a few more pieces, for clarity? It
> looks like we could do the removal of toast_compress_header to the new
> toast_compress_set_size_and_method() as a change of its own. The
> introduction of toast_pointer_build() is a second one. I am not sure
> that the new VARTAG_IS_ONDISK() buys us much, we have already too many
> of these. Adding compress_method to toast_external_data and reducing
> the footprint of VARATT_EXTINFO_GET_COMPRESS_METHOD() would be a third
> one.
Done, in that order, in the attached v3:
0001 removes toast_compress_header and the TOAST_COMPRESS_* macros in
favor of the varatt.h accessors and
toast_compress_set_size_and_method().
0002 adds toast_pointer_build().
0003 adds compress_method to toast_external_data and makes the three
callers read it from there.
0004 and 0005 are the format change and zstd, as before.
VARTAG_IS_ONDISK() is gone. With two tags the comparison is fine as
HEAD spells it, so the three refactoring patches no longer touch
varatt.h at all.
0004 adds two VARTAG_IS_* helpers, down from four in v2:
- VARTAG_IS_ONDISK_OID8() is true for the two oid8 tags, plain and
long. toast_external_info_get() uses it to pick the oid8 struct, and
amcheck uses it to compare the tag with the chunk_id type of the TOAST
table.
- VARTAG_IS_ONDISK_LONG() is true for the two long tags. It tells
toast_external_info_get() to read the method byte,
toast_pointer_build() to write it, and amcheck that the value must be
compressed.
Each one compares the tag with its two enum values by name. The
low-bit trick is gone, so the numbering of the long tags next to the
plain ones is only a mnemonic now. VARATT_IS_EXTERNAL_ONDISK() accepts
all four tags, and amcheck's "unexpected TOAST tag" check calls it
instead of keeping its own list.
> > 0002: Allow more than four TOAST compression methods
> > ----------------------------------------------------
>
> Seems like HEAD disagrees with this one in varatt.h and amcheck.
Sorry about that. v2 was generated from a tree that predated
be00f041a33, which touched the two comment lines that the patch edits.
v3 applies on HEAD as of 4545cee303c. While there I fixed a doubled
word that my reflow of the amcheck comment had introduced.
> > The inline long form is described by a new struct:
> > typedef struct
> > {
> > uint32 va_header;
> > uint32 va_tcinfo; /* method bits =
> > VARLENA_COMPRESS_METHOD_LONG */
> > uint8 va_cmid; /* compression method ID */
> > char va_data[FLEXIBLE_ARRAY_MEMBER];
> > } varattrib_4b_long;
>
> This is the fundamental part of the change for me. We just have one
> bit left that gets redirected to this new "long" flavor, where we have
> to pay a penalty of 1 byte to be able to store compression methods
> other than pglz and lz4.
Yes. That is the whole of the format change; the rest of 0004 is
plumbing for it.
> Honestly, I don't care much about the level setting. If really
> needed, we could bundle that as a GUC but i really doubt that people
> are going to bother about tuning it. The default is usually good
> enough, as support for compression of full pages in WAL has
> demonstrated over the years.
Agreed, it stays out. Nothing in the format depends on it, so it can
be added later as a GUC or an attribute option.
> An integration with dictionaries worry me a lot, because only somewhat
> make sense when attached at tuple-level, and they evolve depending on
> how zstd see fit. What worries me the most is the amount of work
> between the on-disk data and the backend in terms of retention of past
> dictionnaries.
I understand that, and this series does not try to answer it. How
dictionaries would be stored, versioned and retired is a separate
design, and I'd rather not touch it until plain zstd is in.
> Michael
--
Nikhil Veldanda
