Re: ZSTD TOAST compression, and an extensible compression method encoding - Mailing list pgsql-hackers

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