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