Обсуждение: pg_index.indisreplident and invalid indexes

Hi all,

While digging into a different patch involving DROP INDEX CONCURRENTLY
and replica indexes, I have found that the handling of indisreplident
is inconsistent for invalid indexes:
https://www.postgresql.org/message-id/20200827022835.GM2017@paquier.xyz

In short, imagine the following sequence:
CREATE TABLE test_replica_identity_4 (id int NOT NULL);
CREATE UNIQUE INDEX test_replica_index_4 ON
  test_replica_identity_4(id);
ALTER TABLE test_replica_identity_4 REPLICA IDENTITY
  USING INDEX test_replica_index_4;
-- imagine that this fails in the second transaction used in
-- index_drop().
DROP INDEX CONCURRENTLY test_replica_index_4;
-- here the index still exists, is invalid, marked with
-- indisreplident.
CREATE UNIQUE INDEX test_replica_index_4_2 ON
  test_replica_identity_4(id);
ALTER TABLE test_replica_identity_4 REPLICA IDENTITY
  USING INDEX test_replica_index_4_2;
-- set back the index to a valid state.
REINDEX INDEX test_replica_index_4;
-- And here we have two valid indexes usable as replica identities.
SELECT indexrelid::regclass, indisvalid, indisreplident FROM pg_index
  WHERE indexrelid IN ('test_replica_index_4'::regclass,
                       'test_replica_index_4_2'::regclass);
       indexrelid       | indisvalid | indisreplident
------------------------+------------+----------------
 test_replica_index_4_2 | t          | t
 test_replica_index_4   | t          | t
(2 rows)

You can just use the following trick to emulate a failure in DIC:
@@ -2195,6 +2195,9 @@ index_drop(Oid indexId, bool concurrent, bool
concurrent_lock_mode)
    if (userIndexRelation->rd_rel->relkind != RELKIND_PARTITIONED_INDEX)
            RelationDropStorage(userIndexRelation);
+   if (concurrent)
+       elog(ERROR, "burp");

This results in some problems for ALTER TABLE in tablecmds.c, as it is
possible to reach a state in the catalogs where we have *multiple*
indexes marked with indisreplindex for REPLICA_IDENTITY_INDEX set on
the parent table.  Even worse, this messes up with
RelationGetIndexList() as it would set rd_replidindex in the relcache
for the last index found marked with indisreplident, depending on the
order where the indexes are scanned, you may get a different replica
index loaded.

I think that this problem is similar to indisclustered, and that we
had better set indisreplident to false when clearing indisvalid for an
index concurrently dropped.  This would prevent problems with ALTER
TABLE of course, but also the relcache.

Any objections to the attached?  I am not sure that this is worth a
backpatch as that's unlikely going to be a problem in the field, so
I'd like to fix this issue only on HEAD.  This exists since 9.4 and
the introduction of replica identities.
--
Michael

> On Thu, Aug 27, 2020 at 11:57:21AM +0900, Michael Paquier wrote:
>
> I think that this problem is similar to indisclustered, and that we
> had better set indisreplident to false when clearing indisvalid for an
> index concurrently dropped.  This would prevent problems with ALTER
> TABLE of course, but also the relcache.
>
> Any objections to the attached?  I am not sure that this is worth a
> backpatch as that's unlikely going to be a problem in the field, so
> I'd like to fix this issue only on HEAD.  This exists since 9.4 and
> the introduction of replica identities.

Thanks for the patch. It sounds right, so no objections from me. But I
wonder if something similar has to be done also for
index_concurrently_swap function?

    /*
     * Mark the new index as valid, and the old index as invalid similarly to
     * what index_set_state_flags() does.
     */
    newIndexForm->indisvalid = true;
    oldIndexForm->indisvalid = false;
    oldIndexForm->indisclustered = false;

On Fri, Aug 28, 2020 at 10:15:37AM +0200, Dmitry Dolgov wrote:
> Thanks for the patch. It sounds right, so no objections from me. But I
> wonder if something similar has to be done also for
> index_concurrently_swap function?

As of index.c, this already happens:
    /* Preserve indisreplident in the new index */
    newIndexForm->indisreplident = oldIndexForm->indisreplident;
    oldIndexForm->indisreplident = false;

In short, the new concurrent index is created first with
indisreplident = false, and when swapping the old and new indexes, the
new index inherits the setting of the old one, and the old one planned
for drop uses indisreplident = false when swapping.
--
Michael