Ewan Young <[email protected]> wrote: > The transient heap built by make_new_heap() is intentionally created > without the old table's defaults and constraints, so it has no generation > expressions for its generated columns, even though the tuple descriptor > still has attgenerated set. > > When apply_concurrent_update() replays a non-HOT update, it calls > ExecInsertIndexTuples() with EIIT_IS_UPDATE. To decide whether to pass > the "indexUnchanged" hint, that calls index_unchanged_by_update() -> > ExecGetExtraUpdatedCols() -> ExecInitGenerated(), which looks up the > generation expression of each generated column via build_column_default() > and errors out when it finds none on the transient heap. > > The apply path does not need to recompute generated columns at all: the > decoded tuple already carries the correct value, and it is only inserted. > Note also that ExecGetUpdatedCols() already returns an empty set for this > ResultRelInfo, because it is not part of any range table -- so the > indexUnchanged determination here is already approximate.
I'm sorry for the confusion, but the fact that ExecGetUpdatedCols() returns an empty set is an omission rather than deliberate choice. Assuming we fix that, the result of ExecGetExtraUpdatedCols() does matter. Thus we should copy the related pg_attrdef entries, as I suggest in this patch. Another question is how serious problem it is that ExecGetUpdatedCols() returns empty set. AFAICS, "indexUnchanged" does not affect correctness - it's is only a hint that helps the btree AM decide whent the bottom-up deletion and de-duplication techniques should (not) be used. I'm not sure it's easy to update the set for individual UPDATEs: the UPDATE commands REPACK replays originate from different SQL queries and the logical decoding does not transfer this information. Even then, I think it'd be "less bad" to have ExecGetUpdatedCols() return a set containing all the attributes rather than empty set. That is, avoid using the btree optimizations altogether rather than allow them them when not appropriate. However, per index_unchanged_by_update(), if ExecGetUpdatedCols() tells that all columns are updated, the result of ExecGetExtraUpdatedCols() does not matter. Nevertheless, I'd still slightly prefer copying the pg_attrdef entries to hacking the executor. -- Antonin Houska Web: https://www.cybertec-postgresql.com
>From b6ae449d32c2b2ce7ec12605effc32b579497c2f Mon Sep 17 00:00:00 2001 From: Antonin Houska <[email protected]> Date: Mon, 22 Jun 2026 09:34:05 +0200 Subject: [PATCH] Copy the relevant pg_attrdef catalog entries for the transient relation. The default values may be needed by the executor when processing the concurrent data changes. In particular, ExecInsertIndexTuples() needs it when determining the value of the 'indexUnchanged' hint for the index AM. Like in copy_index_constraints(), we make the new catalog entries dependent on the transient relation, so they are dropped along with it automatically. --- src/backend/commands/repack.c | 97 +++++++++++++++++++ .../injection_points/specs/repack.spec | 3 +- 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index 4d177c868bb..725efa236bc 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -48,6 +48,7 @@ #include "catalog/namespace.h" #include "catalog/objectaccess.h" #include "catalog/pg_am.h" +#include "catalog/pg_attrdef.h" #include "catalog/pg_constraint.h" #include "catalog/pg_inherits.h" #include "catalog/toasting.h" @@ -204,6 +205,7 @@ static void rebuild_relation_finish_concurrent(Relation NewHeap, Relation OldHea static List *build_new_indexes(Relation NewHeap, Relation OldHeap, List *OldIndexes); static void copy_index_constraints(Relation old_index, Oid new_index_id, Oid new_heap_id); +static void copy_attribute_defaults(Relation old_heap, Relation new_heap); static Relation process_single_relation(RepackStmt *stmt, LOCKMODE lockmode, bool isTopLevel, @@ -1083,6 +1085,13 @@ rebuild_relation(Relation OldHeap, Relation index, bool verbose, Assert(CheckRelationOidLockedByMe(OIDNewHeap, AccessExclusiveLock, false)); NewHeap = table_open(OIDNewHeap, NoLock); + /* + * Copy attribute defaults - the executor may need them, in order to + * process the concurrent data changes. In particular, this is related to + * ExecInsertIndexTuples(). + */ + copy_attribute_defaults(OldHeap, NewHeap); + /* Copy the heap data into the new table in the desired order */ copy_table_data(NewHeap, OldHeap, index, snapshot, verbose, &swap_toast_by_content, &frozenXid, &cutoffMulti); @@ -3434,6 +3443,94 @@ copy_index_constraints(Relation old_index, Oid new_index_id, Oid new_heap_id) CommandCounterIncrement(); } +/* + * Create a transient copy of attribute defaults for the transient table. + * + * Like above, the executor needs information on attribute defaults. Once the + * repacking is finished, the catalog entries we create here are dropped. + */ +static void +copy_attribute_defaults(Relation old_heap, Relation new_heap) +{ + Oid old_heap_id = RelationGetRelid(old_heap); + Oid new_heap_id = RelationGetRelid(new_heap); + ScanKeyData skey; + Relation rel; + Relation att_rel = NULL; + TupleDesc desc; + SysScanDesc scan; + HeapTuple tup; + ObjectAddress objrel; + + rel = table_open(AttrDefaultRelationId, RowExclusiveLock); + ObjectAddressSet(objrel, RelationRelationId, new_heap_id); + + ScanKeyInit(&skey, + Anum_pg_attrdef_adrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(old_heap_id)); + scan = systable_beginscan(rel, AttrDefaultIndexId, true, + NULL, 1, &skey); + desc = RelationGetDescr(rel); + while (HeapTupleIsValid(tup = systable_getnext(scan))) + { + Form_pg_attrdef adform; + Oid oid; + Datum values[Natts_pg_attrdef] = {0}; + bool nulls[Natts_pg_attrdef] = {0}; + bool replaces[Natts_pg_attrdef] = {0}; + HeapTuple new_tup, att_tup, att_new_tup; + ObjectAddress objad; + Datum att_values[Natts_pg_attribute] = {0}; + bool att_nulls[Natts_pg_attribute] = {0}; + bool att_replaces[Natts_pg_attribute] = {0}; + + adform = (Form_pg_attrdef) GETSTRUCT(tup); + Assert(adform->adrelid == old_heap_id); + + oid = GetNewOidWithIndex(rel, AttrDefaultOidIndexId, + Anum_pg_attrdef_oid); + values[Anum_pg_attrdef_oid - 1] = ObjectIdGetDatum(oid); + replaces[Anum_pg_attrdef_oid - 1] = true; + values[Anum_pg_attrdef_adrelid - 1] = ObjectIdGetDatum(new_heap_id); + replaces[Anum_pg_attrdef_adrelid - 1] = true; + + new_tup = heap_modify_tuple(tup, desc, values, nulls, replaces); + + /* Insert it into the catalog. */ + CatalogTupleInsert(rel, new_tup); + + /* Create a dependency so it's removed when we drop the new heap. */ + ObjectAddressSet(objad, AttrDefaultRelationId, oid); + recordDependencyOn(&objad, &objrel, DEPENDENCY_AUTO); + + /* Set atthasdef - new heap has it cleared. */ + att_tup = SearchSysCache2(ATTNUM, + ObjectIdGetDatum(new_heap_id), + ObjectIdGetDatum(adform->adnum)); + if (!HeapTupleIsValid(att_tup)) + elog(ERROR, "cache lookup failed for attribute %d of relation %u", + adform->adnum, new_heap_id); + + att_values[Anum_pg_attribute_atthasdef - 1] = BoolGetDatum(true); + att_replaces[Anum_pg_attribute_atthasdef - 1] = true; + + if (att_rel == NULL) + att_rel = table_open(AttributeRelationId, RowExclusiveLock); + att_new_tup = heap_modify_tuple(att_tup, RelationGetDescr(att_rel), + att_values, att_nulls, att_replaces); + ReleaseSysCache(att_tup); + CatalogTupleUpdate(att_rel, &att_new_tup->t_self, att_new_tup); + } + systable_endscan(scan); + + table_close(rel, RowExclusiveLock); + if (att_rel) + table_close(att_rel, RowExclusiveLock); + + CommandCounterIncrement(); +} + /* * Try to start a background worker to perform logical decoding of data * changes applied to relation while REPACK CONCURRENTLY is copying its diff --git a/src/test/modules/injection_points/specs/repack.spec b/src/test/modules/injection_points/specs/repack.spec index d727a9b056b..7896d1456ad 100644 --- a/src/test/modules/injection_points/specs/repack.spec +++ b/src/test/modules/injection_points/specs/repack.spec @@ -3,7 +3,8 @@ setup { CREATE EXTENSION injection_points; - CREATE TABLE repack_test(i int PRIMARY KEY, j int); + CREATE TABLE repack_test(i int PRIMARY KEY, j int, + k int GENERATED ALWAYS AS (j * 2) STORED); INSERT INTO repack_test(i, j) VALUES (1, 1), (2, 2), (3, 3), (4, 4); CREATE TABLE relfilenodes(node oid); -- 2.52.0
