Review of patch 0002: - I think you should just regard ReplicationSlotCtlLock as protecting the "name" and "active" flags of every slot. ReplicationSlotCreate() would then not need to mess with the spinlocks at all, and ReplicationSlotAcquire and ReplicationSlotDrop get a bit simpler too I think. Functions like ReplicationSlotsComputeRequiredXmin() can acquire this lock in shared mode and only lock the slots that are actually active, instead of all of them.
- If you address /* FIXME: apply sanity checking to slot name */, then I think that also addresses /* XXX: do we want to use truncate identifier instead? */. In other words, let's just error out if the name is too long. I'm not sure what other sanity checking is needed here; maybe just restrict it to 7-bit characters to avoid problems with encodings for individual databases varying. - ReplicationSlotAcquire probably needs to ignore slots that are not active. - ReplicationSlotAcquire should be tweaked so that the code that holds the spinlock is more self-contained. If you adopt the above-proposed recasting of ReplicationSlotCtlLock, then the part that holds the spinlock can probably look like this: SpinLockAcquire(&slot->mutex); was_active = slot->active; slot->active = true; SpinLockRelease(&slot->mutex), which looks quite a bit safer. - If there's a coding rule that slot->database can't be changed while the slot is active, then the check to make sure that the user isn't trying to bind to a slot with a mis-matching database could be done before the code described in the previous point, avoiding the need to go back and release the resource. - I think the critical section in ReplicationSlotDrop is bogus. If DeleteSlot() fails, we scarcely need to PANIC. The slot just isn't gone. - cancel_before_shmem_exit is only guaranteed to remove the most-recently-added callback. - Why does ReplicationSlotsComputeRequiredXmin() need to hold ProcArrayLock at all? - ReplicationSlotsComputeRequiredXmin scarcely needs to hold the spinlock while it does all of those gyrations. It can just acquire the spinlock, copy the three fields needed into local variables, and release the spinlock. The rest can be worked out afterwards. Similarly in ReplicationSlotsComputeRequiredXmin. - A comment in KillSlot wonders whether locking is required. I say yes. It's safe to take lwlocks and spinlocks during shmem exit, and failing to do so seems like a recipe for subtle corner-case bugs. - pg_get_replication_slots() wonders what permissions we require. I don't know that any special permissions are needed here; the data we're exposing doesn't appear to be sensitive. Unless I'm missing something? - PG_STAT_GET_LOGICAL_DECODING_SLOTS_COLS has a leftover "logical" in its name. - There seems to be no interface to acquire or release slots from either SQL or the replication protocol, nor any way for a client of this code to update its slot details. The value of catalog_xmin/data_xmin vs. effective_catalog_xmin/effective_data_xmin is left to the imagination. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
