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

From Peter Smith
Subject Re: add log messages when replication slots become active and inactive (was Re: Is it worth adding ReplicationSlot active_pid to ReplicationSlotPersistentData?)
Date
Msg-id CAHut+Pu6Knwooc_NckMxszGrAJnytgpMadtoJ-OA-SFWT+GFYw@mail.gmail.com
Whole thread Raw
In response to Re: add log messages when replication slots become active and inactive (was Re: Is it worth adding ReplicationSlot active_pid to ReplicationSlotPersistentData?)  (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>)
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: [BUG] pg_stat_statements and extended query protocol
Next
From: Alvaro Herrera
Date:
Subject: Re: doc: add missing "id" attributes to extension packaging page