Re: Confusing error message for REINDEX TABLE CONCURRENTLY

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.

> @@ -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.

> + 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.

> -
> CommitTransactionCommand();

Useless noise diff.
--
Michael