Hi Michael,
Thanks for updating the patch.
Please see following comments,
1.
*/
if (resetreplident)
{
SetRelationReplIdent(heapId, REPLICA_IDENTITY_NOTHING);
/* make the update visible, only for the non-concurrent case */
if (!concurrent)
CommandCounterIncrement();
}
/* continue the concurrent process */
if (concurrent)
{
PopActiveSnapshot();
CommitTransactionCommand();
StartTransactionCommand();
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?
Although, it seems like a really small window.
2. I have following suggestion for the test. To the existing partitioned table test, can we add a test to demonstrate
that relreplident is set to 'n' for even the individual partitions.
I mean explicitly setting replica identity index for individual partitions
ALTER TABLE part1 REPLICA IDENTITY
USING INDEX test_replica_part_index_part_1;
and checking for relreplident for individual partition after parent index is dropped.
SELECT relreplident FROM pg_class WHERE oid = 'part1'::regclass;
3.
+* Again, commit the transaction to make the pg_index and pg_class
+ * (in the case where indisreplident is set) updates are visible to
+ * other sessions.
Typo, s/updates are visible/updates visible
4. I am wondering with the recent change to move the SetRelationRepldent
together with invalidating index, whether your initial concern stated as follows
becomes valid.
- 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 more complexity to
the patch.
I will try to test that.
Thank you,
Rahila Syed