Обсуждение: Confusing error message for REINDEX TABLE CONCURRENTLY

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

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

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

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

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

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