Re: add log messages when replication slots become active and inactive (was Re: Is it worth adding ReplicationSlot active_pid to ReplicationSlotPersistentData?)

Hi, I had a quick look at the v7 patch.

You might consider to encapsulate some of this logic in new functions like:

void
LogReplicationSlotAquired(bool is_physical, char *slotname)
{
    loglevel = log_replication_commands ? LOG : DEBUG3;

    if (is_physical)
        ereport(loglevel,
            (errmsg("acquired physical replication slot \"%s\"", slotname)));
    else
        ereport(loglevel,
            (errmsg("acquired logical replication slot \"%s\"", slotname)));
}

void
LogReplicationSlotReleased(bool is_physical, char *slotname)
{
    loglevel = log_replication_commands ? LOG : DEBUG3;

    if (is_physical)
        ereport(loglevel,
            (errmsg("released physical replication slot \"%s\"", slotname)));
    else
        ereport(loglevel,
            (errmsg("released logical replication slot \"%s\"", slotname)));
}

~~

THEN

ReplicationSlotShmemExit and WalSndErrorCleanup can call it like:
if (MyReplicationSlot != NULL)
{
    bool is_physical = SlotIsPhysical(MyReplicationSlot);
    char *slotname = pstrdup(NameStr(MyReplicationSlot->data.name));

    ReplicationSlotRelease();
    LogReplicationSlotReleased(is_physical, slotname);
}

~

StartlReplication can call like:
LogReplicationSlotAquired(true, cmd->slotname);
...
LogReplicationSlotReleased(true, cmd->slotname);

~

StartLogicalReplication can call like:
LogReplicationSlotAquired(false, cmd->slotname);
...
LogReplicationSlotReleased(false, cmd->slotname);


~~~

TBH, I am not sure for the *current* code if the encapsulation is
worth the trouble or not. But maybe at least it helps message
consistency and will make it easier if future callers are needed. I
guess those functions could also give you some central point to
comment the intent of this logging? Feel free to take or leave this
code suggestion.

------
Kind Regards,
Peter Smith.
Fujitsu Australia