Re: Confusing error message for REINDEX TABLE CONCURRENTLY

On Mon, Jun 3, 2019 at 6:27 PM Michael Paquier <michael(at)paquier(dot)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.