Hello!
While working on [1], I have found a small issue with correctness
of set_indexsafe_procflags usage in ReindexRelationConcurrently introduced
in [2].
> idx->safe = (indexRel->rd_indexprs == NIL && indexRel->rd_indpred == NIL);
It is always true because there are no RelationGetIndexExpressions
and RelationGetIndexPredicate before that check.
Two patches with reproducer + fix are attached.
The issue is simple, but I'll register this in commitfest just in case.
Best regards,
Mikhail.
[1]:
https://www.postgresql.org/message-id/flat/CANtu0ogBOtd9ravu1CUbuZWgq6qvn1rny38PGKDPk9zzQPH8_A%40mail.gmail.com#d4be02ff70f3002522f9fadbd165d631
[2]:
https://github.com/postgres/postgres/commit/f9900df5f94936067e6fa24a9df609863eb08da2
From 7dec20b6211cc2dca02d8806cf0a120236a24517 Mon Sep 17 00:00:00 2001
From: nkey <n...@toloka.ai>
Date: Fri, 6 Sep 2024 13:14:57 +0200
Subject: [PATCH v1 1/2] specification to reproduce issue with incorrect usage
of set_indexsafe_procflags for REINDEX CONCURRENTLY
---
src/backend/commands/indexcmds.c | 3 +++
src/test/modules/injection_points/Makefile | 3 ++-
.../reindex_concurrently_safe_index.out | 9 ++++++++
src/test/modules/injection_points/meson.build | 1 +
.../reindex_concurrently_safe_index.spec | 23 +++++++++++++++++++
5 files changed, 38 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/injection_points/expected/reindex_concurrently_safe_index.out
create mode 100644 src/test/modules/injection_points/specs/reindex_concurrently_safe_index.spec
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index c5a56c75f6..e07d279e2d 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -69,6 +69,7 @@
#include "utils/regproc.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
+#include "utils/injection_point.h"
/* non-export function prototypes */
@@ -3784,6 +3785,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
/* determine safety of this index for set_indexsafe_procflags */
idx->safe = (indexRel->rd_indexprs == NIL &&
indexRel->rd_indpred == NIL);
+ if (idx->safe)
+ INJECTION_POINT("ReindexRelationConcurrently_index_safe");
idx->tableId = RelationGetRelid(heapRel);
idx->amId = indexRel->rd_rel->relam;
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 1c1c2d0b13..7d85e25d4b 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -13,7 +13,8 @@ PGFILEDESC = "injection_points - facility for injection points"
REGRESS = injection_points
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
-ISOLATION = inplace
+ISOLATION = inplace \
+ reindex_concurrently_safe_index
TAP_TESTS = 1
diff --git a/src/test/modules/injection_points/expected/reindex_concurrently_safe_index.out b/src/test/modules/injection_points/expected/reindex_concurrently_safe_index.out
new file mode 100644
index 0000000000..c36033ffe4
--- /dev/null
+++ b/src/test/modules/injection_points/expected/reindex_concurrently_safe_index.out
@@ -0,0 +1,9 @@
+Parsed test spec with 1 sessions
+
+starting permutation: reindex
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step reindex: REINDEX INDEX CONCURRENTLY test.tbl_pkey_not_safe;
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index c9e357f644..aeb47cbd8f 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -42,6 +42,7 @@ tests += {
'isolation': {
'specs': [
'inplace',
+ 'reindex_concurrently_safe_index',
],
},
'tap': {
diff --git a/src/test/modules/injection_points/specs/reindex_concurrently_safe_index.spec b/src/test/modules/injection_points/specs/reindex_concurrently_safe_index.spec
new file mode 100644
index 0000000000..dc2ebe02bf
--- /dev/null
+++ b/src/test/modules/injection_points/specs/reindex_concurrently_safe_index.spec
@@ -0,0 +1,23 @@
+setup
+{
+ CREATE EXTENSION injection_points;
+ CREATE SCHEMA test;
+ CREATE TABLE test.tbl(i int primary key, updated_at timestamp);
+ CREATE UNIQUE INDEX tbl_pkey_not_safe ON test.tbl(ABS(i)) WHERE MOD(i, 2) = 0;
+}
+
+teardown
+{
+ DROP SCHEMA test CASCADE;
+ DROP EXTENSION injection_points;
+}
+
+session test
+setup {
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('ReindexRelationConcurrently_index_safe', 'error');
+}
+step reindex { REINDEX INDEX CONCURRENTLY test.tbl_pkey_not_safe; }
+
+permutation
+ reindex
\ No newline at end of file
--
2.43.0
From 3479ee2848c3022aa31d8b5a1ff3389b82eb13fe Mon Sep 17 00:00:00 2001
From: nkey <n...@toloka.ai>
Date: Fri, 6 Sep 2024 13:17:51 +0200
Subject: [PATCH v1 2/2] Ensure the correct determination of index safety to be
used with set_indexsafe_procflags during REINDEX CONCURRENTLY
---
src/backend/commands/indexcmds.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index e07d279e2d..c34c550b4a 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3783,8 +3783,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
RestrictSearchPath();
/* determine safety of this index for set_indexsafe_procflags */
- idx->safe = (indexRel->rd_indexprs == NIL &&
- indexRel->rd_indpred == NIL);
+ idx->safe = (RelationGetIndexExpressions(indexRel) == NIL &&
+ RelationGetIndexPredicate(indexRel) == NIL);
if (idx->safe)
INJECTION_POINT("ReindexRelationConcurrently_index_safe");
idx->tableId = RelationGetRelid(heapRel);
--
2.43.0
