Re: [PATCH] Release replication slot on error in SQL-callable slot functions

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