Expand comment on the slot recheck in drop_local_obsolete_slots().

The existing comment explained that a user-created slot could reuse the
same shared memory as 'local_slot' during the window between selecting a
slot to drop and locking its database, and that we therefore recheck
before dropping.  It did not, however, spell out the fuller consequence:
because local_slot points to a reusable slot-array entry, its fields may
already describe a replacement slot, so the earlier drop decision and the
slot_database used for locking could relate to an unrelated slot/database.

Expand the comment to describe this, and note that the recheck prevents
us from dropping a user-created replacement slot while the residual risk
(such as briefly locking an unrelated database) is confined to the cycle
and is acceptable given the race is rare and non-fatal.

No functional change.

Author: Fujii Masao <masao.fujii@gmail.com>
Author: Xuneng Zhou <xunengzhou@gmail.com>
Author: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwGGyEDL3dh7uJ6qPsGvnq4QK_R8+U=12CaprnzwrwaLGA@mail.gmail.com
Discussion: https://postgr.es/m/CAHGQGwHqQ1PPVFfYKVxLfRyC-byRdwSN0NeaHj9SLYV97oO5cw@mail.gmail.com

diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index 0563734436351c28d29a3edfc5b1a962c5e60b53..d1936823506ec42074540c6337a893f5303750c5 100644 (file)
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -557,9 +557,18 @@ drop_local_obsolete_slots(List *remote_slot_list)
                         * locking the database, there is a possibility of a parallel
                         * database drop by the startup process and the creation of a new
                         * slot by the user. This new user-created slot may end up using
-                        * the same shared memory as that of 'local_slot'. Thus check if
-                        * local_slot is still the synced one before performing the actual
-                        * drop.
+                        * the same shared memory as that of 'local_slot'.
+                        *
+                        * Because local_slot still points to a reusable slot-array entry,
+                        * its fields (name, database OID, invalidation state) may already
+                        * describe such a replacement slot by the time we reach here.
+                        * That means the drop decision made by local_sync_slot_required()
+                        * above could have been based on the replacement slot's data, and
+                        * slot_database could refer to an unrelated database. The recheck
+                        * below keeps us from actually dropping a user-created
+                        * replacement slot; the residual risk is confined to this cycle
+                        * (for example, briefly locking an unrelated database) and is
+                        * acceptable because the race is rare and non-fatal.
                         */
                        SpinLockAcquire(&local_slot->mutex);
                        synced_slot = local_slot->in_use && local_slot->data.synced;