agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedREPACK CONCURRENTLY fails on tables with generated columns
14+ messages / 4 participants
[nested] [flat]
* REPACK CONCURRENTLY fails on tables with generated columns
@ 2026-06-12 08:39 Ewan Young <kdbase.hack@gmail.com>
2026-06-12 09:52 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
2026-06-14 03:56 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Srinath Reddy Sadipiralla <srinath2133@gmail.com>
2026-06-22 11:12 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
0 siblings, 3 replies; 14+ messages in thread
From: Ewan Young @ 2026-06-12 08:39 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; +Cc: ah@cybertec.at; mihailnikalayeu@gmail.com; alvherre@kurilemu.de
Hi,
REPACK (CONCURRENTLY) aborts with an internal error on any table that has
a STORED generated column, if a concurrent UPDATE that requires index
maintenance is applied during the catch-up phase:
ERROR: no generation expression found for column number 3 of table
"pg_temp_16396"
Plain (non-concurrent) REPACK on such a table works fine, and so does
REPACK (CONCURRENTLY) as long as no qualifying concurrent change is
applied -- so the problem is specific to the concurrent-change apply path.
The attached patch adds an isolation test, but here is the manual
sequence (server built with --enable-injection-points):
CREATE EXTENSION injection_points;
CREATE TABLE t (i int PRIMARY KEY, v int,
g int GENERATED ALWAYS AS (v * 10) STORED);
CREATE INDEX ON t (v); -- makes UPDATE of v non-HOT
INSERT INTO t(i, v) VALUES (1, 1);
-- session 1:
SELECT injection_points_attach('repack-concurrently-before-lock', 'wait');
REPACK (CONCURRENTLY) t; -- blocks at the injection point
-- session 2, once session 1 is waiting:
UPDATE t SET v = v + 1 WHERE i = 1;
SELECT injection_points_wakeup('repack-concurrently-before-lock');
-- session 1 then fails with the ERROR above.
Without injection points this is a race: the concurrent UPDATE has to be
decoded and applied during catch-up, and it has to be a non-HOT update
(one that goes through index maintenance). It is reliably hit on a busy
table with a generated column.
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.
Regards,
Ewan Young
Attachments:
[application/octet-stream] v1-0001-Fix-REPACK-CONCURRENTLY-on-tables-with-generated-.patch (7.5K, ../../CAON2xHMrELwx9vKg6niSf8fMBA=-MGXmG=MPQU6+vMVhGjF8kQ@mail.gmail.com/2-v1-0001-Fix-REPACK-CONCURRENTLY-on-tables-with-generated-.patch)
download | inline diff:
From 35b808572be533eba3565b68b9f96295f66c5cd1 Mon Sep 17 00:00:00 2001
From: Ewan Young <kdbase.hack@gmail.com>
Date: Sat, 13 Jun 2026 00:14:38 +0800
Subject: [PATCH v1] Fix REPACK CONCURRENTLY on tables with generated columns
When REPACK (CONCURRENTLY) applies concurrent data changes to the
transient new heap during the catch-up phase, a non-HOT UPDATE goes
through ExecInsertIndexTuples(), which may call ExecGetExtraUpdatedCols()
to decide whether to pass the "indexUnchanged" hint to the index AM.
That in turn calls ExecInitGenerated(), which looks up the generation
expressions of the relation's STORED generated columns.
The transient heap is intentionally created without the defaults and
constraints of the original table (see make_new_heap()), so it has no
generation expressions, and the lookup failed with:
ERROR: no generation expression found for column number %d of table "%s"
This aborted REPACK (CONCURRENTLY) on any table having a STORED generated
column whenever a concurrent UPDATE that required index maintenance was
applied during catch-up.
The apply path replays already-decoded tuples, which already carry the
correct generated values; it must not recompute them. Mark the
generated-column info of the apply ResultRelInfo as already computed
(with an empty set) so that ExecInsertIndexTuples() does not invoke
ExecInitGenerated(). This is consistent with ExecGetUpdatedCols(), which
already returns an empty set here because this ResultRelInfo is not part
of any range table.
Add an isolation test exercising a non-HOT UPDATE applied during REPACK
(CONCURRENTLY) catch-up on a table with a generated column.
---
src/backend/commands/repack.c | 14 ++++
src/test/modules/injection_points/Makefile | 1 +
.../expected/repack_generated.out | 36 ++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../specs/repack_generated.spec | 65 +++++++++++++++++++
5 files changed, 117 insertions(+)
create mode 100644 src/test/modules/injection_points/expected/repack_generated.out
create mode 100644 src/test/modules/injection_points/specs/repack_generated.spec
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index ec100e3eef5..e9e90114054 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -3011,6 +3011,20 @@ initialize_change_context(ChangeContext *chgcxt,
InitResultRelInfo(chgcxt->cc_rri, relation, 0, 0, 0);
ExecOpenIndices(chgcxt->cc_rri, false);
+ /*
+ * The new heap is created without the defaults and constraints of the old
+ * heap (see make_new_heap()), so it has no generation expressions for its
+ * generated columns. When applying concurrent changes we must not try to
+ * compute those expressions: the decoded tuples already carry the correct
+ * generated values, and we only insert them. Mark the generated-column
+ * info as already computed (with an empty set) so that the index
+ * maintenance performed by ExecInsertIndexTuples() does not call
+ * ExecInitGenerated(), which would fail to find the missing expressions.
+ * This matches ExecGetUpdatedCols(), which is already empty here because
+ * this ResultRelInfo is not in any range table.
+ */
+ chgcxt->cc_rri->ri_extraUpdatedCols_valid = true;
+
/*
* The table's relcache entry already has the relcache entry for the
* identity index; find that.
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index c01d2fb095c..0e5d4ba9973 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -15,6 +15,7 @@ REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
ISOLATION = basic \
inplace \
repack \
+ repack_generated \
repack_temporal \
repack_temporal_multirange \
repack_toast \
diff --git a/src/test/modules/injection_points/expected/repack_generated.out b/src/test/modules/injection_points/expected/repack_generated.out
new file mode 100644
index 00000000000..fc7cf2011c5
--- /dev/null
+++ b/src/test/modules/injection_points/expected/repack_generated.out
@@ -0,0 +1,36 @@
+Parsed test spec with 2 sessions
+
+starting permutation: wait_before_lock change_existing wakeup_before_lock check
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step wait_before_lock:
+ REPACK (CONCURRENTLY) repack_test USING INDEX repack_test_pkey;
+ <waiting ...>
+step change_existing:
+ UPDATE repack_test SET v = v + 1 WHERE i = 1;
+
+step wakeup_before_lock:
+ SELECT injection_points_wakeup('repack-concurrently-before-lock');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step wait_before_lock: <... completed>
+step check:
+ SELECT i, v, g FROM repack_test ORDER BY i;
+
+i|v| g
+-+-+--
+1|2|20
+(1 row)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 59dba1cb023..fb92cfa948a 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -46,6 +46,7 @@ tests += {
'basic',
'inplace',
'repack',
+ 'repack_generated',
'repack_temporal',
'repack_temporal_multirange',
'repack_toast',
diff --git a/src/test/modules/injection_points/specs/repack_generated.spec b/src/test/modules/injection_points/specs/repack_generated.spec
new file mode 100644
index 00000000000..3d057f490f7
--- /dev/null
+++ b/src/test/modules/injection_points/specs/repack_generated.spec
@@ -0,0 +1,65 @@
+# Test REPACK (CONCURRENTLY) on a table with a STORED generated column.
+#
+# When a concurrent UPDATE that requires index maintenance (a non-HOT update)
+# is applied to the transient heap during the catch-up phase, the index
+# maintenance code used to try to look up the generated column's expression on
+# the transient heap. That heap is created without defaults/constraints, so
+# the lookup failed with "no generation expression found ...". The decoded
+# tuple already carries the correct generated value, so REPACK must just apply
+# it and recompute nothing.
+setup
+{
+ CREATE EXTENSION injection_points;
+
+ CREATE TABLE repack_test(i int PRIMARY KEY, v int,
+ g int GENERATED ALWAYS AS (v * 10) STORED);
+ -- Index on v so that an UPDATE of v is a non-HOT update, forcing the
+ -- concurrent-change apply path through index maintenance.
+ CREATE INDEX repack_test_v_idx ON repack_test(v);
+ INSERT INTO repack_test(i, v) VALUES (1, 1);
+}
+
+teardown
+{
+ DROP TABLE repack_test;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+setup
+{
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('repack-concurrently-before-lock', 'wait');
+}
+# Perform the initial load and wait for s2 to do a data change.
+step wait_before_lock
+{
+ REPACK (CONCURRENTLY) repack_test USING INDEX repack_test_pkey;
+}
+teardown
+{
+ SELECT injection_points_detach('repack-concurrently-before-lock');
+}
+
+session s2
+# A non-HOT UPDATE (v is indexed) of the existing row, applied during catch-up.
+step change_existing
+{
+ UPDATE repack_test SET v = v + 1 WHERE i = 1;
+}
+step wakeup_before_lock
+{
+ SELECT injection_points_wakeup('repack-concurrently-before-lock');
+}
+# REPACK must succeed and the concurrent change must be reflected, with the
+# generated column recomputed correctly (g = v * 10).
+step check
+{
+ SELECT i, v, g FROM repack_test ORDER BY i;
+}
+
+permutation
+ wait_before_lock
+ change_existing
+ wakeup_before_lock
+ check
--
2.47.3
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: REPACK CONCURRENTLY fails on tables with generated columns
2026-06-12 08:39 REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
@ 2026-06-12 09:52 ` Antonin Houska <ah@cybertec.at>
2026-06-12 10:25 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Alvaro Herrera <alvherre@kurilemu.de>
2 siblings, 1 reply; 14+ messages in thread
From: Antonin Houska @ 2026-06-12 09:52 UTC (permalink / raw)
To: Ewan Young <kdbase.hack@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; mihailnikalayeu@gmail.com; alvherre@kurilemu.de
Ewan Young <kdbase.hack@gmail.com> wrote:
> REPACK (CONCURRENTLY) aborts with an internal error on any table that has
> a STORED generated column, if a concurrent UPDATE that requires index
> maintenance is applied during the catch-up phase:
>
> ERROR: no generation expression found for column number 3 of table
> "pg_temp_16396"
> Plain (non-concurrent) REPACK on such a table works fine, and so does
> REPACK (CONCURRENTLY) as long as no qualifying concurrent change is
> applied -- so the problem is specific to the concurrent-change apply path.
Thanks for the report!
> The attached patch adds an isolation test, but here is the manual
> sequence (server built with --enable-injection-points):
>
> CREATE EXTENSION injection_points;
> CREATE TABLE t (i int PRIMARY KEY, v int,
> g int GENERATED ALWAYS AS (v * 10) STORED);
> CREATE INDEX ON t (v); -- makes UPDATE of v non-HOT
> INSERT INTO t(i, v) VALUES (1, 1);
>
> -- session 1:
> SELECT injection_points_attach('repack-concurrently-before-lock', 'wait');
> REPACK (CONCURRENTLY) t; -- blocks at the injection point
>
> -- session 2, once session 1 is waiting:
> UPDATE t SET v = v + 1 WHERE i = 1;
> SELECT injection_points_wakeup('repack-concurrently-before-lock');
>
> -- session 1 then fails with the ERROR above.
I confirm I can reproduce it. I'll post a fix next week.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: REPACK CONCURRENTLY fails on tables with generated columns
2026-06-12 08:39 REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2026-06-12 09:52 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
@ 2026-06-12 10:25 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-06-12 11:01 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 14+ messages in thread
From: Alvaro Herrera @ 2026-06-12 10:25 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Ewan Young <kdbase.hack@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; mihailnikalayeu@gmail.com
On 2026-Jun-12, Antonin Houska wrote:
> I confirm I can reproduce it. I'll post a fix next week.
Didn't you like Ewan's proposed patch?
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Update: super-fast reaction on the Postgres bugs mailing list. The report
was acknowledged [...], and a fix is under discussion.
The wonders of open-source !"
https://twitter.com/gunnarmorling/status/1596080409259003906
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: REPACK CONCURRENTLY fails on tables with generated columns
2026-06-12 08:39 REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2026-06-12 09:52 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
2026-06-12 10:25 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Alvaro Herrera <alvherre@kurilemu.de>
@ 2026-06-12 11:01 ` Antonin Houska <ah@cybertec.at>
2026-06-12 11:40 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 14+ messages in thread
From: Antonin Houska @ 2026-06-12 11:01 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@kurilemu.de>; +Cc: Ewan Young <kdbase.hack@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; mihailnikalayeu@gmail.com
Alvaro Herrera <alvherre@kurilemu.de> wrote:
> On 2026-Jun-12, Antonin Houska wrote:
>
> > I confirm I can reproduce it. I'll post a fix next week.
>
> Didn't you like Ewan's proposed patch?
After having read the email, I thought that the attachment only contains the
reproducing test. Now that I look into it, I see it also contains a fix. Sorry
for the confusion.
I cannot concentrate on it today, so if I'm supposed to review it, I need a
few days.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: REPACK CONCURRENTLY fails on tables with generated columns
2026-06-12 08:39 REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2026-06-12 09:52 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
2026-06-12 10:25 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Alvaro Herrera <alvherre@kurilemu.de>
2026-06-12 11:01 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
@ 2026-06-12 11:40 ` Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 0 replies; 14+ messages in thread
From: Alvaro Herrera @ 2026-06-12 11:40 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Ewan Young <kdbase.hack@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; mihailnikalayeu@gmail.com
Hello,
On 2026-Jun-12, Antonin Houska wrote:
> After having read the email, I thought that the attachment only contains the
> reproducing test. Now that I look into it, I see it also contains a fix. Sorry
> for the confusion.
>
> I cannot concentrate on it today, so if I'm supposed to review it, I need a
> few days.
I would welcome your review, but it's not strictly mandatory. That
said, we have some weeks before the next beta, so there's no rush.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"After a quick R of TFM, all I can say is HOLY CR** THAT IS COOL! PostgreSQL was
amazing when I first started using it at 7.2, and I'm continually astounded by
learning new features and techniques made available by the continuing work of
the development team."
Berend Tober, http://archives.postgresql.org/pgsql-hackers/2007-08/msg01009.php
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: REPACK CONCURRENTLY fails on tables with generated columns
2026-06-12 08:39 REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
@ 2026-06-14 03:56 ` Srinath Reddy Sadipiralla <srinath2133@gmail.com>
2026-06-16 02:02 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2 siblings, 1 reply; 14+ messages in thread
From: Srinath Reddy Sadipiralla @ 2026-06-14 03:56 UTC (permalink / raw)
To: Ewan Young <kdbase.hack@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; ah@cybertec.at; mihailnikalayeu@gmail.com; alvherre@kurilemu.de
Hi Ewan,
On Fri, Jun 12, 2026 at 2:10 PM Ewan Young <kdbase.hack@gmail.com> wrote:
> Hi,
>
> REPACK (CONCURRENTLY) aborts with an internal error on any table that has
> a STORED generated column, if a concurrent UPDATE that requires index
> maintenance is applied during the catch-up phase:
>
> ERROR: no generation expression found for column number 3 of table
> "pg_temp_16396"
> Plain (non-concurrent) REPACK on such a table works fine, and so does
> REPACK (CONCURRENTLY) as long as no qualifying concurrent change is
> applied -- so the problem is specific to the concurrent-change apply path.
>
> The attached patch adds an isolation test, but here is the manual
> sequence (server built with --enable-injection-points):
>
> CREATE EXTENSION injection_points;
> CREATE TABLE t (i int PRIMARY KEY, v int,
> g int GENERATED ALWAYS AS (v * 10) STORED);
> CREATE INDEX ON t (v); -- makes UPDATE of v non-HOT
> INSERT INTO t(i, v) VALUES (1, 1);
>
> -- session 1:
> SELECT injection_points_attach('repack-concurrently-before-lock', 'wait');
> REPACK (CONCURRENTLY) t; -- blocks at the injection point
>
> -- session 2, once session 1 is waiting:
> UPDATE t SET v = v + 1 WHERE i = 1;
> SELECT injection_points_wakeup('repack-concurrently-before-lock');
>
> -- session 1 then fails with the ERROR above.
> Without injection points this is a race: the concurrent UPDATE has to be
> decoded and applied during catch-up, and it has to be a non-HOT update
> (one that goes through index maintenance). It is reliably hit on a busy
> table with a generated column.
>
I was able to reproduce this.
>
> 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.
>
makes sense, i have reviewed the patch, it LGTM.
--
Thanks :)
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: REPACK CONCURRENTLY fails on tables with generated columns
2026-06-12 08:39 REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2026-06-14 03:56 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Srinath Reddy Sadipiralla <srinath2133@gmail.com>
@ 2026-06-16 02:02 ` Ewan Young <kdbase.hack@gmail.com>
0 siblings, 0 replies; 14+ messages in thread
From: Ewan Young @ 2026-06-16 02:02 UTC (permalink / raw)
To: Srinath Reddy Sadipiralla <srinath2133@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; ah@cybertec.at; mihailnikalayeu@gmail.com; alvherre@kurilemu.de
Hi Srinath,
Thanks a lot for reproducing it and for the careful review.
On Sun, Jun 14, 2026 at 11:57 AM Srinath Reddy Sadipiralla
<srinath2133@gmail.com> wrote:
>
> Hi Ewan,
>
> On Fri, Jun 12, 2026 at 2:10 PM Ewan Young <kdbase.hack@gmail.com> wrote:
>>
>> Hi,
>>
>> REPACK (CONCURRENTLY) aborts with an internal error on any table that has
>> a STORED generated column, if a concurrent UPDATE that requires index
>> maintenance is applied during the catch-up phase:
>>
>> ERROR: no generation expression found for column number 3 of table
>> "pg_temp_16396"
>> Plain (non-concurrent) REPACK on such a table works fine, and so does
>> REPACK (CONCURRENTLY) as long as no qualifying concurrent change is
>> applied -- so the problem is specific to the concurrent-change apply path.
>>
>> The attached patch adds an isolation test, but here is the manual
>> sequence (server built with --enable-injection-points):
>>
>> CREATE EXTENSION injection_points;
>> CREATE TABLE t (i int PRIMARY KEY, v int,
>> g int GENERATED ALWAYS AS (v * 10) STORED);
>> CREATE INDEX ON t (v); -- makes UPDATE of v non-HOT
>> INSERT INTO t(i, v) VALUES (1, 1);
>>
>> -- session 1:
>> SELECT injection_points_attach('repack-concurrently-before-lock', 'wait');
>> REPACK (CONCURRENTLY) t; -- blocks at the injection point
>>
>> -- session 2, once session 1 is waiting:
>> UPDATE t SET v = v + 1 WHERE i = 1;
>> SELECT injection_points_wakeup('repack-concurrently-before-lock');
>>
>> -- session 1 then fails with the ERROR above.
>> Without injection points this is a race: the concurrent UPDATE has to be
>> decoded and applied during catch-up, and it has to be a non-HOT update
>> (one that goes through index maintenance). It is reliably hit on a busy
>> table with a generated column.
>
>
> I was able to reproduce this.
>
>>
>>
>> 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.
>
>
> makes sense, i have reviewed the patch, it LGTM.
>
> --
> Thanks :)
> Srinath Reddy Sadipiralla
> EDB: https://www.enterprisedb.com/
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: REPACK CONCURRENTLY fails on tables with generated columns
2026-06-12 08:39 REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
@ 2026-06-22 11:12 ` Antonin Houska <ah@cybertec.at>
2026-06-22 12:49 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2 siblings, 1 reply; 14+ messages in thread
From: Antonin Houska @ 2026-06-22 11:12 UTC (permalink / raw)
To: Ewan Young <kdbase.hack@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; mihailnikalayeu@gmail.com; alvherre@kurilemu.de
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
Attachments:
[text/x-diff] 0001-Copy-the-relevant-pg_attrdef-catalog-entries-for-the.patch (6.1K, ../../18222.1782126731@localhost/2-0001-Copy-the-relevant-pg_attrdef-catalog-entries-for-the.patch)
download | inline diff:
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
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: REPACK CONCURRENTLY fails on tables with generated columns
2026-06-12 08:39 REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2026-06-22 11:12 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
@ 2026-06-22 12:49 ` Ewan Young <kdbase.hack@gmail.com>
2026-07-01 13:57 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
2026-07-03 11:12 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Alvaro Herrera <alvherre@kurilemu.de>
0 siblings, 2 replies; 14+ messages in thread
From: Ewan Young @ 2026-06-22 12:49 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; mihailnikalayeu@gmail.com; alvherre@kurilemu.de
Hi Antonin,
On Mon, Jun 22, 2026 at 7:12 PM Antonin Houska <ah@cybertec.at> wrote:
>
> 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.
Agreed, thanks for the correction. Relying on the empty ExecGetUpdatedCols()
set was the weak point of my version -- it's an omission, not something to
build a second approximation on. Copying the pg_attrdef entries fixes the
inconsistency at the root, so let's go with your approach.
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.
>
> --
> Antonin Houska
> Web: https://www.cybertec-postgresql.com
>
Regards,
Ewan Young
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: REPACK CONCURRENTLY fails on tables with generated columns
2026-06-12 08:39 REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2026-06-22 11:12 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
2026-06-22 12:49 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
@ 2026-07-01 13:57 ` Antonin Houska <ah@cybertec.at>
2026-07-02 01:39 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
1 sibling, 1 reply; 14+ messages in thread
From: Antonin Houska @ 2026-07-01 13:57 UTC (permalink / raw)
To: Ewan Young <kdbase.hack@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; mihailnikalayeu@gmail.com; alvherre@kurilemu.de
Ewan Young <kdbase.hack@gmail.com> wrote:
> On Mon, Jun 22, 2026 at 7:12 PM Antonin Houska <ah@cybertec.at> wrote:
> >
> > 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.
>
> Agreed, thanks for the correction. Relying on the empty ExecGetUpdatedCols()
> set was the weak point of my version -- it's an omission, not something to
> build a second approximation on. Copying the pg_attrdef entries fixes the
> inconsistency at the root, so let's go with your approach.
>
> 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.
Thanks for checking. Here, 0002 tries to fix the problem of empty updatedCols,
as I proposed above.
0001 fixes two minor coding issues that I found when writing 0002.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
Attachments:
[text/x-diff] 0001-Minor-cleanup-in-initialize_change_context.patch (1.1K, ../../60966.1782914222@localhost/2-0001-Minor-cleanup-in-initialize_change_context.patch)
download | inline diff:
From 620b806eb50738837bba2f8c0cbf838bc360c61f Mon Sep 17 00:00:00 2001
From: Antonin Houska <ah@cybertec.at>
Date: Wed, 1 Jul 2026 14:58:29 +0200
Subject: [PATCH 1/2] Minor cleanup in initialize_change_context().
First, use makeNode() to create an instance of ResultRelInfo, like we do
elsewhere in the tree.
Second, pass NULL for the 'partition_root_rri' argument of InitResultRelInfo
instead of 0.
---
src/backend/commands/repack.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 4d177c868bb..c9b0c047477 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -3010,8 +3010,8 @@ 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);
+ chgcxt->cc_rri = makeNode(ResultRelInfo);
+ InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0);
ExecOpenIndices(chgcxt->cc_rri, false);
/*
--
2.52.0
[text/x-diff] 0002-Provide-the-executor-with-information-on-updated-col.patch (3.6K, ../../60966.1782914222@localhost/3-0002-Provide-the-executor-with-information-on-updated-col.patch)
download | inline diff:
From b6e397e7d9d81ab0d13f31cc236a0e04b6c2d5bd Mon Sep 17 00:00:00 2001
From: Antonin Houska <ah@cybertec.at>
Date: Wed, 1 Jul 2026 15:18:10 +0200
Subject: [PATCH 2/2] Provide the executor with information on updated columns.
When replaying data changes, REPACK calls ExecInsertIndexTuples() for each
INSERT and UPDATE. In the latter case, the function needs to determine the
value of the 'indexUnchanged' hint for the index AM. (The hint is always false
for INSERT.) Therefore, REPACK is supposed to specify which columns are
changed by the UPDATE.
So far, REPACK missed to specify the set of updated columns, so the
'indexUnchanged' hint could incorrectly evaluate to true. Although it should
not affect correctness, it can make the index AM use optimizations that are
not appropriate. Ideally, we should compute the set of updated columns for
each individual UPDATE, however the comparison of the old and new tuple might
add too much overhead.
This patch initializes the set as if all columns were updated. Thus
'indexUnchanged' always evaluates to false. This way the index AM never uses
the related optimizations. It might result in worse structure of the index,
however it seems better to not use the optimizations than to misuse them.
---
src/backend/commands/repack.c | 40 ++++++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index c9b0c047477..f37bb9a21af 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -62,6 +62,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"
@@ -3005,13 +3006,50 @@ static void
initialize_change_context(ChangeContext *chgcxt,
Relation relation, Oid ident_index_id)
{
+ Bitmapset *updatedCols = NULL;
+ RangeTblEntry *rte;
+ List *perminfos = NIL;
+ RTEPermissionInfo *perminfo;
+
chgcxt->cc_rel = relation;
/* Only initialize fields needed by ExecInsertIndexTuples(). */
chgcxt->cc_estate = CreateExecutorState();
+ /*
+ * Initialize updatedCols.
+ *
+ * The point is that ExecInsertIndexTuples() should not pass the
+ * indexUnchanged hint to the index AM unless there's a reason to do
+ * so. For simplicity, we consider all columns updated.
+ *
+ * XXX Should we spend more effort to compare the old and new tuple when
+ * replaying UPDATE, or at least exclude unchanged TOAST values, like we
+ * do in logicalrep_write_tuple()?
+ */
+ for (int i = 0; i < RelationGetDescr(relation)->natts; i++)
+ updatedCols = bms_add_member(updatedCols,
+ i + 1 - FirstLowInvalidHeapAttributeNumber);
+
+ /*
+ * In this case, 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);
+ /* Make updatedCols available to the executor functions. */
+ perminfo = getRTEPermissionInfo(perminfos, rte);
+ perminfo->updatedCols = updatedCols;
+
+ ExecInitRangeTable(chgcxt->cc_estate, list_make1(rte), perminfos,
+ bms_make_singleton(1));
+
chgcxt->cc_rri = makeNode(ResultRelInfo);
- InitResultRelInfo(chgcxt->cc_rri, relation, 0, NULL, 0);
+ InitResultRelInfo(chgcxt->cc_rri, relation, 1, NULL, 0);
ExecOpenIndices(chgcxt->cc_rri, false);
/*
--
2.52.0
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: REPACK CONCURRENTLY fails on tables with generated columns
2026-06-12 08:39 REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2026-06-22 11:12 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
2026-06-22 12:49 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2026-07-01 13:57 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
@ 2026-07-02 01:39 ` Ewan Young <kdbase.hack@gmail.com>
0 siblings, 0 replies; 14+ messages in thread
From: Ewan Young @ 2026-07-02 01:39 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; mihailnikalayeu@gmail.com; alvherre@kurilemu.de
On Wed, Jul 1, 2026 at 9:57 PM Antonin Houska <ah@cybertec.at> wrote:
>
> Ewan Young <kdbase.hack@gmail.com> wrote:
>
> > On Mon, Jun 22, 2026 at 7:12 PM Antonin Houska <ah@cybertec.at> wrote:
> > >
> > > 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.
> >
> > Agreed, thanks for the correction. Relying on the empty ExecGetUpdatedCols()
> > set was the weak point of my version -- it's an omission, not something to
> > build a second approximation on. Copying the pg_attrdef entries fixes the
> > inconsistency at the root, so let's go with your approach.
> >
> > 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.
>
> Thanks for checking. Here, 0002 tries to fix the problem of empty updatedCols,
> as I proposed above.
>
> 0001 fixes two minor coding issues that I found when writing 0002.
Both look good to me. 0001 is a clear improvement — makeNode() is the right
idiom, and NULL rather than 0 for the pointer argument.
For 0002, I agree with the approach: since indexUnchanged only feeds the btree
bottom-up-deletion / dedup heuristics, treating all columns as updated (so the
hint is always false) is the safe choice — better to forgo those optimizations
than to misuse them. I checked the mechanism against the current code and it
holds: with ri_RangeTableIndex = 1, ExecGetUpdatedCols() returns the full set,
so index_unchanged_by_update() always sees a changed key column.
+1 on both.
>
> --
> Antonin Houska
> Web: https://www.cybertec-postgresql.com
>
--
Regards,
Ewan Young
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: REPACK CONCURRENTLY fails on tables with generated columns
2026-06-12 08:39 REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2026-06-22 11:12 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
2026-06-22 12:49 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
@ 2026-07-03 11:12 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-07-03 14:20 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2026-07-03 17:26 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
1 sibling, 2 replies; 14+ messages in thread
From: Alvaro Herrera @ 2026-07-03 11:12 UTC (permalink / raw)
To: Ewan Young <kdbase.hack@gmail.com>; +Cc: Antonin Houska <ah@cybertec.at>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; mihailnikalayeu@gmail.com
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/
Attachments:
[text/x-diff] 0001-REPACK-CONCURRENTLY-Initialize-the-range-table-more-.patch (4.0K, ../../akeN0fZ_Dc4SSjN0@alvherre.pgsql/2-0001-REPACK-CONCURRENTLY-Initialize-the-range-table-more-.patch)
download | inline diff:
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
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: REPACK CONCURRENTLY fails on tables with generated columns
2026-06-12 08:39 REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2026-06-22 11:12 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
2026-06-22 12:49 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2026-07-03 11:12 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Alvaro Herrera <alvherre@kurilemu.de>
@ 2026-07-03 14:20 ` Ewan Young <kdbase.hack@gmail.com>
1 sibling, 0 replies; 14+ messages in thread
From: Ewan Young @ 2026-07-03 14:20 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@kurilemu.de>; +Cc: Antonin Houska <ah@cybertec.at>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; mihailnikalayeu@gmail.com
Thanks, Álvaro.
On Fri, Jul 3, 2026 at 7:12 PM Alvaro Herrera <alvherre@kurilemu.de> wrote:
>
> 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.
I read through the range-table change and it looks correct.
Agreed on skipping dropped columns — a dropped attribute can never be
an index key column, so its presence in updatedCols has no functional effect;
leaving it out is a pure clarity win.
Over-claiming "all columns updated" is the safe direction: it only
affects the btree
indexUnchanged / bottom-up-deletion hint, never correctness. And for the
concurrent-apply path losing that hint should rarely matter, so erring
this way looks right.
+1 to commit.
>
> --
> Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
--
Regards,
Ewan Young
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: REPACK CONCURRENTLY fails on tables with generated columns
2026-06-12 08:39 REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2026-06-22 11:12 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Antonin Houska <ah@cybertec.at>
2026-06-22 12:49 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2026-07-03 11:12 ` Re: REPACK CONCURRENTLY fails on tables with generated columns Alvaro Herrera <alvherre@kurilemu.de>
@ 2026-07-03 17:26 ` Antonin Houska <ah@cybertec.at>
1 sibling, 0 replies; 14+ messages in thread
From: Antonin Houska @ 2026-07-03 17:26 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@kurilemu.de>; +Cc: Ewan Young <kdbase.hack@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; mihailnikalayeu@gmail.com
Alvaro Herrera <alvherre@kurilemu.de> wrote:
> 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.
Good point, thanks.
> 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.
+1
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 14+ messages in thread
end of thread, other threads:[~2026-07-03 17:26 UTC | newest]
Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-06-12 08:39 REPACK CONCURRENTLY fails on tables with generated columns Ewan Young <kdbase.hack@gmail.com>
2026-06-12 09:52 ` Antonin Houska <ah@cybertec.at>
2026-06-12 10:25 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-06-12 11:01 ` Antonin Houska <ah@cybertec.at>
2026-06-12 11:40 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-06-14 03:56 ` Srinath Reddy Sadipiralla <srinath2133@gmail.com>
2026-06-16 02:02 ` Ewan Young <kdbase.hack@gmail.com>
2026-06-22 11:12 ` Antonin Houska <ah@cybertec.at>
2026-06-22 12:49 ` Ewan Young <kdbase.hack@gmail.com>
2026-07-01 13:57 ` Antonin Houska <ah@cybertec.at>
2026-07-02 01:39 ` Ewan Young <kdbase.hack@gmail.com>
2026-07-03 11:12 ` Alvaro Herrera <alvherre@kurilemu.de>
2026-07-03 14:20 ` Ewan Young <kdbase.hack@gmail.com>
2026-07-03 17:26 ` Antonin Houska <ah@cybertec.at>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox