pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed[PATCH] Improve REPACK (CONCURRENTLY) error messages for unsupported configurations
9+ messages / 3 participants
[nested] [flat]
* [PATCH] Improve REPACK (CONCURRENTLY) error messages for unsupported configurations
@ 2026-05-27 03:06 Baji Shaik <baji.pgdev@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Baji Shaik @ 2026-05-27 03:06 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Álvaro Herrera <alvherre@kurilemu.de>
Hi,
While exploring the new REPACK (CONCURRENTLY) feature, I noticed
a few user-facing error paths that could be made more accurate.
Patch series of three:
0001 -- When wal_level < replica, REPACK (CONCURRENTLY) currently
surfaces generic "replication slots ... wal_level" error
from CheckSlotRequirements(), with a CONTEXT line referring
to an internal worker. Add an upfront check that reports a
REPACK-specific error.
0002 -- check_concurrent_repack_requirements() reports the same
generic "no identity index" error for several distinct
cases, two of which are misleading: REPLICA IDENTITY FULL
(which is set, but the hint says there is no identity), and
a deferrable PK as the only identity (skipped per commit
832e220d99a, but the hint suggests adding an index that
already exists). Distinguish these cases.
0003 -- Four ereport(ERROR) calls in the REPACK CONCURRENTLY code
path lack errcode() and default to ERRCODE_INTERNAL_ERROR.
Add appropriate errcodes; in particular, the
apply_concurrent_update/delete failures map cleanly to
ERRCODE_T_R_SERIALIZATION_FAILURE.
All three are error-path only; the success path is unchanged. Each
patch is independently committable. Detailed rationale is in the
individual commit messages.
Tested with `make check` (245/245 pass) and `make isolation/check`
(128/128 pass) on top of master; 0002 also updates
src/test/regress/expected/cluster.out to match the new deferrable-PK
message.
Thanks,
Baji Shaik
Attachments:
[application/octet-stream] 0003-Add-missing-errcode-to-REPACK-CONCURRENTLY-ereport-c.patch (2.9K, ../../CA+fm-ROdgh0rEVuXoViBk4TVgjodrN=MTR_RYuOuKLZ9voX4YA@mail.gmail.com/3-0003-Add-missing-errcode-to-REPACK-CONCURRENTLY-ereport-c.patch)
download | inline diff:
From 210ed20a78e09b8f21eef3f1c22e98082b4b30b2 Mon Sep 17 00:00:00 2001
From: Baji Shaik <baji.pgdev@gmail.com>
Date: Tue, 26 May 2026 21:36:03 -0500
Subject: [PATCH 3/3] Add missing errcode() to REPACK CONCURRENTLY ereport
calls
Four ereport(ERROR) calls in the REPACK CONCURRENTLY code path are
missing errcode(), causing them to default to ERRCODE_INTERNAL_ERROR
(XX000):
* apply_concurrent_update(): "failed to apply concurrent UPDATE"
* apply_concurrent_delete(): "failed to apply concurrent DELETE"
* decode_concurrent_changes(): "could not read WAL record"
* decode_concurrent_changes(): "waiting for WAL failed"
The first two can occur in practice when a concurrent transaction
modifies a tuple between the time it was decoded and the time REPACK
tries to apply the change. Use ERRCODE_T_R_SERIALIZATION_FAILURE
(40001) for these, matching the semantics of a serialization conflict.
Also include the relation name in the errmsg for context.
The third indicates WAL corruption; use ERRCODE_DATA_CORRUPTED. The
fourth is an internal failure; keep ERRCODE_INTERNAL_ERROR but make
it explicit.
---
src/backend/commands/repack.c | 8 ++++++--
src/backend/commands/repack_worker.c | 2 ++
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index e2d405c336f..fd1e21b5f9f 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -2707,7 +2707,9 @@ apply_concurrent_update(Relation rel, TupleTableSlot *spilled_tuple,
&tmfd, &lockmode, &update_indexes);
if (res != TM_Ok)
ereport(ERROR,
- errmsg("failed to apply concurrent UPDATE"));
+ errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
+ errmsg("failed to apply concurrent UPDATE on relation \"%s\"",
+ RelationGetRelationName(rel)));
if (update_indexes != TU_None)
{
@@ -2743,7 +2745,9 @@ apply_concurrent_delete(Relation rel, TupleTableSlot *slot)
if (res != TM_Ok)
ereport(ERROR,
- errmsg("failed to apply concurrent DELETE"));
+ errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
+ errmsg("failed to apply concurrent DELETE on relation \"%s\"",
+ RelationGetRelationName(rel)));
pgstat_progress_incr_param(PROGRESS_REPACK_HEAP_TUPLES_DELETED, 1);
}
diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
index b84041372b8..a6a93fa740b 100644
--- a/src/backend/commands/repack_worker.c
+++ b/src/backend/commands/repack_worker.c
@@ -432,6 +432,7 @@ decode_concurrent_changes(LogicalDecodingContext *ctx,
priv->end_of_wal = false;
else
ereport(ERROR,
+ errcode(ERRCODE_DATA_CORRUPTED),
errmsg("could not read WAL record"));
}
@@ -479,6 +480,7 @@ decode_concurrent_changes(LogicalDecodingContext *ctx,
if (res != WAIT_LSN_RESULT_SUCCESS &&
res != WAIT_LSN_RESULT_TIMEOUT)
ereport(ERROR,
+ errcode(ERRCODE_INTERNAL_ERROR),
errmsg("waiting for WAL failed"));
}
}
--
2.50.1 (Apple Git-155)
[application/octet-stream] 0002-Improve-REPACK-CONCURRENTLY-errors-for-unusable-iden.patch (3.9K, ../../CA+fm-ROdgh0rEVuXoViBk4TVgjodrN=MTR_RYuOuKLZ9voX4YA@mail.gmail.com/4-0002-Improve-REPACK-CONCURRENTLY-errors-for-unusable-iden.patch)
download | inline diff:
From 3d9151c368b16f8ec01dccb4081d4880511da057 Mon Sep 17 00:00:00 2001
From: Baji Shaik <baji.pgdev@gmail.com>
Date: Tue, 26 May 2026 21:36:03 -0500
Subject: [PATCH 2/3] Improve REPACK (CONCURRENTLY) errors for unusable
identity
In check_concurrent_repack_requirements(), GetRelationIdentityOrPK()
returns InvalidOid for several distinct reasons but the user sees the
same generic 'has no identity index' error in all of them. Two of
those cases give misleading guidance:
- REPLICA IDENTITY FULL is set, but the hint says the relation has
no identity index.
- The only primary key is DEFERRABLE (skipped per commit 832e220d99a),
but the hint suggests adding an index that already exists.
Distinguish these two cases with specific messages. The third case
(no identity at all) keeps the existing message.
The new branches use information already on the Relation struct
(relreplident, rd_ispkdeferrable, rd_pkindex).
---
src/backend/commands/repack.c | 29 +++++++++++++++++++++++++++
src/test/regress/expected/cluster.out | 5 +++--
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 3d5bd98ad5f..e2d405c336f 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -959,12 +959,41 @@ check_concurrent_repack_requirements(Relation rel, Oid *ident_idx_p)
*/
ident_idx = GetRelationIdentityOrPK(rel);
if (!OidIsValid(ident_idx))
+ {
+ /*
+ * GetRelationIdentityOrPK() returns InvalidOid in several distinct
+ * cases. Distinguish them so the user sees an actionable message:
+ *
+ * - REPLICA IDENTITY FULL is set: not yet supported.
+ * - The relation has a deferrable primary key as its only
+ * identity: REPACK ignores deferrable PKs (see commit
+ * 832e220d99a).
+ * - The relation has no identity index of any kind.
+ */
+ if (replident == REPLICA_IDENTITY_FULL)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot repack relation \"%s\"",
+ RelationGetRelationName(rel)),
+ errdetail("%s does not support tables with REPLICA IDENTITY FULL.",
+ "REPACK (CONCURRENTLY)"));
+
+ if (rel->rd_ispkdeferrable && OidIsValid(rel->rd_pkindex))
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot repack relation \"%s\"",
+ RelationGetRelationName(rel)),
+ errdetail("%s does not support a deferrable primary key as identity.",
+ "REPACK (CONCURRENTLY)"),
+ errhint("Use ALTER TABLE ... ALTER CONSTRAINT to make the primary key NOT DEFERRABLE, or use ALTER TABLE ... REPLICA IDENTITY USING INDEX to designate another index."));
+
ereport(ERROR,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("cannot process relation \"%s\"",
RelationGetRelationName(rel)),
errhint("Relation \"%s\" has no identity index.",
RelationGetRelationName(rel)));
+ }
*ident_idx_p = ident_idx;
}
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 504ac1a3131..48667547060 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -846,8 +846,9 @@ HINT: Relation "repack_conc_replident" has no identity index.
-- Doesn't support tables with deferrable primary keys
ALTER TABLE repack_conc_replident ADD PRIMARY KEY (i) DEFERRABLE;
REPACK (CONCURRENTLY) repack_conc_replident;
-ERROR: cannot process relation "repack_conc_replident"
-HINT: Relation "repack_conc_replident" has no identity index.
+ERROR: cannot repack relation "repack_conc_replident"
+DETAIL: REPACK (CONCURRENTLY) does not support a deferrable primary key as identity.
+HINT: Use ALTER TABLE ... ALTER CONSTRAINT to make the primary key NOT DEFERRABLE, or use ALTER TABLE ... REPLICA IDENTITY USING INDEX to designate another index.
-- clean up
DROP TABLE repack_conc_replident;
DROP TABLE clustertest;
--
2.50.1 (Apple Git-155)
[application/octet-stream] 0001-Improve-REPACK-CONCURRENTLY-error-when-wal_level-rep.patch (2.1K, ../../CA+fm-ROdgh0rEVuXoViBk4TVgjodrN=MTR_RYuOuKLZ9voX4YA@mail.gmail.com/5-0001-Improve-REPACK-CONCURRENTLY-error-when-wal_level-rep.patch)
download | inline diff:
From 824f87fd6626cde48c1873997a15ed3463231e50 Mon Sep 17 00:00:00 2001
From: Baji Shaik <baji.pgdev@gmail.com>
Date: Tue, 26 May 2026 21:35:37 -0500
Subject: [PATCH 1/3] Improve REPACK (CONCURRENTLY) error when wal_level <
replica
REPACK (CONCURRENTLY) uses logical decoding internally, so it requires
wal_level >= replica. When wal_level is too low, the precondition is
indirectly enforced by CheckSlotRequirements(), which reports a generic
error about replication slots and a CONTEXT line for an internal worker.
That is hard to make sense of for a user who issued a REPACK command.
Add an upfront check in check_concurrent_repack_requirements() so the
error is reported in REPACK's voice before the worker is launched:
ERROR: cannot repack relation "X"
DETAIL: REPACK (CONCURRENTLY) requires "wal_level" to be set to
"replica" or higher.
---
src/backend/commands/repack.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 22a1307b38d..3d5bd98ad5f 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -40,6 +40,7 @@
#include "access/toast_internals.h"
#include "access/transam.h"
#include "access/xact.h"
+#include "access/xlog.h"
#include "catalog/catalog.h"
#include "catalog/dependency.h"
#include "catalog/heap.h"
@@ -897,6 +898,18 @@ check_concurrent_repack_requirements(Relation rel, Oid *ident_idx_p)
replident;
Oid ident_idx;
+ /*
+ * REPACK (CONCURRENTLY) uses logical decoding to capture changes that
+ * occur during the rewrite, so it requires wal_level >= replica.
+ */
+ if (wal_level < WAL_LEVEL_REPLICA)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot repack relation \"%s\"",
+ RelationGetRelationName(rel)),
+ errdetail("%s requires \"wal_level\" to be set to \"replica\" or higher.",
+ "REPACK (CONCURRENTLY)"));
+
/* Data changes in system relations are not logically decoded. */
if (IsCatalogRelation(rel))
ereport(ERROR,
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Improve REPACK (CONCURRENTLY) error messages for unsupported configurations
@ 2026-05-27 06:50 Chao Li <li.evan.chao@gmail.com>
parent: Baji Shaik <baji.pgdev@gmail.com>
0 siblings, 3 replies; 9+ messages in thread
From: Chao Li @ 2026-05-27 06:50 UTC (permalink / raw)
To: Baji Shaik <baji.pgdev@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Álvaro Herrera <alvherre@kurilemu.de>
> On May 27, 2026, at 11:06, Baji Shaik <baji.pgdev@gmail.com> wrote:
>
> Hi,
>
> While exploring the new REPACK (CONCURRENTLY) feature, I noticed
> a few user-facing error paths that could be made more accurate.
> Patch series of three:
>
> 0001 -- When wal_level < replica, REPACK (CONCURRENTLY) currently
> surfaces generic "replication slots ... wal_level" error
> from CheckSlotRequirements(), with a CONTEXT line referring
> to an internal worker. Add an upfront check that reports a
> REPACK-specific error.
>
LGTM
> 0002 -- check_concurrent_repack_requirements() reports the same
> generic "no identity index" error for several distinct
> cases, two of which are misleading: REPLICA IDENTITY FULL
> (which is set, but the hint says there is no identity), and
> a deferrable PK as the only identity (skipped per commit
> 832e220d99a, but the hint suggests adding an index that
> already exists). Distinguish these cases.
>
When I was working on 832e220d99a, I actually considered for more detailed error messages, but I ended up giving up. I think we should be careful about adding more branches here unless the existing message is causing significant confusion in practice.
So, I personally don’t like 0002.
> 0003 -- Four ereport(ERROR) calls in the REPACK CONCURRENTLY code
> path lack errcode() and default to ERRCODE_INTERNAL_ERROR.
> Add appropriate errcodes; in particular, the
> apply_concurrent_update/delete failures map cleanly to
> ERRCODE_T_R_SERIALIZATION_FAILURE.
>
LGTM
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Improve REPACK (CONCURRENTLY) error messages for unsupported configurations
@ 2026-05-27 23:51 Baji Shaik <baji.pgdev@gmail.com>
parent: Chao Li <li.evan.chao@gmail.com>
2 siblings, 0 replies; 9+ messages in thread
From: Baji Shaik @ 2026-05-27 23:51 UTC (permalink / raw)
To: Chao Li <li.evan.chao@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Álvaro Herrera <alvherre@kurilemu.de>
Hi Chao,
Thanks for the quick review and feedback.
On Wed, May 27, 2026 at 1:50 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
> > On May 27, 2026, at 11:06, Baji Shaik <baji.pgdev@gmail.com> wrote:
>
> When I was working on 832e220d99a, I actually considered for more detailed
> error messages, but I ended up giving up. I think we should be careful
> about adding more branches here unless the existing message is causing
> significant confusion in practice.
>
> So, I personally don’t like 0002.
>
I hear the concern of adding more branches here. FWIW, neither the
deferrable PK limitation nor the REPLICA IDENTITY FULL case is
documented in repack.sgml, and the current error message doesn't
surface either issue. The user has no way to discover what's wrong
without reading the source. That's why I think the improved messages
are warranted here. Happy to add a documentation note as well, or
both. Let me know what's preferred.
But happy to drop 0002 if others feel the same. 0001 and 0003 stand on
their own.
Thanks,
Baji Shaik
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Improve REPACK (CONCURRENTLY) error messages for unsupported configurations
@ 2026-05-28 14:54 Álvaro Herrera <alvherre@kurilemu.de>
parent: Chao Li <li.evan.chao@gmail.com>
2 siblings, 0 replies; 9+ messages in thread
From: Álvaro Herrera @ 2026-05-28 14:54 UTC (permalink / raw)
To: Chao Li <li.evan.chao@gmail.com>; +Cc: Baji Shaik <baji.pgdev@gmail.com>; pgsql-hackers@lists.postgresql.org
On 2026-May-27, Chao Li wrote:
> > On May 27, 2026, at 11:06, Baji Shaik <baji.pgdev@gmail.com> wrote:
> >
> > 0001 -- When wal_level < replica, REPACK (CONCURRENTLY) currently
> > surfaces generic "replication slots ... wal_level" error
> > from CheckSlotRequirements(), with a CONTEXT line referring
> > to an internal worker. Add an upfront check that reports a
> > REPACK-specific error.
>
> LGTM
Pushed this one earlier. I changed the errcode though, because in my
mind "object" is a database object, and the server configuration is not
an object. So I used INVALID_PARAMETER_VALUE instead. I also don't
think it makes sense to say "cannot repack table X", so the user leaves
thinking they could repack table Y instead. The whole point being that
you cannot vacuum _any_ tables. So I made the errmsg() say that.
> When I was working on 832e220d99a, I actually considered for more
> detailed error messages, but I ended up giving up. I think we should
> be careful about adding more branches here unless the existing message
> is causing significant confusion in practice.
>
> So, I personally don’t like 0002.
I'll give this a look after some icecream.
> > 0003 -- Four ereport(ERROR) calls in the REPACK CONCURRENTLY code
> > path lack errcode() and default to ERRCODE_INTERNAL_ERROR.
> > Add appropriate errcodes; in particular, the
> > apply_concurrent_update/delete failures map cleanly to
> > ERRCODE_T_R_SERIALIZATION_FAILURE.
Also pushed, with additional editorialization. I have recollections of
out message policy saying something about "could not do X" instead of
"failed to do X", so I changed it that way. (But I couldn't find that
in the style guide.)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Improve REPACK (CONCURRENTLY) error messages for unsupported configurations
@ 2026-05-28 18:35 Álvaro Herrera <alvherre@kurilemu.de>
parent: Chao Li <li.evan.chao@gmail.com>
2 siblings, 2 replies; 9+ messages in thread
From: Álvaro Herrera @ 2026-05-28 18:35 UTC (permalink / raw)
To: Chao Li <li.evan.chao@gmail.com>; +Cc: Baji Shaik <baji.pgdev@gmail.com>; pgsql-hackers@lists.postgresql.org
On 2026-May-27, Chao Li wrote:
> > 0002 -- check_concurrent_repack_requirements() reports the same
> > generic "no identity index" error for several distinct
> > cases, two of which are misleading: REPLICA IDENTITY FULL
> > (which is set, but the hint says there is no identity), and
> > a deferrable PK as the only identity (skipped per commit
> > 832e220d99a, but the hint suggests adding an index that
> > already exists). Distinguish these cases.
>
> When I was working on 832e220d99a, I actually considered for more
> detailed error messages, but I ended up giving up. I think we should
> be careful about adding more branches here unless the existing message
> is causing significant confusion in practice.
I pushed this one too (well, something close to it anyway), because I
think the replica identity issue could be an (unnecessary) usability
tripwire.
I'm curious to know why you gave up on this, if you want to share more.
Thanks both,
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Voy a acabar con todos los humanos / con los humanos yo acabaré
voy a acabar con todos (bis) / con todos los humanos acabaré ¡acabaré! (Bender)
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Improve REPACK (CONCURRENTLY) error messages for unsupported configurations
@ 2026-05-28 21:18 Álvaro Herrera <alvherre@kurilemu.de>
parent: Álvaro Herrera <alvherre@kurilemu.de>
1 sibling, 1 reply; 9+ messages in thread
From: Álvaro Herrera @ 2026-05-28 21:18 UTC (permalink / raw)
To: Chao Li <li.evan.chao@gmail.com>; +Cc: Baji Shaik <baji.pgdev@gmail.com>; pgsql-hackers@lists.postgresql.org
While looking these patches over I noticed that we still have some error
reports cases uncovered. Here's a quick attempt to try and complete
that.
After this patch I see only one uncovered error path, the one that
prevents repacking a temp table of another session. That would require
an isolation test. Not sure it's worth the trouble ... (There's a
bunch of uncovered "elog(ERROR)" cases, but those are mostly just
can't-happen conditions, as I understand).
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Attachments:
[text/x-diff] 0001-Cover-some-errors-and-corner-conditions-in-repack.c.patch (4.0K, ../../ahiwD29RNfVT4tjQ@alvherre.pgsql/2-0001-Cover-some-errors-and-corner-conditions-in-repack.c.patch)
download | inline diff:
From 7eab30e74692ac023ed20bfc85d92e68f0d9db02 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Thu, 28 May 2026 11:20:19 +0200
Subject: [PATCH] Cover some errors and corner conditions in repack.c
---
src/test/regress/expected/cluster.out | 33 +++++++++++++++++++++++++++
src/test/regress/sql/cluster.sql | 28 +++++++++++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index 23f312c62a3..cfbe2764427 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -697,6 +697,39 @@ SELECT * FROM clstr_expression WHERE -a = -3 ORDER BY -a, b;
(4 rows)
COMMIT;
+-- verify some error cases
+CREATE TABLE clstr_table_one (id int, val text);
+CREATE TABLE clstr_table_two (id int, val text);
+CREATE INDEX clstr_idx_b ON clstr_table_two (id);
+CLUSTER clstr_table_one USING clstr_idx_b;
+ERROR: "clstr_idx_b" is not an index for table "clstr_table_one"
+CLUSTER clstr_table_one USING nonexistant;
+ERROR: index "nonexistant" for table "clstr_table_one" does not exist
+CREATE INDEX clstr_hash_idx ON clstr_table_one USING hash (id);
+CLUSTER clstr_table_one USING clstr_hash_idx;
+ERROR: cannot cluster on index "clstr_hash_idx" because access method does not support clustering
+CREATE INDEX clstr_partial_idx ON clstr_table_one (id) WHERE id > 0;
+CLUSTER clstr_table_one USING clstr_partial_idx;
+ERROR: cannot cluster on partial index "clstr_partial_idx"
+REPACK pg_class USING INDEX pg_class_oid_index;
+ERROR: permission denied: "pg_class" is a system catalog
+DETAIL: System catalogs can only be clustered by the index they're already clustered on, if any, unless "allow_system_table_mods" is enabled.
+DROP TABLE clstr_table_one, clstr_table_two;
+-- verify that CLUSTER/REPACK don't touch a NO DATA matview
+CREATE MATERIALIZED VIEW clstr_matview AS
+ SELECT i FROM generate_series(1, 5) i
+ WITH NO DATA;
+CREATE INDEX clstr_matview_idx ON clstr_matview (i);
+SELECT relfilenode FROM pg_class WHERE oid = 'clstr_matview'::regclass \gset
+CLUSTER clstr_matview USING clstr_matview_idx;
+REPACK clstr_matview USING INDEX clstr_matview_idx;
+SELECT relfilenode = :relfilenode FROM pg_class WHERE oid = 'clstr_matview'::regclass;
+ ?column?
+----------
+ t
+(1 row)
+
+DROP MATERIALIZED VIEW clstr_matview;
----------------------------------------------------------------------
--
-- REPACK
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index c2f329ecd1b..1cb1942263c 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -328,6 +328,34 @@ EXPLAIN (COSTS OFF) SELECT * FROM clstr_expression WHERE -a = -3 ORDER BY -a, b;
SELECT * FROM clstr_expression WHERE -a = -3 ORDER BY -a, b;
COMMIT;
+-- verify some error cases
+CREATE TABLE clstr_table_one (id int, val text);
+CREATE TABLE clstr_table_two (id int, val text);
+CREATE INDEX clstr_idx_b ON clstr_table_two (id);
+CLUSTER clstr_table_one USING clstr_idx_b;
+CLUSTER clstr_table_one USING nonexistant;
+
+CREATE INDEX clstr_hash_idx ON clstr_table_one USING hash (id);
+CLUSTER clstr_table_one USING clstr_hash_idx;
+
+CREATE INDEX clstr_partial_idx ON clstr_table_one (id) WHERE id > 0;
+CLUSTER clstr_table_one USING clstr_partial_idx;
+
+REPACK pg_class USING INDEX pg_class_oid_index;
+
+DROP TABLE clstr_table_one, clstr_table_two;
+
+-- verify that CLUSTER/REPACK don't touch a NO DATA matview
+CREATE MATERIALIZED VIEW clstr_matview AS
+ SELECT i FROM generate_series(1, 5) i
+ WITH NO DATA;
+CREATE INDEX clstr_matview_idx ON clstr_matview (i);
+SELECT relfilenode FROM pg_class WHERE oid = 'clstr_matview'::regclass \gset
+CLUSTER clstr_matview USING clstr_matview_idx;
+REPACK clstr_matview USING INDEX clstr_matview_idx;
+SELECT relfilenode = :relfilenode FROM pg_class WHERE oid = 'clstr_matview'::regclass;
+DROP MATERIALIZED VIEW clstr_matview;
+
----------------------------------------------------------------------
--
-- REPACK
--
2.47.3
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Improve REPACK (CONCURRENTLY) error messages for unsupported configurations
@ 2026-05-28 22:08 Baji Shaik <baji.pgdev@gmail.com>
parent: Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 9+ messages in thread
From: Baji Shaik @ 2026-05-28 22:08 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@kurilemu.de>; +Cc: Chao Li <li.evan.chao@gmail.com>; pgsql-hackers@lists.postgresql.org
On Thu, May 28, 2026 at 4:18 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> While looking these patches over I noticed that we still have some error
> reports cases uncovered. Here's a quick attempt to try and complete
> that.
>
> After this patch I see only one uncovered error path, the one that
> prevents repacking a temp table of another session. That would require
> an isolation test. Not sure it's worth the trouble ... (There's a
> bunch of uncovered "elog(ERROR)" cases, but those are mostly just
> can't-happen conditions, as I understand).
>
LGTM, thanks for adding these.
FWIW I see a couple more uncovered ereport(ERROR) paths 1/ invalid
index (line 813) 2/ shared catalog with USING INDEX (line 580,
needs allow_system_table_mods). But both need unusual setup, so
fine to leave them along with the temp table one.
Thanks,
Baji Shaik.
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Improve REPACK (CONCURRENTLY) error messages for unsupported configurations
@ 2026-05-29 00:42 Chao Li <li.evan.chao@gmail.com>
parent: Álvaro Herrera <alvherre@kurilemu.de>
1 sibling, 0 replies; 9+ messages in thread
From: Chao Li @ 2026-05-29 00:42 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@kurilemu.de>; +Cc: Baji Shaik <baji.pgdev@gmail.com>; pgsql-hackers@lists.postgresql.org
> On May 29, 2026, at 02:35, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2026-May-27, Chao Li wrote:
>
>>> 0002 -- check_concurrent_repack_requirements() reports the same
>>> generic "no identity index" error for several distinct
>>> cases, two of which are misleading: REPLICA IDENTITY FULL
>>> (which is set, but the hint says there is no identity), and
>>> a deferrable PK as the only identity (skipped per commit
>>> 832e220d99a, but the hint suggests adding an index that
>>> already exists). Distinguish these cases.
>>
>> When I was working on 832e220d99a, I actually considered for more
>> detailed error messages, but I ended up giving up. I think we should
>> be careful about adding more branches here unless the existing message
>> is causing significant confusion in practice.
>
> I pushed this one too (well, something close to it anyway), because I
> think the replica identity issue could be an (unnecessary) usability
> tripwire.
>
> I'm curious to know why you gave up on this, if you want to share more.
>
Sure. As the committed version is slightly different from the original patch, my comments below are still based on the original patch:
1. I am not against improving the error message in principle, but I'm worried about the maintainability of distinguishing all the possible reasons here.
GetRelationIdentityOrPK() currently returns InvalidOid for several cases, so the patch tries to infer the reason afterwards from relreplident, rd_ispkdeferrable, rd_pkindex, etc. That works for the current cases, but it also means that whenever the REPACK (CONCURRENTLY) requirements change, these extra checks may need to be revisited. For example, if FULL is supported in the future, the FULL specific error would need to be removed or changed.
2. I also think the hint for the deferrable primary key case may be too specific:
```
Use ALTER TABLE ... ALTER CONSTRAINT to make the primary key NOT DEFERRABLE, or use ALTER TABLE ... REPLICA IDENTITY USING INDEX to designate another index.
```
Those are useful suggestions in many cases, but they are not necessarily the only possible solutions. So I was not sure if we should provide such a specific hint.
3. Especially for the FULL check, I have a pending patch [1] that may potentially allow REPLICA_IDENTITY_DEFAULT to fall back to FULL. If something like that eventually happens, this kind of detailed check could become misleading. My general concern is that the more detailed we make these inferred errors, the more fragile they may become when future behavior changes.
BTW, for patch [1], this is not an imagined feature. The requirement came from real customer operations. So, Álvaro, if you could help there, I would greatly appreciate it. That is a long thread, so no rush.
[1] https://postgr.es/m/CAEoWx2mMorbMwjKbT4YCsjDyL3r9Mp+z0bbK57VZ+OkJTgJQVQ@mail.gmail.com
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: [PATCH] Improve REPACK (CONCURRENTLY) error messages for unsupported configurations
@ 2026-05-29 10:43 Álvaro Herrera <alvherre@kurilemu.de>
parent: Baji Shaik <baji.pgdev@gmail.com>
0 siblings, 0 replies; 9+ messages in thread
From: Álvaro Herrera @ 2026-05-29 10:43 UTC (permalink / raw)
To: Baji Shaik <baji.pgdev@gmail.com>; +Cc: Chao Li <li.evan.chao@gmail.com>; L. pgsql-hackers <pgsql-hackers@lists.postgresql.org>
On 2026-05-29, Baji Shaik wrote:
> LGTM, thanks for adding these.
>
> FWIW I see a couple more uncovered ereport(ERROR) paths 1/ invalid
> index (line 813) 2/ shared catalog with USING INDEX (line 580,
> needs allow_system_table_mods). But both need unusual setup, so
> fine to leave them along with the temp table one.
Yeah. I think we could still cover the one in line 580, maybe in test_decoding, but I didn't try. I'm not as excited about this one though; I'm ok to leave it as-is. Anyway, I have pushed the patch, together with moving some previous tests to test_decoding, because the error message added by your initial 0001 caused wal_level=minimal tests to fail, as shown by thorntail
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=thorntail&dt=2026-05-29%2006%3A31%3A03
Thanks!
--
Álvaro Herrera
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2026-05-29 10:43 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-05-27 03:06 [PATCH] Improve REPACK (CONCURRENTLY) error messages for unsupported configurations Baji Shaik <baji.pgdev@gmail.com>
2026-05-27 06:50 ` Chao Li <li.evan.chao@gmail.com>
2026-05-27 23:51 ` Baji Shaik <baji.pgdev@gmail.com>
2026-05-28 14:54 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-05-28 18:35 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-05-28 21:18 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-05-28 22:08 ` Baji Shaik <baji.pgdev@gmail.com>
2026-05-29 10:43 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-05-29 00:42 ` Chao Li <li.evan.chao@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox