Re: More tests with USING INDEX replident and dropped indexes

On Mon, Aug 31, 2020 at 10:36:13AM +0530, Rahila wrote:
> Now, the relreplident is being set in the transaction previous to
> the one marking index as invalid for concurrent drop. Won't
> it cause issues with relreplident cleared but index not dropped,
> if drop index concurrently fails in the small window after
> commit transaction in above snippet and before the start transaction above?

Argh. I missed your point that this stuff uses heap_inplace_update(),
so the update of indisvalid flag is not transactional. The thing is
that we won't be able to update the flag consistently with
relreplident except if we switch index_set_state_flags() to use a
transactional operation here. So, with the current state of affairs,
it looks like we could just call SetRelationReplIdent() in the
last transaction, after the transaction marking the index as dead has
committed (see the top of index_set_state_flags() saying that this
should be the last step of a transaction), but that leaves a window
open as you say.

On the other hand, it looks appealing to make index_set_state_flags()
transactional. This would solve the consistency problem, and looking
at the code scanning pg_index, I don't see a reason why we could not
do that. What do you think?

> To the existing partitioned table test, can we add a test to demonstrate
> that relreplident is set to 'n' for even the individual partitions.

Makes sense. It is still important to test the case where a
partitioned table without leaves is correctly reset IMO.

> Typo, s/updates are visible/updates visible

Thanks.

> - For DROP INDEX CONCURRENTLY, pg_class.relreplident of the parent
> table is set in the last transaction doing the drop.  It would be
> tempting to reset the flag in the same transaction as the one marking
> the index as invalid, but there could be a point in reindexing the
> invalid index whose drop has failed, and this adds morecomplexity to
> the patch.

That's a point I studied, but it makes actually more sense to just
reset the flag once the index is marked as invalid, similarly to
indisclustered. Reindexing an invalid index can have value in some
cases, but we would have much more problems with the relcache if we
finish with two indexes marked as indreplident :(
--
Michael