From: | Ashwin Agrawal <aagrawal(at)pivotal(dot)io> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Confusing error message for REINDEX TABLE CONCURRENTLY |
Date: | 2019-06-04 18:26:44 |
Message-ID: | CALfoeit8j8vRD2T5fxMbD2DLOct2PaaVcqPkNqkS8hBitNWGYA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
Attachment | Content-Type | Size |
---|---|---|
v3-0001-Avoid-confusing-error-message-for-REINDEX.patch | text/x-patch | 4.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Melanie Plageman | 2019-06-04 18:33:18 | Re: Sort support for macaddr8 |
Previous Message | Stephen Frost | 2019-06-04 17:49:23 | Re: Sort support for macaddr8 |