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

Hi Michael, Nikhil,
> I was just looking at v7-0001 that wants to add the ToastCompressionId
> to toast_external_data, and this feels half-baked due to the
> inconsistency this brings with extsize and VARATT_EXTINFO_GET_EXTSIZE.
>
> Couldn't we do better here by normalizing more data from the existing
> fields? Another could be the is_compressed state which is guessed
> from a comparison between the raw size and the compressed size,
> perhaps?
Looking closely at v7-0001 with this in mind, I completely agree. Once we
introducing toast_external_data to decouple callers from the low-level on-disk
representation, keeping extsize and is_compressed as raw macro calculations
for callers to repeat leaves the abstraction half-baked.
In fact, there is an even stronger argument for normalizing is_compressed
here: currently, when an external datum is uncompressed, the high 2 bits of
va_extinfo are zero (since payload size < 1GB). If a caller inspects
toast_ext_data.compress_method without manually checking
VARATT_EXTINFO_IS_COMPRESSED() first, they end up seeing 0
(TOAST_PGLZ_COMPRESSION_ID) instead of an invalid ID.
If we normalize both extsize and is_compressed inside
toast_external_info_get(),
we can explicitly enforce:
```
typedef struct toast_external_data
{
vartag_external tag;
int32 rawsize;
int32 extsize; /* unpacked external payload size */
bool is_compressed; /* true if extsize < rawsize - VARHDRSZ */
ToastCompressionId compress_method; /* valid only if is_compressed */
Oid8 valueid;
Oid toastrelid;
uint32 extinfo; /* preserved raw bits */
} toast_external_data;
And in toast_external_info_get():

toast_ext_data->extsize = toast_ext_data->extinfo & VARLENA_EXTSIZE_MASK;
toast_ext_data->is_compressed =
(toast_ext_data->extsize < toast_ext_data->rawsize - VARHDRSZ);
if (toast_ext_data->is_compressed)
{
/* resolve compress_method, handling long form if tag demands */
...
}
else
toast_ext_data->compress_method = TOAST_INVALID_COMPRESSION_ID;
```

I checked the codebase: doing this cleans up repetitive calls to
VARATT_EXTINFO_GET_EXTSIZE() and VARATT_EXTINFO_IS_COMPRESSED() across
at least 11 sites in detoast.c, toast_internals.c, amcheck, and
reorderbuffer.c. It makes the whole subsystem much cleaner.

Regarding v7-0002 and v7-0003, I am doubting the wisdom of tackling
the last-compression-bit issue for this release. The OID8 code has
already changed a lot of code, and maybe we should be conservative in
terms of the amount of the changes we do in this area for a single
release. By that, I mean to catch up on the refactoring pieces on this
thread once some dust has settled on HEAD and tackle this issue around
the time v21 opens up

From a release management standpoint, taking a conservative stance
here makes total sense. The OID8 expansion was a major physical
storage milestone, and avoided simultaneous churn on on-disk TOAST
pointer formats right after it minimized risk to HEAD.

At the same time, I want to thank Nikhil for the great work on v7:
having re-reviewed the patch series against latest HEAD (commit
45277ca0d1c) and verified it locally, all edge cases previously
discussed (the slice fetch offset calculation, amcheck assertion
guards on corrupt pointers, unsigned underflow checks, and header size
verification) are cleanly resolved, and the entire test suite passes
without issues.

Given that, I think splitting the timeline as Michael suggested is the
best path forward:

Enhance 0001 into a standalone, fully-normalized toast_external_data
refactoring and commit it in the current cycle. It carries zero
on-disk format changes and strictly improves the codebase.
Hold 0002 and 0003 until HEAD settles and v21 opens up. Since
0002/0003 are already technically solid, having a fully-normalized
toast_external_data in place now will make landing the long-form
format and zstd in v21 remarkably straightforward.
Nikhil, what do you think about updating 0001 along these lines for a v8?

Best regards