Re: Refactor code around GUC default_toast_compression

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