Sorry for the late response. I may be misunderstanding the patch, but it looks to me as if it is trying to handle several different classes of problems uniformly by wrapping them in exception handling.
ReplicationSlotAcquire() can exit with an ERROR after setting MyReplicationSlot. I think it should restore that state itself. Or rather, perhaps it should not set MyReplicationSlot until it is certain that the slot can be successfully acquired. I think the same kind of argument applies to ReplicationSlotCreate(). There is also a different issue around pgstat_create_replslot(). If ReplicationSlotCreate() calls it and then an error is thrown later in the caller, the caller needs to clean up that pgstat entry. That seems like a separate kind of cleanup from restoring MyReplicationSlot. In pg_logical_slot_get_changes_guts(), the patch moves the call to ReplicationSlotAcquire() inside the PG_TRY block. In practice the code checks whether MyReplicationSlot is NULL before releasing it, so this may work. However, semantically, it looks as if the caller is also responsible for releasing the slot even when ReplicationSlotAcquire() itself fails. More generally, I am not sure that all of these cleanup actions belong at the same level. Restoring MyReplicationSlot, cleaning up pgstat entry, and releasing an acquired slot seem to be different kinds of responsibilities. Handling them all through the same PG_TRY/PG_CATCH blocks causes those cleanup responsibilities to cross component boundaries. That, in turn, makes it harder to tell whether all resources affected by an error are actually being cleaned up. Regards. -- Kyotaro Horiguchi NTT Open Source Software Center
