Horton

Thread

Commits

  1. Fix confusing NOTICE text in REINDEX CONCURRENTLY

  1. Confusing error message for REINDEX TABLE CONCURRENTLY

    Ashwin Agrawal <aagrawal@pivotal.io> — 2019-05-25T00:06:25Z

    CREATE TABLE circles (c circle, EXCLUDE USING gist (c WITH &&));

REINDEX TABLE CONCURRENTLY circles;
WARNING:  cannot reindex exclusion constraint index "public.circles_c_excl"
concurrently, skipping
NOTICE:  table "circles" has no indexes
REINDEX

The message "table has no indexes" is confusing, as warning above it states
table has index, just was skipped by reindex.

So, currently for any reason (exclusion or invalid index) reindex table
concurrently skips reindex, it reports the table has no index. Looking at
the behavior of non-concurrent reindex, it emits the NOTICE only if table
really has no indexes (since it has no skip cases).

We need to see what really wish to communicate here, table has no indexes
or just that reindex was *not* performed or keep it simple and completely
avoid emitting anything. If we skip any indexes we anyways emit WARNING, so
that should be sufficient and nothing more needs to be conveyed.

In-case we wish to communicate no reindex was performed, what do we wish to
notify for empty tables?

Seems might be just emit the NOTICE "table xxx has no index", if really no
index for concurrent and non-concurrent case, make it consistent, less
confusing and leave it there. Attaching the patch to just do that. Thoughts?

  2. Re: Confusing error message for REINDEX TABLE CONCURRENTLY

    David Rowley <david.rowley@2ndquadrant.com> — 2019-05-25T02:42:59Z

    On Sat, 25 May 2019 at 12:06, Ashwin Agrawal <aagrawal@pivotal.io> wrote:
> Seems might be just emit the NOTICE "table xxx has no index", if really no index for concurrent and non-concurrent case, make it consistent, less confusing and leave it there. Attaching the patch to just do that. Thoughts?

Would it not be better just to change the error message for the
concurrent case so that it reads: "table \"%s\" has no indexes that
can be concurrently reindexed"

Otherwise, what you have now is still confusing for partitioned tables:

postgres=# create table listp (a int primary key) partition by list(a);
CREATE TABLE
postgres=# REINDEX TABLE CONCURRENTLY listp;
psql: WARNING:  REINDEX of partitioned tables is not yet implemented,
skipping "listp"
psql: NOTICE:  table "listp" has no indexes
REINDEX

Also, I think people probably will care more about the fact that
nothing was done for that table rather than if the table happens to
have no indexes. For the non-concurrently case, that just happened to
be the same thing.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  3. Re: Confusing error message for REINDEX TABLE CONCURRENTLY

    Michael Paquier <michael@paquier.xyz> — 2019-05-27T01:43:30Z

    On Sat, May 25, 2019 at 02:42:59PM +1200, David Rowley wrote:
> Also, I think people probably will care more about the fact that
> nothing was done for that table rather than if the table happens to
> have no indexes. For the non-concurrently case, that just happened to
> be the same thing.

This is equally confusing for plain REINDEX as well, no?  Taking your
previous example:
=#  REINDEX TABLE listp;
WARNING:  0A000: REINDEX of partitioned tables is not yet implemented,
skipping "listp"
LOCATION:  reindex_relation, index.c:3513
NOTICE:  00000: table "listp" has no indexes
LOCATION:  ReindexTable, indexcmds.c:2452
REINDEX

In this case the relation has partitioned indexes, not indexes, so
that's actually correct.  Still it seems to me that some users could
get confused by the current wording.

For invalid indexes you would get that:
=# create table aa (a int);
CREATE TABLE
=# insert into aa values (1),(1);
INSERT 0 2
=# create unique index concurrently aai on aa(a);
ERROR:  23505: could not create unique index "aai"
DETAIL:  Key (a)=(1) is duplicated.
SCHEMA NAME:  public
TABLE NAME:  aa
CONSTRAINT NAME:  aai
LOCATION:  comparetup_index_btree, tuplesort.c:405
=# reindex table concurrently aa;
WARNING:  0A000: cannot reindex invalid index "public.aai"
concurrently, skipping
LOCATION:  ReindexRelationConcurrently, indexcmds.c:2772
NOTICE:  00000: table "aa" has no indexes
LOCATION:  ReindexTable, indexcmds.c:2452
REINDEX

As you mention for reindex_relation() no indexes <=> nothing to do,
still let's not rely on that.  Instead of making the error message
specific to concurrent operations, I would suggest to change it to
"table foo has no indexes to reindex".  What do you think about the
attached?
--
Michael

  4. Re: Confusing error message for REINDEX TABLE CONCURRENTLY

    Ashwin Agrawal <aagrawal@pivotal.io> — 2019-05-28T05:23:19Z

    On Sun, May 26, 2019 at 6:43 PM Michael Paquier <michael@paquier.xyz> wrote:

