agora inbox for pgsql-bugs@postgresql.org
help / color / mirror / Atom feedREPACK (CONCURRENTLY) doesn't handle invalid indexes
16+ messages / 7 participants
[nested] [flat]
* REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-07-21 10:22 Zsolt Parragi <zsolt.parragi@percona.com>
0 siblings, 1 reply; 16+ messages in thread
From: Zsolt Parragi @ 2026-07-21 10:22 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Hello,
While testing REPACK, I noticed that REPACK (CONCURRENTLY) fails with
invalid unique indexes, while the non-concurrent version handles them.
The attached patch mirrors this handling for CONCURRENTLY: constraints
won't be enforced for invalid indexes.
Attachments:
[application/octet-stream] 0001-Don-t-let-invalid-indexes-break-REPACK-CONCURRENTLY.patch (8.2K, ../../CAN4CZFO5A3YE0Dd-bn7eKrB20pECO3=U0wKg1z2rO=DxgWJJHQ@mail.gmail.com/2-0001-Don-t-let-invalid-indexes-break-REPACK-CONCURRENTLY.patch)
download | inline diff:
From 5ea70b746281344c877541d65e3d22505c9228c9 Mon Sep 17 00:00:00 2001
From: Zsolt Parragi <zsolt.parragi@percona.com>
Date: Mon, 20 Jul 2026 21:24:52 +0000
Subject: [PATCH] Don't let invalid indexes break REPACK (CONCURRENTLY)
build_new_indexes() copies every index of the table, and it built the
copy of an invalid one (e.g. left over from a failed CREATE INDEX
CONCURRENTLY) as a full index with its constraints enforced. When such
an index is unique and the live data has duplicates, the copy failed to
build and aborted the whole REPACK, with an error naming the internal
'*_repacknew' index. The non-concurrent path does not have this problem:
reindex_relation() rebuilds invalid indexes with constraint enforcement
suppressed.
Do the same here: build the copy of an invalid index as a plain
non-unique index and don't copy its constraints. The old index stays
invalid after the swap, as before.
---
contrib/test_decoding/expected/repack.out | 35 +++++++++++++++++++++++
contrib/test_decoding/sql/repack.sql | 16 +++++++++++
src/backend/catalog/index.c | 10 +++++--
src/backend/commands/indexcmds.c | 3 +-
src/backend/commands/repack.c | 14 +++++++--
src/include/catalog/index.h | 3 +-
6 files changed, 75 insertions(+), 6 deletions(-)
diff --git a/contrib/test_decoding/expected/repack.out b/contrib/test_decoding/expected/repack.out
index c4ff41be690..af1d10f4dfd 100644
--- a/contrib/test_decoding/expected/repack.out
+++ b/contrib/test_decoding/expected/repack.out
@@ -99,6 +99,41 @@ REPACK (CONCURRENTLY) repack_conc_replident;
ERROR: cannot execute REPACK (CONCURRENTLY) on relation "repack_conc_replident"
DETAIL: REPACK (CONCURRENTLY) does not support deferrable primary keys.
HINT: Use ALTER TABLE ... REPLICA IDENTITY USING INDEX to designate another index as replica identity.
+-- Invalid indexes (e.g. left over from a failed CREATE INDEX CONCURRENTLY)
+-- must not prevent the processing; their constraints are not enforced while
+-- rebuilding and they stay invalid.
+CREATE TABLE repack_conc_invidx (i int PRIMARY KEY, j int);
+INSERT INTO repack_conc_invidx VALUES (1, 1), (2, 1);
+CREATE UNIQUE INDEX CONCURRENTLY repack_conc_invidx_uq ON repack_conc_invidx (j);
+ERROR: could not create unique index "repack_conc_invidx_uq"
+DETAIL: Key (j)=(1) is duplicated.
+SELECT indexrelid::regclass, indisvalid FROM pg_index
+WHERE indrelid = 'repack_conc_invidx'::regclass ORDER BY indexrelid::regclass::text;
+ indexrelid | indisvalid
+-------------------------+------------
+ repack_conc_invidx_pkey | t
+ repack_conc_invidx_uq | f
+(2 rows)
+
+REPACK (CONCURRENTLY) repack_conc_invidx;
+SELECT indexrelid::regclass, indisvalid FROM pg_index
+WHERE indrelid = 'repack_conc_invidx'::regclass ORDER BY indexrelid::regclass::text;
+ indexrelid | indisvalid
+-------------------------+------------
+ repack_conc_invidx_pkey | t
+ repack_conc_invidx_uq | f
+(2 rows)
+
+SELECT * FROM repack_conc_invidx ORDER BY i;
+ i | j
+---+---
+ 1 | 1
+ 2 | 1
+(2 rows)
+
+-- the invalid index still must not enforce uniqueness
+INSERT INTO repack_conc_invidx VALUES (3, 1);
+DROP TABLE repack_conc_invidx;
-- clean up
DROP TABLE repack_conc_replident, clstrpart;
-- verify that the pgrepack plugin cannot be called directly
diff --git a/contrib/test_decoding/sql/repack.sql b/contrib/test_decoding/sql/repack.sql
index f461f5479f4..db51c600629 100644
--- a/contrib/test_decoding/sql/repack.sql
+++ b/contrib/test_decoding/sql/repack.sql
@@ -73,6 +73,22 @@ REPACK (CONCURRENTLY) repack_conc_replident;
ALTER TABLE repack_conc_replident ADD PRIMARY KEY (i) DEFERRABLE;
REPACK (CONCURRENTLY) repack_conc_replident;
+-- Invalid indexes (e.g. left over from a failed CREATE INDEX CONCURRENTLY)
+-- must not prevent the processing; their constraints are not enforced while
+-- rebuilding and they stay invalid.
+CREATE TABLE repack_conc_invidx (i int PRIMARY KEY, j int);
+INSERT INTO repack_conc_invidx VALUES (1, 1), (2, 1);
+CREATE UNIQUE INDEX CONCURRENTLY repack_conc_invidx_uq ON repack_conc_invidx (j);
+SELECT indexrelid::regclass, indisvalid FROM pg_index
+WHERE indrelid = 'repack_conc_invidx'::regclass ORDER BY indexrelid::regclass::text;
+REPACK (CONCURRENTLY) repack_conc_invidx;
+SELECT indexrelid::regclass, indisvalid FROM pg_index
+WHERE indrelid = 'repack_conc_invidx'::regclass ORDER BY indexrelid::regclass::text;
+SELECT * FROM repack_conc_invidx ORDER BY i;
+-- the invalid index still must not enforce uniqueness
+INSERT INTO repack_conc_invidx VALUES (3, 1);
+DROP TABLE repack_conc_invidx;
+
-- clean up
DROP TABLE repack_conc_replident, clstrpart;
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 81bba4beac7..a93f01010c5 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1301,10 +1301,16 @@ index_create(Relation heapRelation,
* index_create.
*
* "tablespaceOid" is the tablespace to use for this index.
+ *
+ * If "skip_constraint_checks" is true, the copy is created as a plain
+ * non-unique index, so that neither the build nor later insertions enforce
+ * uniqueness. Useful when the old index is invalid and its constraint
+ * therefore cannot be assumed to hold.
*/
Oid
index_create_copy(Relation heapRelation, uint16 flags,
- Oid oldIndexId, Oid tablespaceOid, const char *newName)
+ Oid oldIndexId, Oid tablespaceOid, const char *newName,
+ bool skip_constraint_checks)
{
Relation indexRelation;
IndexInfo *oldInfo,
@@ -1397,7 +1403,7 @@ index_create_copy(Relation heapRelation, uint16 flags,
oldInfo->ii_Am,
indexExprs,
indexPreds,
- oldInfo->ii_Unique,
+ oldInfo->ii_Unique && !skip_constraint_checks,
oldInfo->ii_NullsNotDistinct,
!concurrently, /* isready */
concurrently, /* concurrent */
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 713bb5d10f1..70da622cb2f 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -4120,7 +4120,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
INDEX_CREATE_SUPPRESS_PROGRESS,
idx->indexId,
tablespaceid,
- concurrentName);
+ concurrentName,
+ false);
/*
* Now open the relation of the new index, a session-level lock is
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index dde56fb1e8d..697bc4a3c70 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -3406,9 +3406,18 @@ build_new_indexes(Relation NewHeap, Relation OldHeap, List *OldIndexes)
Oid newindex;
char *newName;
Relation ind;
+ bool isvalid;
ind = index_open(oldindex, ShareUpdateExclusiveLock);
+ /*
+ * An invalid index (e.g. left over from a failed CREATE INDEX
+ * CONCURRENTLY) may contradict its own constraints, so build its copy
+ * without enforcing them, like reindex_relation() does in the
+ * non-concurrent case. The old index stays invalid after the swap.
+ */
+ isvalid = ind->rd_index->indisvalid;
+
newName = ChooseRelationName(get_rel_name(oldindex),
NULL,
"repacknew",
@@ -3416,8 +3425,9 @@ build_new_indexes(Relation NewHeap, Relation OldHeap, List *OldIndexes)
false);
newindex = index_create_copy(NewHeap, INDEX_CREATE_SUPPRESS_PROGRESS,
oldindex, ind->rd_rel->reltablespace,
- newName);
- copy_index_constraints(ind, newindex, RelationGetRelid(NewHeap));
+ newName, !isvalid);
+ if (isvalid)
+ copy_index_constraints(ind, newindex, RelationGetRelid(NewHeap));
result = lappend_oid(result, newindex);
index_close(ind, NoLock);
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 9aee8226347..499654769f3 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -104,7 +104,8 @@ extern Oid index_create(Relation heapRelation,
extern Oid index_create_copy(Relation heapRelation, uint16 flags,
Oid oldIndexId, Oid tablespaceOid,
- const char *newName);
+ const char *newName,
+ bool skip_constraint_checks);
extern void index_concurrently_build(Oid heapRelationId,
Oid indexRelationId);
--
2.54.0
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-07-27 11:05 Álvaro Herrera <alvherre@kurilemu.de>
parent: Zsolt Parragi <zsolt.parragi@percona.com>
0 siblings, 1 reply; 16+ messages in thread
From: Álvaro Herrera @ 2026-07-27 11:05 UTC (permalink / raw)
To: Zsolt Parragi <zsolt.parragi@percona.com>; +Cc: pgsql-bugs@lists.postgresql.org
Hello,
On 2026-Jul-21, Zsolt Parragi wrote:
> While testing REPACK, I noticed that REPACK (CONCURRENTLY) fails with
> invalid unique indexes, while the non-concurrent version handles them.
Thanks for testing!
> The attached patch mirrors this handling for CONCURRENTLY: constraints
> won't be enforced for invalid indexes.
Wouldn't it make more sense to just ignore invalid indexes and not build
anything at all for them?
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-07-27 21:20 Zsolt Parragi <zsolt.parragi@percona.com>
parent: Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 16+ messages in thread
From: Zsolt Parragi @ 2026-07-27 21:20 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@kurilemu.de>; +Cc: pgsql-bugs@lists.postgresql.org
> Wouldn't it make more sense to just ignore invalid indexes and not build
> anything at all for them?
I followed what REPACK/VACUUM FULL does - it similarly rebuilds
invalid indexes, shouldn't we keep the two consistent?
And by skipping you mean that we should create an empty index instead
when repack sees an invalid index?
One argument for that is that the current code only handles unique
constraint violation, while there's a preexisting issue that these
commands all fail with other index failures, except REINDEX TABLE
CONCURRENTLY, e.g.:
CREATE TABLE t (i int PRIMARY KEY, j int);
INSERT INTO t VALUES (1, 0), (2, 1);
CREATE INDEX CONCURRENTLY t_expr ON t ((1/j));
VACUUM FULL t;
CLUSTER t USING t_pkey;
REINDEX TABLE t;
ALTER TABLE t ALTER COLUMN j TYPE bigint;
REINDEX TABLE CONCURRENTLY t; -- only this works
If we would skip all invalid indexes, that could solve this too
(except for the ALTER TABLE).
I wanted to raise this issue separately, as this seems to be
preexisting in earlier versions, not a new bug. (and a more complex
discussion than "repack and repack (concurrently) should behave
similarly")
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-07-28 06:16 Álvaro Herrera <alvherre@kurilemu.de>
parent: Zsolt Parragi <zsolt.parragi@percona.com>
0 siblings, 1 reply; 16+ messages in thread
From: Álvaro Herrera @ 2026-07-28 06:16 UTC (permalink / raw)
To: Zsolt Parragi <zsolt.parragi@percona.com>; +Cc: pgsql-bugs@lists.postgresql.org
On 2026-Jul-27, Zsolt Parragi wrote:
> > Wouldn't it make more sense to just ignore invalid indexes and not build
> > anything at all for them?
>
> I followed what REPACK/VACUUM FULL does - it similarly rebuilds
> invalid indexes, shouldn't we keep the two consistent?
I don't know. Maybe rebuilding invalid indexes is pointless. Don't you
think so? An invalid index can never be turned valid, so why spend
effort in building it at all?
> And by skipping you mean that we should create an empty index instead
> when repack sees an invalid index?
I mean we should just ignore all invalid indexes.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"¿Cómo puedes confiar en algo que pagas y que no ves,
y no confiar en algo que te dan y te lo muestran?" (Germán Poo)
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-07-28 09:37 Zsolt Parragi <zsolt.parragi@percona.com>
parent: Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 16+ messages in thread
From: Zsolt Parragi @ 2026-07-28 09:37 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@kurilemu.de>; +Cc: pgsql-bugs@lists.postgresql.org
> I don't know. Maybe rebuilding invalid indexes is pointless. Don't you
> think so? An invalid index can never be turned valid, so why spend
> effort in building it at all?
After a bit of testing: it isn't used by reads, but it is used by
uniqueness checks:
CREATE TABLE u (k int);
INSERT INTO u VALUES (1);
CREATE UNIQUE INDEX u_k_uq ON u(k);
SET allow_system_table_mods = on;
UPDATE pg_index SET indisvalid = false WHERE indexrelid = 'u_k_uq'::regclass;
RESET allow_system_table_mods;
INSERT INTO u VALUES (1); -- it exists, fails
INSERT INTO u VALUES (2); -- new row, succeeds, gets indexed
INSERT INTO u VALUES (2); -- it exists, fails
If we empty it, as I suggested, uniqueness checks will only work for
new records after repack, it will ignore everything preexisting.
If we leave it alone, we get completely bogus results and also
sporadic errors like:
ERROR: could not read blocks 344..344 in file "base/5/16401": read
only 0 of 8192 bytes
The leave it alone approach only works if indisready is also true,
which would be the case for my repro above, but not for some other
scenarios. I also have a repro which results in isvalid=false,
inready=true where we should rebuild the index.
I'll create a patch for that approach, but it is a bit more complex than v1.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-08-13 21:37 Zsolt Parragi <zsolt.parragi@percona.com>
parent: Zsolt Parragi <zsolt.parragi@percona.com>
0 siblings, 2 replies; 16+ messages in thread
From: Zsolt Parragi @ 2026-08-13 21:37 UTC (permalink / raw)
To: Álvaro Herrera <alvherre@kurilemu.de>; +Cc: pgsql-bugs@lists.postgresql.org
> I'll create a patch for that approach, but it is a bit more complex than v1.
It turned out not to be that complex, attached v2.
One thing to point out is that this is a slight behavior change for
REINDEX TABLE. We could avoid that, but the current behavior doesn't
seem to be documented anywhere, and this version seems more consistent
to me.
Attachments:
[application/octet-stream] v2-0001-Don-t-rebuild-invalid-indexes-that-are-not-ready-.patch (21.2K, ../../CAN4CZFMt0uYQPrB4Ey3nxVJZB1phf_iPz6Up6WW=ZW5WrYzvSw@mail.gmail.com/2-v2-0001-Don-t-rebuild-invalid-indexes-that-are-not-ready-.patch)
download | inline diff:
From 66cb287d74ad9a36864ec2c6a5e09671bdedbe59 Mon Sep 17 00:00:00 2001
From: Zsolt Parragi <zsolt.parragi@percona.com>
Date: Tue, 28 Jul 2026 20:55:09 +0000
Subject: [PATCH v2] Don't rebuild invalid indexes that are not ready for
inserts
reindex_relation() rebuilds every index of the table, including one that a
failed CREATE INDEX CONCURRENTLY left neither valid nor ready. Nothing
reads or maintains such an index, so the work is wasted, and it can fail
on data the index does not accept, taking the whole command down with it:
CREATE TABLE t (i int PRIMARY KEY, j int);
INSERT INTO t VALUES (1, 0), (2, 1);
CREATE INDEX CONCURRENTLY t_expr ON t ((1/j)); -- fails
VACUUM FULL t; -- ERROR: division by zero
Skip such an index instead, with the warning REINDEX TABLE CONCURRENTLY
already gives for it. REPACK (CONCURRENTLY) does the same by leaving the
index out of the ones it copies to the new heap.
An invalid index that is ready is still rebuilt: DML maintains it, so its
storage has to keep matching the heap. So is one that has to change
tablespace or persistence, else pg_class would not describe the storage
that is there.
REINDEX TABLE loses the undocumented property of repairing an index left
behind by a failed CREATE INDEX CONCURRENTLY. REINDEX INDEX remains the
way to do that.
---
contrib/test_decoding/expected/repack.out | 34 +++++++++++
contrib/test_decoding/sql/repack.sql | 16 +++++
src/backend/catalog/index.c | 28 +++++++++
src/backend/commands/repack.c | 32 +++++++++-
src/backend/utils/cache/lsyscache.c | 23 +++++++
src/include/utils/lsyscache.h | 1 +
src/test/modules/injection_points/Makefile | 2 +-
.../expected/index_invalid.out | 47 +++++++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../injection_points/sql/index_invalid.sql | 25 ++++++++
src/test/regress/expected/cluster.out | 60 +++++++++++++++++++
src/test/regress/expected/create_index.out | 29 ++++++++-
src/test/regress/sql/cluster.sql | 29 +++++++++
src/test/regress/sql/create_index.sql | 5 +-
14 files changed, 326 insertions(+), 6 deletions(-)
create mode 100644 src/test/modules/injection_points/expected/index_invalid.out
create mode 100644 src/test/modules/injection_points/sql/index_invalid.sql
diff --git a/contrib/test_decoding/expected/repack.out b/contrib/test_decoding/expected/repack.out
index 5ddc63238c5..1be2940cac1 100644
--- a/contrib/test_decoding/expected/repack.out
+++ b/contrib/test_decoding/expected/repack.out
@@ -99,6 +99,40 @@ REPACK (CONCURRENTLY) repack_conc_replident;
ERROR: cannot execute REPACK (CONCURRENTLY) on relation "repack_conc_replident"
DETAIL: REPACK (CONCURRENTLY) does not support deferrable primary keys.
HINT: Use ALTER TABLE ... REPLICA IDENTITY USING INDEX to designate another index as replica identity.
+-- An index that a failed CREATE INDEX CONCURRENTLY left neither valid nor
+-- ready is not copied to the new heap; copying this one would fail on the row
+-- its expression rejects.
+CREATE TABLE repack_conc_invalid (i int PRIMARY KEY, j int);
+INSERT INTO repack_conc_invalid VALUES (1, 0), (2, 1);
+CREATE INDEX CONCURRENTLY repack_conc_invalid_expr ON repack_conc_invalid ((1/j));
+ERROR: division by zero
+SELECT relfilenode AS invalid_expr_node FROM pg_class
+WHERE oid = 'repack_conc_invalid_expr'::regclass \gset
+REPACK (CONCURRENTLY) repack_conc_invalid;
+WARNING: skipping invalid index "public.repack_conc_invalid_expr"
+HINT: Use DROP INDEX or REINDEX INDEX.
+SELECT indisvalid, indisready FROM pg_index
+WHERE indexrelid = 'repack_conc_invalid_expr'::regclass;
+ indisvalid | indisready
+------------+------------
+ f | f
+(1 row)
+
+SELECT relfilenode = :invalid_expr_node FROM pg_class
+WHERE oid = 'repack_conc_invalid_expr'::regclass;
+ ?column?
+----------
+ t
+(1 row)
+
+SELECT * FROM repack_conc_invalid ORDER BY i;
+ i | j
+---+---
+ 1 | 0
+ 2 | 1
+(2 rows)
+
+DROP TABLE repack_conc_invalid;
-- clean up
DROP TABLE repack_conc_replident, clstrpart;
-- verify that the pgrepack plugin cannot be called directly
diff --git a/contrib/test_decoding/sql/repack.sql b/contrib/test_decoding/sql/repack.sql
index f461f5479f4..b7b3c936442 100644
--- a/contrib/test_decoding/sql/repack.sql
+++ b/contrib/test_decoding/sql/repack.sql
@@ -73,6 +73,22 @@ REPACK (CONCURRENTLY) repack_conc_replident;
ALTER TABLE repack_conc_replident ADD PRIMARY KEY (i) DEFERRABLE;
REPACK (CONCURRENTLY) repack_conc_replident;
+-- An index that a failed CREATE INDEX CONCURRENTLY left neither valid nor
+-- ready is not copied to the new heap; copying this one would fail on the row
+-- its expression rejects.
+CREATE TABLE repack_conc_invalid (i int PRIMARY KEY, j int);
+INSERT INTO repack_conc_invalid VALUES (1, 0), (2, 1);
+CREATE INDEX CONCURRENTLY repack_conc_invalid_expr ON repack_conc_invalid ((1/j));
+SELECT relfilenode AS invalid_expr_node FROM pg_class
+WHERE oid = 'repack_conc_invalid_expr'::regclass \gset
+REPACK (CONCURRENTLY) repack_conc_invalid;
+SELECT indisvalid, indisready FROM pg_index
+WHERE indexrelid = 'repack_conc_invalid_expr'::regclass;
+SELECT relfilenode = :invalid_expr_node FROM pg_class
+WHERE oid = 'repack_conc_invalid_expr'::regclass;
+SELECT * FROM repack_conc_invalid ORDER BY i;
+DROP TABLE repack_conc_invalid;
+
-- clean up
DROP TABLE repack_conc_replident, clstrpart;
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 4c5da7e5db0..7bad1eb3249 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -4094,6 +4094,34 @@ reindex_relation(const ReindexStmt *stmt, Oid relid, int flags,
Oid indexOid = lfirst_oid(indexId);
Oid indexNamespaceId = get_rel_namespace(indexOid);
+ /*
+ * Skip an index that is neither valid nor ready for inserts, such as
+ * one left behind by a failed CREATE INDEX CONCURRENTLY. Nothing
+ * reads such an index and DML does not maintain it, so its storage
+ * need not follow the heap, and rebuilding it can fail on data that
+ * the index does not accept, taking the whole command down.
+ *
+ * An invalid index that is ready has to be rebuilt: DML maintains it,
+ * so its storage must keep matching the heap. So has one whose
+ * storage moves to another tablespace or changes persistence, else
+ * pg_class would not describe the storage that is there.
+ */
+ if (!OidIsValid(params->tablespaceOid) &&
+ get_rel_persistence(indexOid) == persistence &&
+ !get_index_isvalid(indexOid) && !get_index_isready(indexOid))
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("skipping invalid index \"%s.%s\"",
+ get_namespace_name(indexNamespaceId),
+ get_rel_name(indexOid)),
+ errhint("Use DROP INDEX or REINDEX INDEX.")));
+
+ if (flags & REINDEX_REL_SUPPRESS_INDEX_USE)
+ RemoveReindexPending(indexOid);
+ continue;
+ }
+
/*
* Skip any invalid indexes on a TOAST table. These can only be
* duplicate leftovers from a failed REINDEX CONCURRENTLY, and if
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index dde56fb1e8d..a3a75fa316d 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -203,6 +203,7 @@ static void rebuild_relation_finish_concurrent(Relation NewHeap, Relation OldHea
Oid identIdx,
TransactionId frozenXid,
MultiXactId cutoffMulti);
+static List *filter_indexes_to_rebuild(Relation OldHeap);
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);
@@ -3185,7 +3186,7 @@ rebuild_relation_finish_concurrent(Relation NewHeap, Relation OldHeap,
List *ind_oids_new;
Oid old_table_oid = RelationGetRelid(OldHeap);
Oid new_table_oid = RelationGetRelid(NewHeap);
- List *ind_oids_old = RelationGetIndexList(OldHeap);
+ List *ind_oids_old = filter_indexes_to_rebuild(OldHeap);
ListCell *lc,
*lc2;
char relpersistence;
@@ -3383,6 +3384,35 @@ rebuild_relation_finish_concurrent(Relation NewHeap, Relation OldHeap,
relpersistence);
}
+/*
+ * Return the indexes to copy over to the new heap, that is, all of them except
+ * the ones that are neither valid nor ready for inserts. See the matching
+ * comment in reindex_relation().
+ */
+static List *
+filter_indexes_to_rebuild(Relation OldHeap)
+{
+ List *result = NIL;
+
+ foreach_oid(indexoid, RelationGetIndexList(OldHeap))
+ {
+ if (!get_index_isvalid(indexoid) && !get_index_isready(indexoid))
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("skipping invalid index \"%s.%s\"",
+ get_namespace_name(get_rel_namespace(indexoid)),
+ get_rel_name(indexoid)),
+ errhint("Use DROP INDEX or REINDEX INDEX.")));
+ continue;
+ }
+
+ result = lappend_oid(result, indexoid);
+ }
+
+ return result;
+}
+
/*
* Build indexes on NewHeap according to those on OldHeap.
*
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index cc6f05a0aa7..c8e114fb7a8 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -3953,6 +3953,29 @@ get_index_isvalid(Oid index_oid)
return isvalid;
}
+/*
+ * get_index_isready
+ *
+ * Given the index OID, return pg_index.indisready.
+ */
+bool
+get_index_isready(Oid index_oid)
+{
+ bool isready;
+ HeapTuple tuple;
+ Form_pg_index rd_index;
+
+ tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(index_oid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for index %u", index_oid);
+
+ rd_index = (Form_pg_index) GETSTRUCT(tuple);
+ isready = rd_index->indisready;
+ ReleaseSysCache(tuple);
+
+ return isready;
+}
+
/*
* get_index_isclustered
*
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index 865980cb0f1..f3c6610c4f1 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -211,6 +211,7 @@ extern Oid get_multirange_range(Oid multirangeOid);
extern Oid get_index_column_opclass(Oid index_oid, int attno);
extern bool get_index_isreplident(Oid index_oid);
extern bool get_index_isvalid(Oid index_oid);
+extern bool get_index_isready(Oid index_oid);
extern bool get_index_isclustered(Oid index_oid);
extern Oid get_publication_oid(const char *pubname, bool missing_ok);
extern char *get_publication_name(Oid pubid, bool missing_ok);
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 25a3ddd890d..f1d480f0157 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -9,7 +9,7 @@ EXTENSION = injection_points
DATA = injection_points--1.0.sql
PGFILEDESC = "injection_points - facility for injection points"
-REGRESS = injection_points hashagg reindex_conc vacuum
+REGRESS = injection_points hashagg index_invalid reindex_conc vacuum
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
ISOLATION = basic \
diff --git a/src/test/modules/injection_points/expected/index_invalid.out b/src/test/modules/injection_points/expected/index_invalid.out
new file mode 100644
index 00000000000..4c0dfd63606
--- /dev/null
+++ b/src/test/modules/injection_points/expected/index_invalid.out
@@ -0,0 +1,47 @@
+-- Tests for how a heap rewrite treats an invalid index
+CREATE EXTENSION injection_points;
+SELECT injection_points_set_local();
+ injection_points_set_local
+----------------------------
+
+(1 row)
+
+-- A CREATE INDEX CONCURRENTLY that fails after the index became ready for
+-- inserts leaves it invalid, but maintained by DML, so a rewrite has to
+-- rebuild it.
+SELECT injection_points_attach('define-index-before-set-valid', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+CREATE TABLE index_inj_tbl (i int);
+INSERT INTO index_inj_tbl VALUES (1), (2);
+CREATE INDEX CONCURRENTLY index_inj_idx ON index_inj_tbl (i);
+ERROR: error triggered for injection point define-index-before-set-valid
+SELECT injection_points_detach('define-index-before-set-valid');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+SELECT indisvalid, indisready FROM pg_index
+WHERE indexrelid = 'index_inj_idx'::regclass;
+ indisvalid | indisready
+------------+------------
+ f | t
+(1 row)
+
+SELECT relfilenode AS inj_idx_node FROM pg_class
+WHERE oid = 'index_inj_idx'::regclass \gset
+VACUUM FULL index_inj_tbl;
+SELECT relfilenode = :inj_idx_node FROM pg_class
+WHERE oid = 'index_inj_idx'::regclass;
+ ?column?
+----------
+ f
+(1 row)
+
+-- Cleanup
+DROP TABLE index_inj_tbl;
+DROP EXTENSION injection_points;
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index aaf0536ba7e..9a5ea5ca4ee 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -34,6 +34,7 @@ tests += {
'sql': [
'injection_points',
'hashagg',
+ 'index_invalid',
'reindex_conc',
'vacuum',
],
diff --git a/src/test/modules/injection_points/sql/index_invalid.sql b/src/test/modules/injection_points/sql/index_invalid.sql
new file mode 100644
index 00000000000..777291d4926
--- /dev/null
+++ b/src/test/modules/injection_points/sql/index_invalid.sql
@@ -0,0 +1,25 @@
+-- Tests for how a heap rewrite treats an invalid index
+CREATE EXTENSION injection_points;
+
+SELECT injection_points_set_local();
+
+-- A CREATE INDEX CONCURRENTLY that fails after the index became ready for
+-- inserts leaves it invalid, but maintained by DML, so a rewrite has to
+-- rebuild it.
+SELECT injection_points_attach('define-index-before-set-valid', 'error');
+CREATE TABLE index_inj_tbl (i int);
+INSERT INTO index_inj_tbl VALUES (1), (2);
+CREATE INDEX CONCURRENTLY index_inj_idx ON index_inj_tbl (i);
+SELECT injection_points_detach('define-index-before-set-valid');
+SELECT indisvalid, indisready FROM pg_index
+WHERE indexrelid = 'index_inj_idx'::regclass;
+SELECT relfilenode AS inj_idx_node FROM pg_class
+WHERE oid = 'index_inj_idx'::regclass \gset
+VACUUM FULL index_inj_tbl;
+SELECT relfilenode = :inj_idx_node FROM pg_class
+WHERE oid = 'index_inj_idx'::regclass;
+
+-- Cleanup
+DROP TABLE index_inj_tbl;
+
+DROP EXTENSION injection_points;
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index d1bc8a13286..fc5257913d9 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -829,6 +829,66 @@ ORDER BY o.relname;
clstr_3
(2 rows)
+-- An index that a failed CREATE INDEX CONCURRENTLY left neither valid nor
+-- ready is skipped by a rewrite; rebuilding this one would fail on the row
+-- its expression rejects.
+CREATE TABLE clstr_invalid (i int PRIMARY KEY, j int);
+INSERT INTO clstr_invalid VALUES (1, 0), (2, 1);
+CREATE INDEX CONCURRENTLY clstr_invalid_expr ON clstr_invalid ((1/j));
+ERROR: division by zero
+SELECT indisvalid, indisready FROM pg_index
+WHERE indexrelid = 'clstr_invalid_expr'::regclass;
+ indisvalid | indisready
+------------+------------
+ f | f
+(1 row)
+
+SELECT relfilenode AS invalid_expr_node FROM pg_class
+WHERE oid = 'clstr_invalid_expr'::regclass \gset
+VACUUM FULL clstr_invalid;
+WARNING: skipping invalid index "public.clstr_invalid_expr"
+HINT: Use DROP INDEX or REINDEX INDEX.
+CLUSTER clstr_invalid USING clstr_invalid_pkey;
+WARNING: skipping invalid index "public.clstr_invalid_expr"
+HINT: Use DROP INDEX or REINDEX INDEX.
+REPACK clstr_invalid;
+WARNING: skipping invalid index "public.clstr_invalid_expr"
+HINT: Use DROP INDEX or REINDEX INDEX.
+ALTER TABLE clstr_invalid ALTER COLUMN i TYPE bigint;
+WARNING: skipping invalid index "public.clstr_invalid_expr"
+HINT: Use DROP INDEX or REINDEX INDEX.
+-- the index storage is untouched, and the rows are still there
+SELECT relfilenode = :invalid_expr_node FROM pg_class
+WHERE oid = 'clstr_invalid_expr'::regclass;
+ ?column?
+----------
+ t
+(1 row)
+
+SELECT * FROM clstr_invalid ORDER BY i;
+ i | j
+---+---
+ 1 | 0
+ 2 | 1
+(2 rows)
+
+-- changing persistence has to rebuild the index, which fails here
+ALTER TABLE clstr_invalid SET UNLOGGED;
+ERROR: division by zero
+-- REINDEX INDEX still rebuilds it, and fails on the same row
+REINDEX INDEX clstr_invalid_expr;
+ERROR: division by zero
+-- once that row is gone it makes the index valid again
+DELETE FROM clstr_invalid WHERE j = 0;
+REINDEX INDEX clstr_invalid_expr;
+SELECT indisvalid, indisready FROM pg_index
+WHERE indexrelid = 'clstr_invalid_expr'::regclass;
+ indisvalid | indisready
+------------+------------
+ t | t
+(1 row)
+
+DROP TABLE clstr_invalid;
-- clean up
DROP TABLE clustertest;
DROP TABLE clstr_1;
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 7b2640f0e04..3505ba45f9d 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -1473,13 +1473,18 @@ DROP FUNCTION predicate_stable();
BEGIN;
CREATE INDEX std_index on concur_heap(f2);
COMMIT;
--- Failed builds are left invalid by VACUUM FULL, fixed by REINDEX
+-- Failed builds are left invalid by VACUUM FULL and by REINDEX TABLE,
+-- fixed by REINDEX INDEX
VACUUM FULL concur_heap;
+WARNING: skipping invalid index "public.concur_index3"
+HINT: Use DROP INDEX or REINDEX INDEX.
REINDEX TABLE concur_heap;
-ERROR: could not create unique index "concur_index3"
-DETAIL: Key (f2)=(b) is duplicated.
+WARNING: skipping invalid index "public.concur_index3"
+HINT: Use DROP INDEX or REINDEX INDEX.
DELETE FROM concur_heap WHERE f1 = 'b';
VACUUM FULL concur_heap;
+WARNING: skipping invalid index "public.concur_index3"
+HINT: Use DROP INDEX or REINDEX INDEX.
\d concur_heap
Table "public.concur_heap"
Column | Type | Collation | Nullable | Default
@@ -1496,6 +1501,24 @@ Indexes:
"std_index" btree (f2)
REINDEX TABLE concur_heap;
+WARNING: skipping invalid index "public.concur_index3"
+HINT: Use DROP INDEX or REINDEX INDEX.
+\d concur_heap
+ Table "public.concur_heap"
+ Column | Type | Collation | Nullable | Default
+--------+------+-----------+----------+---------
+ f1 | text | | |
+ f2 | text | | |
+Indexes:
+ "concur_heap_f2_f1_idx" btree ((f2 || f1))
+ "concur_index1" btree (f2, f1)
+ "concur_index2" UNIQUE, btree (f1)
+ "concur_index3" UNIQUE, btree (f2) INVALID
+ "concur_index4" btree (f2) WHERE f1 = 'a'::text
+ "concur_index5" btree (f2) WHERE f1 = 'x'::text
+ "std_index" btree (f2)
+
+REINDEX INDEX concur_index3;
\d concur_heap
Table "public.concur_heap"
Column | Type | Collation | Nullable | Default
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index e7a62367adf..1f10199a784 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -411,6 +411,35 @@ JOIN relnodes_new n ON o.relname = n.relname
WHERE o.relfilenode <> n.relfilenode
ORDER BY o.relname;
+-- An index that a failed CREATE INDEX CONCURRENTLY left neither valid nor
+-- ready is skipped by a rewrite; rebuilding this one would fail on the row
+-- its expression rejects.
+CREATE TABLE clstr_invalid (i int PRIMARY KEY, j int);
+INSERT INTO clstr_invalid VALUES (1, 0), (2, 1);
+CREATE INDEX CONCURRENTLY clstr_invalid_expr ON clstr_invalid ((1/j));
+SELECT indisvalid, indisready FROM pg_index
+WHERE indexrelid = 'clstr_invalid_expr'::regclass;
+SELECT relfilenode AS invalid_expr_node FROM pg_class
+WHERE oid = 'clstr_invalid_expr'::regclass \gset
+VACUUM FULL clstr_invalid;
+CLUSTER clstr_invalid USING clstr_invalid_pkey;
+REPACK clstr_invalid;
+ALTER TABLE clstr_invalid ALTER COLUMN i TYPE bigint;
+-- the index storage is untouched, and the rows are still there
+SELECT relfilenode = :invalid_expr_node FROM pg_class
+WHERE oid = 'clstr_invalid_expr'::regclass;
+SELECT * FROM clstr_invalid ORDER BY i;
+-- changing persistence has to rebuild the index, which fails here
+ALTER TABLE clstr_invalid SET UNLOGGED;
+-- REINDEX INDEX still rebuilds it, and fails on the same row
+REINDEX INDEX clstr_invalid_expr;
+-- once that row is gone it makes the index valid again
+DELETE FROM clstr_invalid WHERE j = 0;
+REINDEX INDEX clstr_invalid_expr;
+SELECT indisvalid, indisready FROM pg_index
+WHERE indexrelid = 'clstr_invalid_expr'::regclass;
+DROP TABLE clstr_invalid;
+
-- clean up
DROP TABLE clustertest;
DROP TABLE clstr_1;
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index 88ca3c80875..c29db6e5a99 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -534,7 +534,8 @@ BEGIN;
CREATE INDEX std_index on concur_heap(f2);
COMMIT;
--- Failed builds are left invalid by VACUUM FULL, fixed by REINDEX
+-- Failed builds are left invalid by VACUUM FULL and by REINDEX TABLE,
+-- fixed by REINDEX INDEX
VACUUM FULL concur_heap;
REINDEX TABLE concur_heap;
DELETE FROM concur_heap WHERE f1 = 'b';
@@ -542,6 +543,8 @@ VACUUM FULL concur_heap;
\d concur_heap
REINDEX TABLE concur_heap;
\d concur_heap
+REINDEX INDEX concur_index3;
+\d concur_heap
-- Temporary tables with concurrent builds and on-commit actions
-- CONCURRENTLY used with CREATE INDEX and DROP INDEX is ignored.
--
2.54.0
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-08-25 19:48 Nathan Bossart <nathandbossart@gmail.com>
parent: Zsolt Parragi <zsolt.parragi@percona.com>
1 sibling, 0 replies; 16+ messages in thread
From: Nathan Bossart @ 2026-08-25 19:48 UTC (permalink / raw)
To: Zsolt Parragi <zsolt.parragi@percona.com>; +Cc: Álvaro Herrera <alvherre@kurilemu.de>; pgsql-bugs@lists.postgresql.org
Does this one deserve a mention on the open items wiki [0]?
[0] https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items
--
nathan
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-08-26 06:28 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Zsolt Parragi <zsolt.parragi@percona.com>
1 sibling, 1 reply; 16+ messages in thread
From: Kyotaro Horiguchi @ 2026-08-26 06:28 UTC (permalink / raw)
To: zsolt.parragi@percona.com; +Cc: alvherre@kurilemu.de; pgsql-bugs@lists.postgresql.org
Hello,
At Thu, 13 Aug 2026 22:37:25 +0100, Zsolt Parragi <zsolt.parragi@percona.com> wrote in
> > I'll create a patch for that approach, but it is a bit more complex than v1.
>
> It turned out not to be that complex, attached v2.
>
> One thing to point out is that this is a slight behavior change for
> REINDEX TABLE. We could avoid that, but the current behavior doesn't
> seem to be documented anywhere, and this version seems more consistent
> to me.
I think Alvaro's point about whether invalid indexes should be rebuilt
in the first place is worth considering further. In fact, I wonder
whether REPACK should accept a relation containing an invalid index at
all.
I understand that an invalid index with indisready set cannot simply
be ignored by normal DML. As I understand it, indisready represents an
intermediate state in a concurrent index build, allowing INSERT/UPDATE
maintenance to start before the index becomes available for
queries. Therefore, if a concurrent index build fails at that stage,
an index with indisready = true and indisvalid = false can be left
behind.
However, I think there is a distinction between DML having to continue
maintaining such an index according to indisready and a later,
unrelated DDL command rebuilding it as if it were a normal index.
An invalid index left behind by a failed concurrent index build cannot
be used for queries, and is normally either dropped or explicitly
rebuilt with REINDEX. The fact that indisready is true does not mean
that the index is valid for normal use; it can simply mean that
maintenance had already been enabled as part of the concurrent
operation before it failed.
For that reason, I am somewhat uncomfortable with REPACK implicitly
rebuilding such an index. REPACK is not a command for repairing
indexes, so wouldn't it be more natural to reject the operation if an
invalid index exists and require the user to DROP or REINDEX it first,
rather than trying to reproduce or repair that state as part of
REPACK?
I understand that non-concurrent REPACK and VACUUM FULL currently
rebuild invalid indexes. However, that seems to be a consequence of
rebuilding all indexes as part of the heap rewrite, and I am not sure
that this behavior should necessarily define the semantics for REPACK
CONCURRENTLY. In fact, Alvaro's point also makes me wonder about the
existing behavior itself. I wonder whether non-concurrent REPACK or
VACUUM FULL should implicitly rebuild such an index as a side effect
of an operation with a different purpose.
So rather than deciding which invalid indexes to rebuild based on
indisready, as in v2, perhaps we should first decide whether a
relation containing an invalid index should be considered a valid
input for REPACK at all.
Regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-08-26 07:31 Zsolt Parragi <zsolt.parragi@percona.com>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 2 replies; 16+ messages in thread
From: Zsolt Parragi @ 2026-08-26 07:31 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org; alvherre@kurilemu.de
> I think Alvaro's point about whether invalid indexes should be rebuilt
> in the first place is worth considering further. In fact, I wonder
> whether REPACK should accept a relation containing an invalid index at
> all.
Not rebuilding it / emptying it isn't really an option, as I showed an
example in my earlier emails, not rebuilding it results in bogus
checks and statements failing with file read errors, if we empty it it
results in additional constraint violations.
Not allowing these commands (consistently) to work on tables with
invalid indexes is an option, but then that should be consistent
across all similar commands, and it will be a behavior change for
normal vacuum too.
Actually after I looked at this again after Nathan's email yesterday,
I realized that even v2 causes a regression (or lets call it a
behavior change at least), most likely v1 is a better solution.
Consider the following scenario:
CREATE TABLE orders (id int PRIMARY KEY, price int);
INSERT INTO orders VALUES (1, 10), (2, 0), (3, 20);
-- currently fails with division by zero
CREATE INDEX CONCURRENTLY orders_margin ON orders ((100/price));
-- removing bad data
DELETE FROM orders WHERE price = 0;
-- repairs the index
VACUUM FULL orders;
or another less visible example is REFRESH MATERIALIZED VIEW:
CREATE MATERIALIZED VIEW mv AS SELECT * FROM src;
REFRESH MATERIALIZED VIEW mv; -- let's say this is a daily/hourly cron
job or something like that
CREATE INDEX CONCURRENTLY mv_margin ON mv ((100/price)); -- fails
DELETE FROM src WHERE price = 0;
REFRESH MATERIALIZED VIEW mv; -- index now works on master/v1, remains
invalid in v2
And if I follow your suggestion consistently across all commands about
treating it as an invalid input the last command should fail in both
scenarios.
v1 seems to be a better/less risky version to me, especially for 19.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-08-26 08:25 Ewan Young <kdbase.hack@gmail.com>
parent: Zsolt Parragi <zsolt.parragi@percona.com>
1 sibling, 0 replies; 16+ messages in thread
From: Ewan Young @ 2026-08-26 08:25 UTC (permalink / raw)
To: Zsolt Parragi <zsolt.parragi@percona.com>; +Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; pgsql-bugs@lists.postgresql.org; alvherre@kurilemu.de
Hi
On Wed, Aug 26, 2026 at 3:31 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote:
>
> > I think Alvaro's point about whether invalid indexes should be rebuilt
> > in the first place is worth considering further. In fact, I wonder
> > whether REPACK should accept a relation containing an invalid index at
> > all.
>
> Not rebuilding it / emptying it isn't really an option, as I showed an
> example in my earlier emails, not rebuilding it results in bogus
> checks and statements failing with file read errors, if we empty it it
> results in additional constraint violations.
>
> Not allowing these commands (consistently) to work on tables with
> invalid indexes is an option, but then that should be consistent
> across all similar commands, and it will be a behavior change for
> normal vacuum too.
>
> Actually after I looked at this again after Nathan's email yesterday,
> I realized that even v2 causes a regression (or lets call it a
> behavior change at least), most likely v1 is a better solution.
I tested v2 on master as of 2f4df67f5d0 (cassert build) and can confirm
both of your scenarios. The underlying rule is in reindex_index():
after a successful rebuild it marks an invalid index valid again unless
it had to skip a uniqueness check, and skipped_constraint is only set
for unique and exclusion indexes (index.c:3847). So today a heap
rewrite fully repairs any *non-unique* invalid index -- VACUUM FULL,
CLUSTER and TRUNCATE included -- and REFRESH MATERIALIZED VIEW, which
passes check_constraints = true to finish_heap_swap() (matview.c),
repairs unique ones as well. In both of your scenarios the index ends
up valid on an unpatched build and stays invalid with v2, with a
WARNING. So the behavior change in v2 is wider than its commit message
suggests; it is not limited to REINDEX TABLE.
While testing v2 I ran into two more reindex_relation() callers that
hadn't come up in the thread:
- TRUNCATE goes through reindex_relation() too (tablecmds.c,
"Reconstruct the indexes to match"), so with v2 every TRUNCATE of
such a table emits the WARNING and leaves the index storage alone,
although rebuilding an index over an empty heap cannot fail:
CREATE TABLE tr (i int, j int);
INSERT INTO tr VALUES (1, 0);
CREATE INDEX CONCURRENTLY tr_expr ON tr ((1/j)); -- fails
TRUNCATE tr;
WARNING: skipping invalid index "public.tr_expr"
HINT: Use DROP INDEX or REINDEX INDEX.
- The new check sits in front of the existing invalid-TOAST-index
check, so a toast index that a failed REINDEX CONCURRENTLY left
neither valid nor ready gets the generic message above instead of
"cannot reindex invalid index ... on TOAST table, skipping" -- and
the REINDEX INDEX half of the new hint fails on toast indexes:
REINDEX INDEX pg_toast.pg_toast_16515_index_ccnew;
ERROR: cannot reindex invalid index on TOAST table
So +1 to going with v1 for 19 -- it still applies cleanly to the current
master and fixes the original failure here.
>
> Consider the following scenario:
>
> CREATE TABLE orders (id int PRIMARY KEY, price int);
> INSERT INTO orders VALUES (1, 10), (2, 0), (3, 20);
> -- currently fails with division by zero
> CREATE INDEX CONCURRENTLY orders_margin ON orders ((100/price));
> -- removing bad data
> DELETE FROM orders WHERE price = 0;
> -- repairs the index
> VACUUM FULL orders;
>
> or another less visible example is REFRESH MATERIALIZED VIEW:
>
> CREATE MATERIALIZED VIEW mv AS SELECT * FROM src;
> REFRESH MATERIALIZED VIEW mv; -- let's say this is a daily/hourly cron
> job or something like that
> CREATE INDEX CONCURRENTLY mv_margin ON mv ((100/price)); -- fails
> DELETE FROM src WHERE price = 0;
> REFRESH MATERIALIZED VIEW mv; -- index now works on master/v1, remains
> invalid in v2
>
> And if I follow your suggestion consistently across all commands about
> treating it as an invalid input the last command should fail in both
> scenarios.
>
> v1 seems to be a better/less risky version to me, especially for 19.
>
>
--
Regards,
Ewan Young
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-08-31 21:34 Christophe Pettus <xof@thebuild.com>
parent: Zsolt Parragi <zsolt.parragi@percona.com>
1 sibling, 1 reply; 16+ messages in thread
From: Christophe Pettus @ 2026-08-31 21:34 UTC (permalink / raw)
To: Zsolt Parragi <zsolt.parragi@percona.com>; +Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; pgsql-bugs@lists.postgresql.org; alvherre@kurilemu.de
> On Aug 26, 2026, at 00:31, Zsolt Parragi <zsolt.parragi@percona.com> wrote:
> Not allowing these commands (consistently) to work on tables with
> invalid indexes is an option, but then that should be consistent
> across all similar commands, and it will be a behavior change for
> normal vacuum too.
I think your statement has its own answer embedded in it: It's acceptable for REPACK, a brand-new command, to have different behavior in the presence of invalid indexes specifically because it would be a behavior change if we pushed that change to other commands.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-08-31 23:08 Zsolt Parragi <zsolt.parragi@percona.com>
parent: Christophe Pettus <xof@thebuild.com>
0 siblings, 2 replies; 16+ messages in thread
From: Zsolt Parragi @ 2026-08-31 23:08 UTC (permalink / raw)
To: Christophe Pettus <xof@thebuild.com>; +Cc: pgsql-bugs@lists.postgresql.org, Kyotaro Horiguchi <horikyota.ntt@gmail.com>
On Mon, 31 Aug 2026, Christophe Pettus <xof@thebuild.com> wrote:
> I think your statement has its own answer embedded in it: It's acceptable for REPACK, a brand-new command, to have different behavior in the presence of invalid indexes specifically because it would be a behavior change if we pushed that change to other commands.
But that's not exactly what happens here: currently REPACK and REPACK
(CONCURRENTLY) behave differently in the presence of invalid indexes,
and that is definitely a bug. There's no issue with the non concurrent
REPACK, that behaves like the other commands do. Making REPACK
(CONCURRENTLY) handle indexes the same way as other commands currently
do resolves the difference, and doesn't cause any behavior change for
other commands.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-09-01 00:43 Christophe Pettus <xof@thebuild.com>
parent: Zsolt Parragi <zsolt.parragi@percona.com>
1 sibling, 1 reply; 16+ messages in thread
From: Christophe Pettus @ 2026-09-01 00:43 UTC (permalink / raw)
To: Zsolt Parragi <zsolt.parragi@percona.com>; +Cc: pgsql-bugs@lists.postgresql.org, Kyotaro Horiguchi <horikyota.ntt@gmail.com>
> On Aug 31, 2026, at 16:08, Zsolt Parragi <zsolt.parragi@percona.com> wrote:
>
> On Mon, 31 Aug 2026, Christophe Pettus <xof@thebuild.com> wrote:
>> I think your statement has its own answer embedded in it: It's acceptable for REPACK, a brand-new command, to have different behavior in the presence of invalid indexes specifically because it would be a behavior change if we pushed that change to other commands.
>
> But that's not exactly what happens here: currently REPACK and REPACK
> (CONCURRENTLY) behave differently in the presence of invalid indexes,
> and that is definitely a bug.
"Differently" as such isn't a bug. "Incorrectly" would be. If REPACK AND REPACK CONCURRENTLY simply refused to run on a table with invalid indexes, that's minimally intrusive and fixes the bug. I agree that the ideal situation is that they work tables with invalid indexes, but I don't consider it a bug if they just refused to.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-09-07 03:13 shihao zhong <zhong950419@gmail.com>
parent: Christophe Pettus <xof@thebuild.com>
0 siblings, 0 replies; 16+ messages in thread
From: shihao zhong @ 2026-09-07 03:13 UTC (permalink / raw)
To: Christophe Pettus <xof@thebuild.com>; +Cc: Zsolt Parragi <zsolt.parragi@percona.com>; pgsql-bugs@lists.postgresql.org, Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Hi Zsolt, and team,
Just a housekeeping note as commitfest manager: this thread had two
identical commitfest entries, #7039 and #7040, both created in the same
second -- almost certainly a double submit of the registration form.
I have withdrawn #7040 as a duplicate. The surviving entry is:
https://commitfest.postgresql.org/patch/7039/
Thanks,
Shihao
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-09-08 07:25 Álvaro Herrera <alvherre@kurilemu.de>
parent: Zsolt Parragi <zsolt.parragi@percona.com>
1 sibling, 1 reply; 16+ messages in thread
From: Álvaro Herrera @ 2026-09-08 07:25 UTC (permalink / raw)
To: Zsolt Parragi <zsolt.parragi@percona.com>; +Cc: Christophe Pettus <xof@thebuild.com>; pgsql-hackers@lists.postgresql.org, Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Moving thread to -hackers.
On 2026-Aug-31, Zsolt Parragi wrote:
> On Mon, 31 Aug 2026, Christophe Pettus <xof@thebuild.com> wrote:
> > I think your statement has its own answer embedded in it: It's
> > acceptable for REPACK, a brand-new command, to have different
> > behavior in the presence of invalid indexes specifically because it
> > would be a behavior change if we pushed that change to other
> > commands.
>
> But that's not exactly what happens here: currently REPACK and REPACK
> (CONCURRENTLY) behave differently in the presence of invalid indexes,
> and that is definitely a bug.
Yes, but I think the question is in which direction should we fix said
bug. My preference is to go for Kyotaro's suggestion: have both REPACK
and REPACK (CONCURRENTLY) raise an error with an invalid index, asking
the user to drop it.
Would anybody oppose that?
Maybe in pg20, barring complaints against this, we can propagate the
same behavior to CLUSTER and VACUUM FULL. (But that obviously need more
discussion.)
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: REPACK (CONCURRENTLY) doesn't handle invalid indexes
@ 2026-09-10 09:24 Álvaro Herrera <alvherre@kurilemu.de>
parent: Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 0 replies; 16+ messages in thread
From: Álvaro Herrera @ 2026-09-10 09:24 UTC (permalink / raw)
To: Zsolt Parragi <zsolt.parragi@percona.com>; +Cc: Christophe Pettus <xof@thebuild.com>; pgsql-hackers@lists.postgresql.org, Kyotaro Horiguchi <horikyota.ntt@gmail.com>
On 2026-Sep-08, Álvaro Herrera wrote:
> Yes, but I think the question is in which direction should we fix said
> bug. My preference is to go for Kyotaro's suggestion: have both REPACK
> and REPACK (CONCURRENTLY) raise an error with an invalid index, asking
> the user to drop it.
>
> Would anybody oppose that?
Concretely, something like this. (Hmm, I guess this should be noted in
repack.sgml as well.)
I don't want to touch the behavior of REINDEX, CLUSTER or VACUUM FULL in
pg19 at this stage, much less within the context of an "open item"; we
can discuss that for pg20 afterwards.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Uno puede defenderse de los ataques; contra los elogios se esta indefenso"
Attachments:
[text/x-diff] 0001-Fail-REPACK-in-presence-of-isready-indvalid-indexes.patch (0B, ../../aqJ0Qt-3teW-BhSN@alvherre.pgsql/2-0001-Fail-REPACK-in-presence-of-isready-indvalid-indexes.patch)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2026-09-10 09:24 UTC | newest]
Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-07-21 10:22 REPACK (CONCURRENTLY) doesn't handle invalid indexes Zsolt Parragi <zsolt.parragi@percona.com>
2026-07-27 11:05 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-07-27 21:20 ` Zsolt Parragi <zsolt.parragi@percona.com>
2026-07-28 06:16 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-07-28 09:37 ` Zsolt Parragi <zsolt.parragi@percona.com>
2026-08-13 21:37 ` Zsolt Parragi <zsolt.parragi@percona.com>
2026-08-25 19:48 ` Nathan Bossart <nathandbossart@gmail.com>
2026-08-26 06:28 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2026-08-26 07:31 ` Zsolt Parragi <zsolt.parragi@percona.com>
2026-08-26 08:25 ` Ewan Young <kdbase.hack@gmail.com>
2026-08-31 21:34 ` Christophe Pettus <xof@thebuild.com>
2026-08-31 23:08 ` Zsolt Parragi <zsolt.parragi@percona.com>
2026-09-01 00:43 ` Christophe Pettus <xof@thebuild.com>
2026-09-07 03:13 ` shihao zhong <zhong950419@gmail.com>
2026-09-08 07:25 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-09-10 09:24 ` Álvaro Herrera <alvherre@kurilemu.de>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox