On Mon, Sep 21, 2026 at 07:26:20PM -0700, Nikhil Kumar Veldanda wrote: > 0001: Refactor TOAST compression header handling > ------------------------------------------------ > > Preparatory, with no behavior change, so that 0002 only has to touch > one place for each thing it changes:
> - toast_internals.h duplicated the layout of the compressed-in-line
> header as toast_compress_header, with TOAST_COMPRESS_* macros to read
> and write the tcinfo word, while varatt.h describes the same bytes as
> varattrib_4b.va_compressed. The duplicate is removed and the header
> is written through one inline function,
> toast_compress_set_size_and_method().
+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.
+++ b/src/include/access/detoast.h
[...]
+#include "access/toast_compression.h"
[...]
+ ToastCompressionId compress_method; /* compression method ID */
So you need this new header dependency due to ToastCompressionId.
Okay.
> - The compression method of an external value was fetched by callers
> from the raw bits of va_extinfo. It is now decoded once, in
> toast_external_info_get(), into a new compress_method field of
> toast_external_data, and detoast_attr_slice(),
> toast_get_compression_id() and amcheck read it from there.
>
> - VARTAG_IS_ONDISK() is added next to VARTAG_IS_EXPANDED(), and the
> TOAST pointer assembly at the end of toast_save_datum() moves into a
> helper, toast_pointer_build().
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.
> 0002: Allow more than four TOAST compression methods
> ----------------------------------------------------
Seems like HEAD disagrees with this one in varatt.h and amcheck.
> So a long-form pointer is 19 or 23 bytes instead of 18 or 22, and the
> inline header is 9 bytes instead of 8, for methods other than pglz and
> lz4 only. This leaves room for method IDs up to 255.
With 8-byte alignment in mind, this does not bother me in heap, be it
for oid-based varlenas or oid8-based varlenas.
> 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.
> It is deliberately not a member of the varattrib_4b union. A member
> with the extra byte pads to 12 bytes and would grow
> sizeof(varattrib_4b) from 8, and a lot of code inspects varlena
> headers of unknown or smaller size through pointers of that type;
> Outside the union the struct's trailing padding is harmless, since
> like the rest of varatt.h it is only ever used with offsetof() and
> field access, never sizeof(). Two static assertions tie its va_tcinfo
> and va_cmid offsets to va_compressed, so the "va_compressed plus one
> byte" relationship is checked by the compiler rather than by a
> comment.
I guess that's fine by me, on top of my mind. I am wondering if
others have different opinions and/or ideas about these structures?
> 0003: Add zstd as a TOAST compression method
> --------------------------------------------
The backend-side core change goes at 200 lines or so with the two
first refactorings in place, where most of it consists of the full
compression and decompression and slice handling.
> Not in this series: a compression level setting, and dictionary
> support, which was the motivation for making the ID space large and
> would be another method ID using the same long form.
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.
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.
--
Michael
signature.asc
Description: PGP signature
