Re: Refactor code around GUC default_toast_compression

> On May 11, 2026, at 11:06, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Sat, May 02, 2026 at 09:55:30AM +0800, Chao Li wrote:
>> Otherwise, a third-party extension that relies on this variable
>> could silently misbehave. I understand that a major release is
>> allowed to change API/ABI contracts, but a build failure would be
>> better than silent misbehavior. Or at least we should document this
>> change somewhere.
>>
>> Would it better to also rename DEFAULT_TOAST_COMPRESSION to DEFAULT_TOAST_COMPRESSION_GUC.
>
> After pondering about this point, I think that you are touching
> something sensible here, but not for the reason you mention: the _GUC
> bits serve no actual purpose and we can keep using attcompression in
> the GUC.
>
>> 3
>> ```
>> +#define TOAST_COMPRESS_PGLZ 0
>> +#define TOAST_COMPRESS_LZ4 1
>> +#define TOAST_COMPRESS_INVALID 2
>> ```
>>
>> Now TOAST_COMPRESS_PGLZ is 0, and TOAST_PGLZ_COMPRESSION is
>> ‘p’. When they appear together in the code, it’s hard to guess which
>> is 0 and which is ‘p’. So, would it better to rename
>> TOAST_COMPRESS_PGLZ to TOAST_PGLZ_COMPRESS_ID, and rename
>> TOAST_PGLZ_COMPRESSION to TOAST_PGLZ_COMPRESS_METHOD?
>
> Here as well, I can get some of the confusion. We can just reuse the
> same names, with _ID instead.
>
>> As the switch/default explicitly rejects invalid cmethod, I feel
>> slightly better for readability to place "cmid =
>> MethodToCompressionId(cmethod);" after the switch clause.
>
> WFM.
>
> At the end I have the updated version attached, which still does the
> job I want it to do, just simpler.
>
> One extra thing to keep in mind is that we may want to make
> CompressionIdIsValid() smarter in the future, especially across
> multiple vartag_external or varlena types if the same ID values are
> shared across multiple compression methods, but would be simpler after
> this patch with all this knowledge kept local to toast_compression.c.
> Something similar could be said about toast_compress_datum() at some
> point, once/if we get there. Another argument would be to just switch
> ToastCompressionId to a uint32 and move the numbers to varatt.h, but
> I'd like to be more ambitious. This patch is just my take on the
> matter.
>
> What do you think?
> --
> Michael
> <v3-0001-Refactor-some-code-logic-around-GUC-default_toast.patch>

Hi Michael,

I rebased this onto current master (a23ab4862cf). It doesn't apply
cleanly anymore — amcheck picked up the Oid8 TOAST-value-ID rework
(toast_pointer_valueid / OID8_FORMAT) on top of the
VARATT_EXTERNAL_IS_COMPRESSED -> VARATT_EXTERNAL_OID_IS_COMPRESSED
rename. Both were easy to sort out, and the rebased patch is attached.

A couple of small things I noticed while reading through it.

1) varatt.h

When the IDs moved here, the comment that used to sit above the
ToastCompressionId enum got lost. It was the one warning that these raw
bits shouldn't be used to identify a method (that's what
TOAST_PGLZ_COMPRESSION etc. are for), and since both are just small
integers in C, nothing else stops a mixup. Maybe it'd be worth keeping
that note next to the defines (adjusted a bit, since the constants are
now above it):

* Don't use these values for anything other than understanding the
* meaning of the raw bits from a varlena; in particular, if the
* goal is to identify a compression method, use the constants
* TOAST_PGLZ_COMPRESSION, etc. instead.

2) CompressionNameToMethod()

I can see the hardcoded "lz4" next to the #ifndef makes the intent
obvious, so it might well be deliberate. But the loop already has the
matching registry entry in hand, and keeping the name in exactly one
place was kind of the point of the registry. So I wonder whether it'd
be cleaner to compare the method and reuse the entry's name — the error
message then comes from the registry too:

#ifndef USE_LZ4
if (toast_compression_registry[i].method == TOAST_LZ4_COMPRESSION)
NO_COMPRESSION_SUPPORT(toast_compression_registry[i].name);
#endif

3) %d vs %u

cmid is uint32 now, and elsewhere in the tree unsigned ids are
generally printed with %u — these four %d's (detoast.c twice,
varlena.c, verify_heapam.c) look like the odd ones out. Would it make
sense to switch them to %u? Purely cosmetic, though.

4) toast_internals.h

TOAST_COMPRESS_SET_SIZE_AND_COMPRESS_METHOD() uses
TOAST_*_COMPRESSION_ID, which now live in varatt.h, but the header
doesn't include it — it only works through transitive includes today
(the only file using the macro happens to get varatt.h via
heaptoast.h -> htup_details.h). Maybe add a direct #include "varatt.h"
so the header is self-contained?

I also wondered about going further and moving the compress/decompress
dispatch into the registry (function pointers + a cmid->name lookup).
Probably out of scope for this patch, though — the registry centralizes
the properties, and the dispatch part overlaps with the vartag
direction you mention. Just flagging it in case it's worth a follow-up
at some point.

Regards,
Aidar Imamov