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