> As you mention for reindex_relation() no indexes <=> nothing to do,
> still let's not rely on that.  Instead of making the error message
> specific to concurrent operations, I would suggest to change it to
> "table foo has no indexes to reindex".  What do you think about the
> attached?
>

I think we will need to separate out the NOTICE message for concurrent and
regular case.

For example this doesn't sound correct
WARNING:  cannot reindex exclusion constraint index "public.circles_c_excl"
concurrently, skipping
NOTICE:  table "circles" has no indexes to reindex

As no indexes can't be reindexed *concurrently* but there are still indexes
which can be reindexed, invalid indexes I think fall in same category.

  5. Re: Confusing error message for REINDEX TABLE CONCURRENTLY

    David Rowley <david.rowley@2ndquadrant.com> — 2019-05-28T19:04:46Z

    On Tue, 28 May 2019 at 01:23, Ashwin Agrawal <aagrawal@pivotal.io> wrote:
> I think we will need to separate out the NOTICE message for concurrent and regular case.
>
> For example this doesn't sound correct
> WARNING:  cannot reindex exclusion constraint index "public.circles_c_excl" concurrently, skipping
> NOTICE:  table "circles" has no indexes to reindex
>
> As no indexes can't be reindexed *concurrently* but there are still indexes which can be reindexed, invalid indexes I think fall in same category.

Swap "can't" for "can" and, yeah. I think it would be good to make the
error messages differ for these two cases. This would serve as a hint
to the user that they might have better luck trying without the
"concurrently" option.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  6. Re: Confusing error message for REINDEX TABLE CONCURRENTLY

    Ashwin Agrawal <aagrawal@pivotal.io> — 2019-06-03T23:53:48Z

    On Tue, May 28, 2019 at 12:05 PM David Rowley <david.rowley@2ndquadrant.com>
wrote:

> On Tue, 28 May 2019 at 01:23, Ashwin Agrawal <aagrawal@pivotal.io> wrote:
> > I think we will need to separate out the NOTICE message for concurrent
> and regular case.
> >
> > For example this doesn't sound correct
> > WARNING:  cannot reindex exclusion constraint index
> "public.circles_c_excl" concurrently, skipping
> > NOTICE:  table "circles" has no indexes to reindex
> >
> > As no indexes can't be reindexed *concurrently* but there are still
> indexes which can be reindexed, invalid indexes I think fall in same
> category.
>
> Swap "can't" for "can" and, yeah. I think it would be good to make the
> error messages differ for these two cases. This would serve as a hint
> to the user that they might have better luck trying without the
> "concurrently" option.
>

Please check if the attached patch addresses and satisfies all the points
discussed so far in this thread.

Was thinking of adding explicit errhint for concurrent case NOTICE to
convey, either the table has no indexes or can only be reindexed without
CONCURRENTLY. But thought may be its obvious but feel free to add if would
be helpful.

  7. Re: Confusing error message for REINDEX TABLE CONCURRENTLY

    Michael Paquier <michael@paquier.xyz> — 2019-06-04T01:27:00Z

    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

  8. Re: Confusing error message for REINDEX TABLE CONCURRENTLY

    Ashwin Agrawal <aagrawal@pivotal.io> — 2019-06-04T18:26:44Z

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

  9. Re: Confusing error message for REINDEX TABLE CONCURRENTLY

    Michael Paquier <michael@paquier.xyz> — 2019-06-05T06:11:13Z

    On Tue, Jun 04, 2019 at 11:26:44AM -0700, Ashwin Agrawal wrote:
> The variable is used in else scope hence I moved it there. But yes its
> removed completely for this scope.

Thanks for updating the patch.  It does its job by having one separate
message for the concurrent and the non-concurrent cases as discussed.
David, what do you think?  Perhaps you would like to commit it
yourself?
--
Michael

  10. Re: Confusing error message for REINDEX TABLE CONCURRENTLY

    David Rowley <david.rowley@2ndquadrant.com> — 2019-06-05T09:08:46Z

    On Wed, 5 Jun 2019 at 18:11, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Jun 04, 2019 at 11:26:44AM -0700, Ashwin Agrawal wrote:
> > The variable is used in else scope hence I moved it there. But yes its
> > removed completely for this scope.
>
> Thanks for updating the patch.  It does its job by having one separate
> message for the concurrent and the non-concurrent cases as discussed.
> David, what do you think?  Perhaps you would like to commit it
> yourself?

Thanks. I've just pushed this with some additional comment changes.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services