From: Antonin Houska <ah@cybertec.at>
To: Ewan Young <kdbase.hack@gmail.com>
cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
    mihailnikalayeu@gmail.com, alvherre@kurilemu.de
Subject: Re: REPACK CONCURRENTLY fails on tables with generated columns
In-reply-to: 
 <CAON2xHMrELwx9vKg6niSf8fMBA=-MGXmG=MPQU6+vMVhGjF8kQ@mail.gmail.com>
References: 
 <CAON2xHMrELwx9vKg6niSf8fMBA=-MGXmG=MPQU6+vMVhGjF8kQ@mail.gmail.com>
Comments: In-reply-to Ewan Young <kdbase.hack@gmail.com>
   message dated "Fri, 12 Jun 2026 16:39:54 +0800."
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Date: Mon, 22 Jun 2026 13:12:11 +0200
Message-ID: <18222.1782126731@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/18222.1782126731%40localhost>
Precedence: bulk

--=-=-=
Content-Type: text/plain

Ewan Young <kdbase.hack@gmail.com> 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


--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment;
 filename=0001-Copy-the-relevant-pg_attrdef-catalog-entries-for-the.patch

From b6ae449d32c2b2ce7ec12605effc32b579497c2f Mon Sep 17 00:00:00 2001
From: Antonin Houska <ah@cybertec.at>
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


--=-=-=--