Re: reindex concurrently and two toast indexes

On Thu, Feb 27, 2020 at 04:32:11PM +0900, Michael Paquier wrote:
> On Sat, Feb 22, 2020 at 04:06:57PM +0100, Julien Rouhaud wrote:
> > Sorry, I just realized that I forgot to commit the last changes before sending
> > the patch, so here's the correct v2.
>
> Thanks for the patch.
>
> > +    if (skipit)
> > +    {
> > +        ereport(NOTICE,
> > +             (errmsg("skipping invalid index \"%s.%s\"",
> > +                 get_namespace_name(get_rel_namespace(indexOid)),
> > +                 get_rel_name(indexOid))));
>
> ReindexRelationConcurrently() issues a WARNING when bumping on an
> invalid index, shouldn't the same log level be used?

For ReindexRelationConcurrently, the index is skipped because the feature isn't
supported, thus a warning.  In this case that would work, it's just that we
don't want to process such indexes, so I used a notice instead.

I'm not opposed to use a warning instead if you prefer.  What errcode should be
used though, ERRCODE_WARNING?  ERRCODE_FEATURE_NOT_SUPPORTED doesn't feel
right.

> Even with this patch, it is possible to reindex an invalid toast index
> with REINDEX INDEX (with and without CONCURRENTLY), which is the
> problem I mentioned upthread (Er, actually only for the non-concurrent
> case as told about reindex_index).  Shouldn't both cases be prevented
> as well with an ERROR?

Ah indeed, sorry I missed that.

While looking at it, I see that invalid indexes seem to leaked when the table
is dropped, with no way to get rid of them:

s1:
CREATE TABLE t1(val text);
CREATE INDEX ON t1 (val);
BEGIN;
SELECT * FROM t1 FOR UPDATE;

s2:
REINDEX TABLE CONCURRENTLY t1;
[stucked and canceled]
SELECT indexrelid::regclass, indrelid::regclass FROM pg_index WHERE NOT indisvalid;
             indexrelid              |        indrelid
-------------------------------------+-------------------------
 t1_val_idx_ccold                    | t1
 pg_toast.pg_toast_16385_index_ccold | pg_toast.pg_toast_16385
(2 rows)

s1:
ROLLBACK;
DROP TABLE t1;

SELECT indexrelid::regclass, indrelid::regclass FROM pg_index WHERE NOT indisvalid;
             indexrelid              | indrelid
-------------------------------------+----------
 t1_val_idx_ccold                    | 16385
 pg_toast.pg_toast_16385_index_ccold | 16388
(2 rows)

REINDEX INDEX t1_val_idx_ccold;
ERROR:  XX000: could not open relation with OID 16385
LOCATION:  relation_open, relation.c:62

DROP INDEX t1_val_idx_ccold;
ERROR:  XX000: could not open relation with OID 16385
LOCATION:  relation_open, relation.c:62

REINDEX INDEX pg_toast.pg_toast_16385_index_ccold;
ERROR:  XX000: could not open relation with OID 16388
LOCATION:  relation_open, relation.c:62

DROP INDEX pg_toast.pg_toast_16385_index_ccold;
ERROR:  XX000: could not open relation with OID 16388
LOCATION:  relation_open, relation.c:62

REINDEX DATABASE rjuju;
REINDEX

SELECT indexrelid::regclass, indrelid::regclass FROM pg_index WHERE NOT indisvalid;
             indexrelid              | indrelid
-------------------------------------+----------
 t1_val_idx_ccold                    | 16385
 pg_toast.pg_toast_16385_index_ccold | 16388
(2 rows)

Shouldn't DROP TABLE be fixed to also drop invalid indexes?