On Mon, Jun 3, 2019 at 6:27 PM Michael Paquier <mich...@paquier.xyz> wrote:
> On Mon, Jun 03, 2019 at 04:53:48PM -0700, Ashwin Agrawal wrote:
> > Please check if the attached patch addresses and satisfies all the points
> > discussed so far in this thread.
>
> It looks to be so, please see below for some comments.
>
> > + {
> > result = ReindexRelationConcurrently(heapOid, options);
> > +
> > + if (!result)
> > + ereport(NOTICE,
> > + (errmsg("table \"%s\" has no indexes that can be
> concurrently reindexed",
> > + relation->relname)));
>
> "concurrently" should be at the end of this string. I have had the
> exact same argument with Tom for 508300e.
>
Sure modified the same, find attached.
> > @@ -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 */
>
> Indeed this variable is not used. So we could just get rid of it
> completely.
>
The variable is used in else scope hence I moved it there. But yes its
removed completely for this scope.
> + 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();
> > }
>
> The table has been considered for reindexing even if nothing has been
> reindexed, so perhaps we'd want to keep this part as-is? We have the
> same level of reporting for a couple of releases for this part.
>
I don't understand the review comment. I functionally didn't change
anything in that part of code, just have result variable confined to that
scope of code.
> > -
> > CommitTransactionCommand();
>
> Useless noise diff.
>
Okay, removed it.
From 4486f9d114084e3b484be8d23333ec0eb95b8f47 Mon Sep 17 00:00:00 2001
From: Ashwin Agrawal <aagra...@pivotal.io>
Date: Mon, 3 Jun 2019 16:30:39 -0700
Subject: [PATCH v3] Avoid confusing error message for REINDEX.
---
src/backend/commands/indexcmds.c | 25 ++++++++++++++++------
src/test/regress/expected/create_index.out | 10 ++++-----
2 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 40ea629ffe7..a09ee059a30 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 reindexed concurrently",
+ 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,
@@ -2676,6 +2684,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..c30e6738ba5 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 reindexed concurrently
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 reindexed concurrently
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 reindexed concurrently
\d concur_reindex_tab4
Table "public.concur_reindex_tab4"
Column | Type | Collation | Nullable | Default
--
2.19.1
