Feedback-ID: ie3de48e3:Fastmail
Date: Fri, 3 Jul 2026 13:12:39 +0200
From: Alvaro Herrera <alvherre@kurilemu.de>
To: Ewan Young <kdbase.hack@gmail.com>
Cc: Antonin Houska <ah@cybertec.at>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 mihailnikalayeu@gmail.com
Subject: Re: REPACK CONCURRENTLY fails on tables with generated columns
Message-ID: <akeN0fZ_Dc4SSjN0@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="pi6rmivyoaehyqn6"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAON2xHOV3XBqvPiM+-TAAaQeDVHTF33JoUBCRgoQpDpc4k33aw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/akeN0fZ_Dc4SSjN0%40alvherre.pgsql>
Precedence: bulk


--pi6rmivyoaehyqn6
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Hello,

On 2026-Jun-22, Ewan Young wrote:

> I applied the patch and ran it through an injection-point reproducer
> (cassert). Without the fix the bug reproduces (ERROR: no generation
> expression found for column number 3 ...); with it, REPACK CONCURRENTLY
> succeeds under a concurrent non-HOT UPDATE for a STORED generated column, an
> index directly on the generated column, and a VIRTUAL column, with correct
> values afterwards. Your repack.spec change passes.
> 
> The approach is right and I've confirmed it fixes the bug, so +1 from me in
> this direction.

Cool, thanks for reviewing -- I have pushed this fix, with some
stylistic changes and one bigger change: these catalog rows are only
needed in concurrent mode, so there was no reason to copy them in the
other case.  So I restricted the copying to that case.

I've been looking at the other proposed change, and I agree with it.
Here's it, again with some style changes, and only one other proposed
change: for setting up updatedCols, ignore dropped columns.  I don't
think this should change anything in practice, but it just feels wrong
to claim that a dropped column is being changed by an update.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

--pi6rmivyoaehyqn6
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename=0001-REPACK-CONCURRENTLY-Initialize-the-range-table-more-.patch

From 6d85052127d9f304a95034521567819a3f0b4f31 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Fri, 3 Jul 2026 12:47:54 +0200
Subject: [PATCH] REPACK CONCURRENTLY: Initialize the range table more honestly

We were skipping a bunch of things that are mostly unnecessary for
REPACK.  However, one thing that seems would be better to pass closer to
truth, is the updatedCols bitmapset in the range table entry for the
repacked table.  Cons up an RTE and install it into the EState.

This only has an effect on btree indexes, because certain operations are
optimized in the case of unchanged columns; and even then, correctnesss
is not being compromised.

The values we pass after this commit are not fully trustworthy either,
because we simply say "all columns were updated" for all insert/updates,
regardless of whether their values were actually modified or not.
However, this way we err to the side of caution rather than to the
opposite direction as we were originally doing.  This could be refined
in the future, but there's a trade-off: determining whether the column
was in fact updated could be expensive.

Author: Antonin Houska <ah@cybertec.at>
Reviewed-by: Ewan Young <kdbase.hack@gmail.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/18222.1782126731@localhost
---
 src/backend/commands/repack.c | 57 +++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 2 deletions(-)

diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 83a49afe7e1..faa07d1a118 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -63,6 +63,7 @@
 #include "libpq/pqmq.h"
 #include "miscadmin.h"
 #include "optimizer/optimizer.h"
+#include "parser/parse_relation.h"
 #include "pgstat.h"
 #include "replication/logicalrelation.h"
 #include "storage/bufmgr.h"
@@ -3019,8 +3020,60 @@ initialize_change_context(ChangeContext *chgcxt,
 	/* Only initialize fields needed by ExecInsertIndexTuples(). */
 	chgcxt->cc_estate = CreateExecutorState();
 
-	chgcxt->cc_rri = (ResultRelInfo *) palloc(sizeof(ResultRelInfo));
-	InitResultRelInfo(chgcxt->cc_rri, relation, 0, 0, 0);
+	/*
+	 * Set up a range table for the executor, containing our repacked table as
+	 * its only member.
+	 */
+	{
+		RangeTblEntry *rte;
+		TupleDesc	desc = RelationGetDescr(relation);
+		List	   *perminfos = NIL;
+		Bitmapset  *updatedCols = NULL;
+		RTEPermissionInfo *perminfo;
+
+		/*
+		 * For our use, the RTE only needs to have perminfoindex initialized,
+		 * but there's no reason to not set the fields whose values we have at
+		 * hand.
+		 */
+		rte = makeNode(RangeTblEntry);
+		rte->rtekind = RTE_RELATION;
+		rte->relid = RelationGetRelid(relation);
+		rte->relkind = RelationGetForm(relation)->relkind;
+		/* Create the RTEPermissionInfo instance (and set ->perminfoindex). */
+		addRTEPermissionInfo(&perminfos, rte);
+
+		/*
+		 * Initialize updatedCols to show that all columns are updated.  This
+		 * is of course not necessarily true, and we cannot know this early;
+		 * but this is only used by ExecInsertIndexTuples to flag index
+		 * updates with no logical value changes, so if it's wrong, nothing
+		 * terribly bad happens. We may want to improve this someday though.
+		 *
+		 * Don't claim that dropped columns are changed though.
+		 */
+		for (int i = 0; i < desc->natts; i++)
+		{
+			CompactAttribute *attr = TupleDescCompactAttr(desc, i);
+
+			if (attr->attisdropped)
+				continue;
+			updatedCols = bms_add_member(updatedCols,
+										 i + 1 - FirstLowInvalidHeapAttributeNumber);
+		}
+
+		/* install updatedCols in the right place */
+		perminfo = getRTEPermissionInfo(perminfos, rte);
+		perminfo->updatedCols = updatedCols;
+
+		/* finally we can initialize the range table proper */
+		ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos,
+						   bms_make_singleton(1));
+	}
+
+	/* Set up our ResultRelInfo to use for index updates */
+	chgcxt->cc_rri = makeNode(ResultRelInfo);
+	InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0);
 	ExecOpenIndices(chgcxt->cc_rri, false);
 
 	/*
-- 
2.47.3


--pi6rmivyoaehyqn6--