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

On Mon, Nov 2, 2026 at 23:45 UTC, Hannu Krosing <hannuk(at)google(dot)com> wrote:
> Thanks! I ran another full review and test pass on v14 (zstd+lz4 and
> --without-zstd, with cassert, 002_pg_upgrade, isolation/repack-concurrently,
> and logical decoding).

Thanks, Hannu, and thanks, Manu, for the containment audit and the
UPDATE numbers. v15 is attached; it applies on master as of
72b9498df9a.

> 1. Missed VARATT_IS_COMPRESSED_DICT() in check_domain_for_new_field()
> (expandedrecord.c, 0004)

Fixed as you suggested. compression_zstd_dict has a case for it now:
a domain over the composite whose CHECK stores VALUE, and a DO block
that assigns a dictionary value to a field. Without the fix the stored
copy is still zstd_dict.

> 2. Missed toast_inline_compression_id() caller in toast_save_datum()
> (toast_internals.c, 0002 & 0004)

Fixed: toast_save_datum() uses toast_inline_compression_id() and
reports "compressed data is corrupt", and the Assert is gone. I added
the UPDATE path to my corrupt-header test, widening another column so
that a bad value gets moved out of line. On v14 the long headers with
va_cmid 0 and 1, and the plain header claiming zstd, were moved out of
line as they were, and va_cmid 99 tripped the Assert. On v15 those and
va_cmid 3 fail with the error. The two truncated headers (6 and 8
bytes) are smaller than a TOAST pointer, so they are never moved out of
line and the UPDATE keeps them as master does; ASan reports nothing
there.

> (Two tiny related validation polish items if you want them:

toast_inline_compression_id() rejects method IDs above the last one in
use (TOAST_LAST_COMPRESSION_ID, which each patch that adds a method
moves up). toast_external_info_get() rejects a long pointer whose
method byte names pglz or lz4, and toast_get_compression_id() reports a
compressed pointer that decodes to no method as corrupt. I did not add
the VARHDRSZ_COMPRESSED_DICT check to the validator.
pg_column_compression() on a 10-byte dictionary header still says
zstd_dict, but reading the value fails in dict_datum_get_id(), and
toast_compression.h can't see that macro without a new include.

> 3. Temp tables with ON COMMIT DELETE ROWS miss toast_dictionary_retire()
> (heap.c, 0004)

Fixed in heap_truncate(), as you suggested, under an active snapshot.
compression_zstd_dict tests it with your sequence.

> 4. Moving rd_toastdictoids / rd_toastdictvalid from 0005 into 0004

Done. 0004 now has the relcache cache, its cleanup in relcache.c, and
the invalidation in dict_install(), so 0001-0004 stand on their own.
0005 only adds the evolution policy.

> 5. Two minor gate-ordering items in toast_dictionary_maybe_evolve() (0005)

Both done. The gates read only the dictionary's row. The dictionary
count is taken just before the cap gate, and the bytes are fetched only
for the held-out test. The "row count unknown" baseline is now
recorded before the min_rows check. compression_dict_evolve tests it
with min_rows above the table's size.

best.

-greg