On Tue, May 28, 2019 at 12:05 PM David Rowley <david.row...@2ndquadrant.com>
wrote:
> On Tue, 28 May 2019 at 01:23, Ashwin Agrawal <aagra...@pivotal.io> wrote:
> > I think we will need to separate out the NOTICE message for concurrent
> and regular case.
> >
> > For example this doesn't sound correct
> > WARNING: cannot reindex exclusion constraint index
> "public.circles_c_excl" concurrently, skipping
> > NOTICE: table "circles" has no indexes to reindex
> >
> > As no indexes can't be reindexed *concurrently* but there are still
> indexes which can be reindexed, invalid indexes I think fall in same
> category.
>
> Swap "can't" for "can" and, yeah. I think it would be good to make the
> error messages differ for these two cases. This would serve as a hint
> to the user that they might have better luck trying without the
> "concurrently" option.
>
Please check if the attached patch addresses and satisfies all the points
discussed so far in this thread.
Was thinking of adding explicit errhint for concurrent case NOTICE to
convey, either the table has no indexes or can only be reindexed without
CONCURRENTLY. But thought may be its obvious but feel free to add if would
be helpful.
From 2e9eceff07d9bdadef82a54c17f399263436bf9d Mon Sep 17 00:00:00 2001
From: Ashwin Agrawal <aagra...@pivotal.io>
Date: Mon, 3 Jun 2019 16:30:39 -0700
Subject: [PATCH v2] Avoid confusing error message for REINDEX.
---
src/backend/commands/indexcmds.c | 26 +++++++++++++++-------
src/test/regress/expected/create_index.out | 10 ++++-----
2 files changed, 23 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 40ea629ffe7..bab20028090 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2438,17 +2438,25 @@ ReindexTable(RangeVar *relation, int options, bool concurrent)
RangeVarCallbackOwnsTable, NULL);
if (concurrent)
+ {
result = ReindexRelationConcurrently(heapOid, options);
+
+ if (!result)
+ ereport(NOTICE,
+ (errmsg("table \"%s\" has no indexes that can be concurrently reindexed",
+ relation->relname)));
+ }
else
+ {
result = reindex_relation(heapOid,
REINDEX_REL_PROCESS_TOAST |
REINDEX_REL_CHECK_CONSTRAINTS,
options);
-
- if (!result)
- ereport(NOTICE,
- (errmsg("table \"%s\" has no indexes",
- relation->relname)));
+ if (!result)
+ ereport(NOTICE,
+ (errmsg("table \"%s\" has no indexes to reindex",
+ relation->relname)));
+ }
return heapOid;
}
@@ -2630,7 +2638,6 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
foreach(l, relids)
{
Oid relid = lfirst_oid(l);
- bool result;
StartTransactionCommand();
/* functions in indexes may want a snapshot set */
@@ -2638,11 +2645,12 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
if (concurrent)
{
- result = ReindexRelationConcurrently(relid, options);
+ ReindexRelationConcurrently(relid, options);
/* ReindexRelationConcurrently() does the verbose output */
}
else
{
+ bool result;
result = reindex_relation(relid,
REINDEX_REL_PROCESS_TOAST |
REINDEX_REL_CHECK_CONSTRAINTS,
@@ -2656,7 +2664,6 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
PopActiveSnapshot();
}
-
CommitTransactionCommand();
}
StartTransactionCommand();
@@ -2676,6 +2683,9 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
* The locks taken on parent tables and involved indexes are kept until the
* transaction is committed, at which point a session lock is taken on each
* relation. Both of these protect against concurrent schema changes.
+ *
+ * Returns true if any indexes have been rebuilt (including toast table's
+ * indexes when relevant).
*/
static bool
ReindexRelationConcurrently(Oid relationOid, int options)
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index c8bc6be0613..72a8fca48e4 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -1944,9 +1944,9 @@ DROP TABLE reindex_verbose;
CREATE TABLE concur_reindex_tab (c1 int);
-- REINDEX
REINDEX TABLE concur_reindex_tab; -- notice
-NOTICE: table "concur_reindex_tab" has no indexes
+NOTICE: table "concur_reindex_tab" has no indexes to reindex
REINDEX TABLE CONCURRENTLY concur_reindex_tab; -- notice
-NOTICE: table "concur_reindex_tab" has no indexes
+NOTICE: table "concur_reindex_tab" has no indexes that can be concurrently reindexed
ALTER TABLE concur_reindex_tab ADD COLUMN c2 text; -- add toast index
-- Normal index with integer column
CREATE UNIQUE INDEX concur_reindex_ind1 ON concur_reindex_tab(c1);
@@ -2043,10 +2043,10 @@ ERROR: REINDEX is not yet implemented for partitioned indexes
-- REINDEX is a no-op for partitioned tables
REINDEX TABLE concur_reindex_part_10;
WARNING: REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
-NOTICE: table "concur_reindex_part_10" has no indexes
+NOTICE: table "concur_reindex_part_10" has no indexes to reindex
REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
WARNING: REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
-NOTICE: table "concur_reindex_part_10" has no indexes
+NOTICE: table "concur_reindex_part_10" has no indexes that can be concurrently reindexed
SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
ORDER BY relid, level;
relid | parentrelid | level
@@ -2150,7 +2150,7 @@ DELETE FROM concur_reindex_tab4 WHERE c1 = 1;
-- The invalid index is not processed when running REINDEX TABLE.
REINDEX TABLE CONCURRENTLY concur_reindex_tab4;
WARNING: cannot reindex invalid index "public.concur_reindex_ind5" concurrently, skipping
-NOTICE: table "concur_reindex_tab4" has no indexes
+NOTICE: table "concur_reindex_tab4" has no indexes that can be concurrently reindexed
\d concur_reindex_tab4
Table "public.concur_reindex_tab4"
Column | Type | Collation | Nullable | Default
--
2.19.1
