Hi Greg,
On Fri, Oct 30, 2026 at 6:14 PM Greg Burd <[email protected]> wrote:
> v13 is attached.
>
> It is v12 plus a sixth patch that collects the fixes, kept separate so
> the review is easy to follow; I will fold it into 0001-0005 before a
> commit.
Thanks for the fast turnaround and for keeping 0006 separate -- that made
the diff very easy to audit.
I ran a second review and live test pass on v13 (0001-0006) under both
zstd+lz4 and lz4-only (--without-zstd) with cassert enabled.
Items #2 (CCtx reuse), #3 (CHECK_FOR_INTERRUPTS + external uncompressed
method ID), #4b/c (indirect TOAST pointer + datumTransfer containment),
#5 (ownercheck before lock, RELATION_IS_OTHER_TEMP, workcxt, plus marking
pg_train_toast_dictionary proparallel => 'u'), #8 (hashvalue-targeted
DictCache invalidation and the --without-zstd build), #9 (rewritten
compression_dict_evolve.sql with compressed multi-generation rows), #10
(palloc ordering / static LZ4 stream), and #13 (PGC_SUSET) all look good.
Relying on Muzzammil's core fix for #4a also makes complete sense.
However, 0006 introduced two regressions (#1 and #2 below, one of which
breaks pg_upgrade whenever any dictionary exists), and a few of the v12
fixes are still incomplete (#3-#5). I also looked into your question on
#7 and found a clean way to eliminate most of the unrelated-column UPDATE
overhead without losing containment.
=== Regressions & incomplete fixes in v13 ===
1. [Blocker] pg_upgrade fails whenever pg_toast_dictionary is non-empty
pg_toast_dictionary.tdtraintime is declared as int8 in pg_toast_dictionary.h
(storing raw TimestampTz microseconds since 2000-01-01, e.g.
846716483046138), and binary_upgrade_dump_toast_dictionaries() dumps
d.tdtraintime directly from that int8 column.
However, 0006 declared binary_upgrade_add_toast_dictionary()'s 6th
parameter as timestamptz in pg_proc.dat (using PG_GETARG_TIMESTAMPTZ(5))
and emits:
'%s'::pg_catalog.timestamptz
in pg_dump.c:5882. When pg_restore runs during pg_upgrade, PostgreSQL
tries to parse the raw microsecond integer string as a timestamptz literal
and aborts immediately:
ERROR: date/time field value out of range: "846716483046138"
002_pg_upgrade.pl missed this only because compression_zstd_dict.sql,
compression_lz4_dict.sql, and compression_dict_evolve.sql all DROP their
schemas at the end, leaving zero rows in pg_toast_dictionary.
Making the 6th argument of binary_upgrade_add_toast_dictionary() int8
(PG_GETARG_INT64(5)) and emitting %s::pg_catalog.int8 in pg_dump.c -- just
like tdtrainrows -- fixes it cleanly. (Also, if live callers of
dict_install() pass GetCurrentTimestamp() explicitly instead of 0,
traintime == 0 won't collide with 2000-01-01 00:00:00+00.) Please also
leave at least one table with active + retired dictionaries behind in the
regression database so 002_pg_upgrade.pl exercises this path.
2. CacheMemoryContext leak and unreset pg_internal.init pointers on
rd_toastdictoids / rd_toastdictvalid
In toast_dictionary_current() (toast_dictionary.c:245-254),
rel->rd_toastdictoids and rel->rd_toastdictvalid are allocated directly in
CacheMemoryContext:
oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
rel->rd_toastdictoids = palloc0_array(Oid, natts);
...
rel->rd_toastdictvalid = bms_add_member(rel->rd_toastdictvalid, attnum);
Unlike rd_indexcxt / rd_rulescxt, RelationData has no per-relation memory
context: CacheMemoryContext lives for the entire backend lifetime, and
RelationDestroyRelation() in relcache.c explicitly pfrees every heap-
allocated field of RelationData. Because neither RelationDestroyRelation()
nor RelationClearRelation() frees rd_toastdictoids / rd_toastdictvalid,
every relcache rebuild leaks both allocations in CacheMemoryContext for
the life of the backend.
In addition, write_relcache_init_file() writes the raw RelationData bytes
to pg_internal.init, and load_relcache_init_file() resets all pointer
fields on reload; if a cached relation ever had those pointers populated,
reloading pg_internal.init in another process would leave dangling
pointers unless load_relcache_init_file() resets rd_toastdictoids = NULL
and rd_toastdictvalid = NULL (and bumps RELCACHE_INIT_FILEMAGIC).
(Nit: the comment above toast_dictionary_current() at lines 212-218 still
describes the v12 single-entry cache.)
3. Remaining OOB reads & non-canonical header bypass on corrupt 4B_C
datums (0002 / 0004 / 0006)
0006 fixed toast_decompress_datum() to check VARSIZE >=
VARHDRSZ_COMPRESSED_LONG (9) before reading va_cmid, and to reject
non-canonical long-form cmids ({0, 1, 3}). Three gaps remain:
(a) toast_get_compression_id() (toast_compression.c:534) and
VARATT_IS_COMPRESSED_DICT() (toast_compression.h:91) still call
VARDATA_COMPRESSED_GET_COMPRESS_METHOD(attr) with no VARSIZE >= 9
check. On a corrupt 8-byte 4B_C datum with raw_method == 3,
SELECT pg_column_compression(col) reads 1 byte past the datum, and
varlena.c:4252 even prints the exact decimal value of that OOB byte
in the error message ("ERROR: invalid compression method id 119")
for 6..255 while mapping 0..5 to codec names/NULL.
(b) None of toast_decompress_datum(), toast_decompress_datum_slice(),
toast_raw_datum_size(), toast_get_compression_id(), or
VARATT_IS_COMPRESSED_DICT() checks VARSIZE(attr) >=
VARHDRSZ_COMPRESSED (8) before reading va_tcinfo (bytes 4..7). A
corrupt 4B_C varlena with length 4..7 causes SELECT octet_length(col)
to read 4 bytes past the 4-byte header and return 30 bits of OOB
memory as the length (and pg_column_compression() reads up to 5 bytes
past the datum).
(c) toast_decompress_datum_slice() (detoast.c:602-627) got the VARSIZE >= 9
check, but missed the `!toast_compression_id_needs_cmid_byte(cmid)`
check that was added to toast_decompress_datum() (detoast.c:548-553).
So `substr(col, 1, 5)` on a corrupt long header with va_cmid in {0, 1}
still dispatches to pglz/lz4 slice decompression starting at offset 8
(&va_cmid) instead of rejecting it as corrupt.
Centralizing 4B_C header validation in a single inline helper (checking
VARSIZE >= 8, and if raw_method == 3 then VARSIZE >= 9 and
toast_compression_id_needs_cmid_byte(cmid)) shared by
toast_decompress_datum(), toast_decompress_datum_slice(), and
toast_get_compression_id() -- plus guarding toast_raw_datum_size() with
VARSIZE >= 8 and VARATT_IS_COMPRESSED_DICT() with VARSIZE >=
VARHDRSZ_COMPRESSED_DICT (13) -- would close all of these in one place.
Once that and #2/#1 are folded in, 0001-0003 look completely ready to me.
4. Same-subtransaction TRUNCATE misses toast_dictionary_retire()
In ExecuteTruncateGuts() (tablecmds.c:2231-2291), toast_dictionary_retire()
was placed inside the `else` branch (RelationSetNewRelfilenumber), so
truncating a table created or previously truncated in the same
(sub)transaction takes the `if (rel->rd_createSubid == mySubid ||
rel->rd_newRelfilelocatorSubid == mySubid)` in-place heap_truncate_one_rel()
branch and skips retirement:
BEGIN;
-- table created/rewritten/truncated in this xact, evolved, then:
TRUNCATE t;
COMMIT;
Moving toast_dictionary_retire(RelationGetRelid(rel)) right after that
if/else block handles both paths.
5. tdtrainrows = 0 clamping and un-updated cooldown when adopt == false
Two small state-machine bugs remain in 0005/0006:
- Clamping reltuples == -1 to 0 in pg_train_toast_dictionary() means
`0 * growth == 0`, so after `CREATE TABLE -> COPY ->
pg_train_toast_dictionary('t', 'c', 4096) -> ANALYZE`, the first ANALYZE
after min_interval sees totalrows >= 0 even with zero table growth and
can replace a user-sized 4 kB dictionary with a 64 kB auto-trained one.
Since pg_train_toast_dictionary() already scans the heap (or samples
blocks), passing the scanned/extrapolated row count when reltuples < 0
gives a realistic baseline.
- Worse, in toast_dictionary_maybe_evolve() (toast_dictionary.c:1503),
when both the growth gate and min_interval (86400s) have passed, if the
held-out check rejects the candidate (`adopt == false`, which is the
normal case when data grew 2x without changing distribution), neither
tdtraintime nor tdtrainrows is updated on the current dictionary row!
As a result, *every* subsequent ANALYZE / autoanalyze forever re-samples
30,000 rows, detoasts them, and runs ZDICT_trainFromBuffer() over and
over. Updating tdtraintime = GetCurrentTimestamp() (and tdtrainrows =
totalrows) on the tdcurrent row when !adopt resets the gates cleanly.
=== On #7: Narrowing inline HEAP_HASEXTERNAL UPDATE / DELETE overhead ===
> This one I have not changed, and I want to flag it rather than paper
> over it. The extra per-UPDATE work on a row that holds an inline
> dictionary datum is real, and it is the cost of making those datums
> self-contained the same way a TOAST pointer is; I don't see a way to
> keep containment without HEAP_HASEXTERNAL set. I did fix the misleading
> comment in the test. If you see a way to narrow the flag to rows that
> actually carry a dictionary datum without losing containment, I'm
> interested.
First, on the flag scope itself: fill_val() (heaptuple.c:378) already sets
HEAP_HASEXTERNAL only when VARATT_IS_EXTERNAL(val) ||
VARATT_IS_COMPRESSED_DICT(val), so rows whose column stayed uncompressed
or fell back to plain zstd/lz4 do not get HEAP_HASEXTERNAL. And I agree
that keeping HEAP_HASEXTERNAL on rows that *do* hold an inline dict datum
is necessary for containment without burning a scarce t_infomask2 bit.
However, we *can* eliminate two expensive pieces of that overhead without
changing on-disk flags or losing containment / retirement:
(1) Skip !toast_dictionary_keep() (and the TOASTDICTOID SysCache lookup +
eager ZSTD decompress/recompress) on unchanged columns during an
in-place UPDATE of the same relation.
In toast_tuple_init() (toast_helper.c:162), `ttc->ttc_oldvalues != NULL`
is true *only* during an in-place table_tuple_update() on `ttc->ttc_rel`
(INSERT, VACUUM FULL, CLUSTER, rewriting ALTER TABLE, REFRESH MATVIEW,
and cross-partition UPDATE row movement all pass oldtup == NULL; the
only rewrite path that replays concurrent updates through
table_tuple_update() is REPACK CONCURRENTLY in repack.c, where
OidIsValid(ttc->ttc_rel->rd_rel->relrewrite) is true on NewHeap).
So in toast_tuple_init(), if:
- ttc->ttc_oldvalues != NULL,
- !OidIsValid(ttc->ttc_rel->rd_rel->relrewrite),
- att->attstorage != TYPSTORAGE_PLAIN,
- !ttc->ttc_oldisnull[i], and
- old_value and new_value are byte-identical VARATT_IS_COMPRESSED_DICT
datums (same pointer or same VARSIZE + memcmp == 0),
then `new_value` came directly from `oldtup` of this exact (relid,
attnum) and is staying in this exact heap! Skipping the
!toast_dictionary_keep() check in that case:
- avoids the TOASTDICTOID SysCache lookup on every unrelated-column
UPDATE,
- avoids eagerly decompressing + recompressing inline dict columns on
unrelated-column UPDATEs after a dictionary evolves (while full
table rewrites still recompress everything via oldtup == NULL or
relrewrite != InvalidOid, so retirement remains 100% intact),
- still leaves `new_value` eligible for out-of-line toasting if
another updated column pushed the tuple past TOAST_TUPLE_THRESHOLD,
- and actually makes the comment in compression_zstd_dict.sql:135
("ordinary updates of other columns leave the value, and its old
dictionary, alone" -- which btw wasn't touched in 0006) true for
inline dictionary rows too!
(2) Guard heap_toast_delete() on DELETE when the table has no TOAST table:
Inline dictionary datums have no external chunks in pg_toast_<oid> to
delete. In heapam.c:3185 (and speculative abort at 6526), checking
`OidIsValid(relation->rd_rel->reltoastrelid)` before calling
heap_toast_delete() avoids deforming the tuple on DELETE for tables
without a TOAST relation (e.g., STORAGE MAIN columns on an otherwise
fixed-width table).
=== Minor nits & docs ===
- toast_dictionary.c:1158-1166: when !object_ownercheck() fails in
pg_train_toast_dictionary(), get_rel_relkind(relid) and
get_rel_name(relid) are called unlocked; if the relation is concurrently
dropped between them, get_rel_name(relid) returns NULL into
aclcheck_error(). Looking up RELOID once in SysCache (or checking
relname == NULL) avoids that.
- toast_dictionary.c:268: "compressed zstd data is corrupt" in
dict_datum_get_id() is also reached for lz4_dict ("compressed dictionary
data is corrupt" would fit both). Similarly, under --without-zstd
--with-lz4, pg_train_toast_dictionary() on an lz4_dict column fails with
"compression method zstd_dict not supported" because ZDICT lives in
libzstd; a clearer message ("dictionary training requires zstd support")
would be less confusing.
- compression_dict_evolve.sql:18-19: stale comment ("the row count is held
constant below") now that the test grows from 4,000 to 12,000 rows.
- SGML docs:
* catalogs.sgml omits tdtrainrows and tdtraintime from
pg_toast_dictionary, omits 'd' and 'e' from pg_attribute.attcompression,
and says DROP COLUMN immediately deletes the column's dictionaries
(they remain until a table rewrite or DROP TABLE).
* func-admin.sgml and alter_table.sgml mention zstd_dict in a couple of
places where lz4_dict also applies.
Cheers,
Hannu
