From 66cb287d74ad9a36864ec2c6a5e09671bdedbe59 Mon Sep 17 00:00:00 2001 From: Zsolt Parragi 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