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

Hi Greg, Manu, Nikhil,

On Thu, Oct 29, 2026 at 1:48 PM Manu <manuelreyesbravo(at)gmail(dot)com> wrote:
> On Tue, Oct 27, 2026 at 11:57 AM Greg Burd <greg(at)burd(dot)me> wrote:
> > Okay, v12 on master (065dc52f1a4).
...
> Containment also held everywhere I could take a value out of its
> column before a rewrite retired its dictionary: INSERT ... SELECT,
> CTAS, a whole-row composite, an array, another dictionary column, a
> matview, partition row movement, a WITH HOLD cursor, a procedure
> variable across COMMIT, logical decoding and an extended MCV list.

I did a full review and live testing pass over v12 (0001-0005) on master
with cassert enabled.

Overall, the split and the architecture read very cleanly -- especially
keeping varattrib_4b_long out of union varattrib_4b in 0002, and the
Ownership / Containment / Retirement write-up in toast_dictionary.c.
0001-0003 look quite close to committable on their own, while 0004-0005
have several containment, security/ACL, memory-management, and test bugs
that need another pass. (If helpful for PG20 progress, committing
0001-0003 first while iterating on 0004-0005 seems very natural.)

Here is what I found, grouped by patch.

=== 0001 - 0003 (Extensible encoding & stateless zstd) ===

1. 1-byte out-of-bounds read on corrupt 8-byte compressed varlenas (0002)

In src/include/varatt.h, VARDATA_COMPRESSED_GET_COMPRESS_METHOD(PTR)
unconditionally reads byte 8 (va_cmid, the 9th byte) whenever the top two
bits of va_tcinfo equal VARLENA_COMPRESS_METHOD_LONG (3):

if (method == VARLENA_COMPRESS_METHOD_LONG)
method = ((const varattrib_4b_long *) PTR)->va_cmid;

Both detoast.c:toast_decompress_datum() and the Assert() at the top of
zstd_decompress_datum() (and VARATT_IS_COMPRESSED_DICT() in 0004) call
this macro *before* zstd_decompress_datum() checks:

if (VARSIZE(value) < VARHDRSZ_COMPRESSED_LONG)

So a corrupted 8-byte 4B_C varlena with method bits == 3 at the end of a
palloc chunk or page reads 1 byte past the allocation. Also, if a
corrupt long header has va_cmid in {0, 1}, it dispatches to pglz/lz4
decompression, which reads starting at offset 8 instead of 9 (feeding
va_cmid itself into the bitstream). Long-form va_cmid values that do
not belong in a long header ({0, 1, 3}) should be rejected as corrupt,
and contrib/amcheck/verify_heapam.c currently returns early on
!VARATT_IS_EXTERNAL(attr) without validating inline compressed headers.

2. One-shot ZSTD_compress() allocates/frees a ~1.3 MB CCtx per datum (0003)

zstd_decompress_datum() nicely reuses a backend-static ZSTD_DCtx, and
zstd_dict_compress_datum() in 0004 reuses a backend-static ZSTD_CCtx
(dict_cctx), but plain zstd_compress_datum() in 0003 still calls one-shot
ZSTD_compress(), which mallocs, initializes, and frees a ~1.2-1.5 MB
ZSTD_CCtx per datum. Reusing a backend-static ZSTD_CCtx via
ZSTD_compressCCtx() avoids per-datum context churn / mmap syscalls on
bulk loads.

3. Minor items in 0001-0003:
- zstd_decompress_datum_slice(): the streaming while (outbuf.pos <
outbuf.size) loop should call CHECK_FOR_INTERRUPTS().
- toast_external_info_get() (0001): still leaves compress_method = 0
(pglz) when the external datum is uncompressed; setting it to
TOAST_INVALID_COMPRESSION_ID when !VARATT_EXTINFO_IS_COMPRESSED()
would make toast_external_data self-describing.
- toast_fetch_datum_slice() (0002): adds VARHDRSZ_COMPRESSED_LONG -
VARHDRSZ (5 bytes) for VARTAG_IS_ONDISK_LONG, which is 4 bytes too
short for 0004's VARHDRSZ_COMPRESSED_DICT - VARHDRSZ (9 bytes). It is
masked today only because slice pushdown is not enabled for zstd_dict /
lz4_dict, but will bite as soon as max_size < extsize is used.

=== 0004 (Compression dictionaries & containment) ===

4. Three containment escapes (including pure PL/pgSQL reproducer)

(a) R/W ExpandedRecord fast path into non-atomic DO / PROCEDURE:
When an atomic PL/pgSQL function returns a named-composite ExpandedRecord,
expanded_record_set_fields(..., !estate->atomic) passes expand_external =
false, keeping the inline zstd_dict datum compressed inside erh->dvalues.
When a non-atomic DO block or PROCEDURE assigns that result to a matching
composite variable, exec_move_row_from_datum() (pl_exec.c:7726) takes the
R/W transfer fast path:

if (VARATT_IS_EXTERNAL_EXPANDED_RW(value) && ...)
{
assign_record_var(estate, rec, erh);
return;
}

completely bypassing field detoasting even though !estate->atomic is true!
(Note: this fast path appears to have the same bug upstream for ordinary
on-disk external TOAST pointers across COMMIT + DROP TABLE.)

Minimal pure-SQL/PLpgSQL reproducer:

CREATE TYPE er_pair AS (a int, b text);
CREATE TABLE cmdict_er(id int, f1 text COMPRESSION zstd_dict,
pad text STORAGE PLAIN);
INSERT INTO cmdict_er
SELECT g,
'{"org":"acme-corporation","dept":"storage","seq":' || g || '}',
repeat('x', 1960)
FROM generate_series(1, 200) g;
SELECT pg_train_toast_dictionary('cmdict_er', 'f1');
INSERT INTO cmdict_er VALUES (
999,
'{"org":"acme-corporation","dept":"storage","seq":999}',
repeat('y', 1960));

CREATE FUNCTION make_er_pair(p_id int) RETURNS er_pair
LANGUAGE plpgsql AS $$
DECLARE r er_pair;
BEGIN
SELECT 1, f1 INTO r FROM cmdict_er WHERE id = p_id;
RETURN r;
END;
$$;

DO $$
DECLARE r er_pair;
BEGIN
r := make_er_pair(999);
RAISE NOTICE 'before: %', pg_column_compression(r.b); -- zstd_dict
COMMIT;
DROP TABLE cmdict_er CASCADE;
COMMIT;
RAISE NOTICE 'len: %', length(r.b);
END;
$$;

Output:
NOTICE: before: zstd_dict
ERROR: compression dictionary 16592 does not exist

(b) Indirect TOAST pointers in detoast_external_attr() (detoast.c):
In the VARATT_IS_EXTERNAL_INDIRECT(attr) branch, the recursion check still
says:

if (VARATT_IS_EXTERNAL(attr))
return detoast_external_attr(attr);

without `|| VARATT_IS_COMPRESSED_DICT(attr)`, so an indirect pointer
referencing an inline dict datum falls through to memcpy() and returns a
still-dictionary-compressed 4B_C datum.

(c) SPI_datumTransfer() / datumTransfer():
SPI_datumTransfer() only flattens VARATT_IS_EXTERNAL_EXPANDED, copying
inline VARATT_IS_COMPRESSED_DICT datums verbatim via datumCopy(). Non-
PL/pgSQL PLs (PL/Python, PL/Perl, PL/Tcl, extensions) or C procedures
using SPI_datumTransfer across SPI_commit() can hold a dangling dictid
after a concurrent rewrite or DROP.

5. Lock-before-ACL, cross-session temp tables, and per-call leak in
pg_train_toast_dictionary()

- Lock before ownership check: pg_train_toast_dictionary() is executable
by PUBLIC and calls `table_open(relid, ShareUpdateExclusiveLock)`
*before* dict_check_column() checks object_ownercheck(). Any
unprivileged role can therefore acquire ShareUpdateExclusiveLock on any
table (blocking VACUUM, ANALYZE, ALTER TABLE, CREATE INDEX CONCURRENTLY,
REPACK) before failing the ACL check. Ownership should be checked
before taking ShareUpdateExclusiveLock (similar to VACUUM/ANALYZE).
- Missing RELATION_IS_OTHER_TEMP(rel) check: dict_check_column() checks
RELKIND_RELATION (which includes temp tables) and object_ownercheck(),
but never rejects RELATION_IS_OTHER_TEMP(rel). A second session of the
same role locks another backend's temp table and enters table_beginscan
until localbuf errors out ("cannot access temporary tables of other
sessions").
- Memory leak in caller context: pg_train_toast_dictionary() allocates
samples.data (up to 100 * dict_size, i.e., ~100 MB at 1 MB dict_size),
sizes, and dictbuf in CurrentMemoryContext (ExecutorState) and never
pfrees them before returning, so calling it across multiple columns/
partitions in one SELECT accumulates hundreds of MBs to GBs in
ExecutorState.

6. TRUNCATE does not retire non-current dictionaries

Retirement is wired only into finish_heap_swap() (repack.c). TRUNCATE
(ExecuteTruncateGuts in tablecmds.c) acquires AccessExclusiveLock and
creates a new empty relfilenode, destroying all rows referencing old
dictionaries, yet never calls toast_dictionary_retire(). If a column hits
dictionary_evolution_max_retained, TRUNCATE + reload leaves all dead
dictionaries wedged until the user runs VACUUM FULL on the truncated table.

7. Inline HEAP_HASEXTERNAL overhead & unrelated-column UPDATE behavior

Setting HEAP_HASEXTERNAL on inline dict tuples preserves HOT updates, but
in heap_update() (heapam.c:3888) `need_toast` is true for every update of
any unrelated column on that row -- forcing an extra XLOG_HEAP_LOCK WAL
record, heap buffer unlock/relock, dual tuple deform, and TOASTDICTOID
syscache lookup on every single UPDATE (and tuple deform in
heap_toast_delete() on every DELETE).

Also note the behavioral difference vs. compression_zstd_dict.sql's
comment ("ordinary updates of other columns leave the value, and its old
dictionary, alone"): that holds for external TOAST pointers, whereas for
*inline* dict datums toast_tuple_init() runs !toast_dictionary_keep() on
every UPDATE and eagerly decompresses + recompresses the column as soon
as its dictionary becomes non-current or SET COMPRESSION changes.

8. Cache sizing & invalidation in toast_dictionary.c

- dict_cache_invalidate() / lz4_dict_cache_invalidate() ignore `hashvalue`
and wipe all 4 slots across the backend whenever *any* row in
pg_toast_dictionary changes anywhere in the database.
- With DICT_CACHE_SIZE = 4 per backend, scanning a partitioned table with
>4 partitions (each partition has its own relid and dictid OID!) will
thrash ZSTD_createDDict() on every tuple.

=== 0005 (Automatic dictionary evolution during ANALYZE) ===

9. compression_dict_evolve.sql compresses ZERO rows on disk!

In compression_dict_evolve.sql, `evo (id int, f1 text COMPRESSION
zstd_dict)` stores docA(g) (~175 bytes) and docB(id) (~195 bytes). Total
tuple width is ~210 bytes -- roughly 10x below TOAST_TUPLE_THRESHOLD -- so
the toaster is never invoked on any row in the entire test:

SELECT pg_column_compression(f1), count(*) FROM evo GROUP BY 1;
pg_column_compression | count
-----------------------+-------
| 4000

So the test currently exercises catalog row churn in pg_toast_dictionary,
not actual multi-generation dictionary compression/decompression on disk
(and `UPDATE evo SET f1 = docB(id)` overwrote all rows anyway, so no
dictA-compressed tuples would coexist with dictB tuples even with wider
rows).

This also highlights a policy gap in toast_dictionary_maybe_evolve():
- On narrow tables whose tuples never approach TOAST_TUPLE_THRESHOLD,
ANALYZE still trains and installs 64 kB dictionaries that no row will
ever use.
- When !OidIsValid(curid), `adopt = true` is unconditional without
checking on the 20% held-out set whether dictionary compression actually
beats plain zstd/lz4 (e.g., on UUID/high-entropy text).

10. Permanent C malloc() leaks across PG_CATCH() OOM rollback (0004 & 0005)

In zstd_dict_compressed_total():

cd = ZSTD_createCDict(dict, dictlen, ZSTD_CLEVEL_DEFAULT); /* malloc */
if (cd == NULL)
return 0;
out = palloc(ZSTD_compressBound(maxlen)); /* can ereport(ERROR)! */

Because toast_dictionary_maybe_evolve() catches non-cancel errors in
PG_CATCH(), rolls back the subtransaction, and downgrades to WARNING, an
OOM in palloc() jumps over ZSTD_freeCDict(cd) and permanently leaks the
malloc'd CDict in long-lived backends / autovacuum workers. The same
pattern occurs with LZ4_createStream() before palloc() in
lz4_dict_compress_datum() (0004) and lz4_dict_compressed_total() (0005)
(where a stack or static LZ4_stream_t + LZ4_resetStream_fast() would also
avoid calling malloc/free inside the per-value loop).

11. tdtrainrows = -1 breaks the growth gate & pg_upgrade loses metadata

- In pg_train_toast_dictionary():
dict_install(..., (int64) rel->rd_rel->reltuples, ...);
On a freshly loaded table before its first VACUUM/ANALYZE, reltuples is
-1, storing tdtrainrows = -1. In toast_dictionary_maybe_evolve(),
`(double) currows * growth` is -2.0, so `totalrows < -2.0` is always
false; and whenever the candidate does not beat the manual dictionary by
min_gain, tdtrainrows (-1) and tdtraintime are left untouched -- causing
every subsequent post-cooldown ANALYZE to re-run ZDICT_trainFromBuffer()
forever.
- 0005 does not update pg_dump's binary_upgrade_dump_toast_dictionaries()
or binary_upgrade_add_toast_dictionary(), resetting tdtrainrows = 0 and
tdtraintime = now() across pg_upgrade.

12. Single-slot rd_toastdictattnum relcache cache thrashes 100% on 2+ columns

RelationData caches a single (rd_toastdictattnum, rd_toastdictoid) pair.
The comment notes "A single entry suffices for a scan of one column", but
toast_dictionary_current() is called tuple-by-tuple on the *write* path:
inserting into a table with two zstd_dict columns alternates attnum 2, 3,
2, 3... and misses the cache on 100% of calls, doing 2*N catalog index
scans. A lazily allocated Oid[natts] array in CacheMemoryContext would
avoid that completely.

13. PGC_USERSET evolution GUCs vs. MAINTAIN privilege

dictionary_evolution_min_interval, dictionary_evolution_min_gain, and
dictionary_evolution_max_retained are all PGC_USERSET. Since PG17, a role
with MAINTAIN (which grants neither DDL ownership nor SELECT on the table)
can run ANALYZE. With PGC_USERSET, a MAINTAIN role can set min_interval=0,
min_gain=0, max_retained=INT_MAX and loop ANALYZE to churn dictionaries
and bloat pg_toast_dictionary (or set client_min_messages=debug1 to read
the held-out compressed byte counts logged in DEBUG1). Making these
PGC_SUSET (or per-column attoptions owned by the table owner) would keep
the privilege boundary intact. Separately, since pg_toast_dictionary has
REVOKE ALL FROM PUBLIC, a security-barrier view (like pg_stats) exposing
metadata without tddict bytes would let non-superuser table owners inspect
their dictionaries.

Hope this helps!

Cheers,
Hannu

On Thu, Oct 29, 2026 at 2:48 PM Manu <manuelreyesbravo(at)gmail(dot)com> wrote:
>
> Hi Greg,
>
> On Tue, Oct 27, 2026 at 11:57 AM Greg Burd <greg(at)burd(dot)me> wrote:
> > Okay, v12 on master (065dc52f1a4).
>
> v12 fixes all five on my side. It also applies cleanly on today's
> master (76a55702d89), and make check (243) and amcheck pass with no
> warnings with both libraries (-O2, cassert+ASan), lz4 only and zstd
> only. A SIGINT on entry to the evaluation gives the usual ERROR, the
> damaged chunk one WARNING and no "resource was not closed", and the
> numbers match yours: 27,881 and 27,894 instructions per value with 2
> fetches, and 7 CDicts per evaluation, 5 of them inside ZDICT.
>
> The subtransaction also holds in VACUUM (PARALLEL 2, ANALYZE), BEGIN
> READ ONLY, a DO block with EXCEPTION and autovacuum; an autovacuum
> ANALYZE cancelled by LOCK TABLE inside the evaluation stops without
> adopting, and a hot standby refuses ANALYZE and replays the adoption.
>
> Containment also held everywhere I could take a value out of its
> column before a rewrite retired its dictionary: INSERT ... SELECT,
> CTAS, a whole-row composite, an array, another dictionary column, a
> matview, partition row movement, a WITH HOLD cursor, a procedure
> variable across COMMIT, logical decoding and an extended MCV list.
> So did REPACK (CONCURRENTLY) with DML committed during the copy, and
> pg_upgrade, which keeps the dictionary OIDs.
>
> Two leftovers: the 0005 message still says compressed_size() and "any
> failure is logged and swallowed, never aborting ANALYZE", and
> toast_dictionary.h:39 still names compressed_size(); my diff missed it.
>
> Thanks for taking all of these in.
>
> Regards,
> Manu
>
>