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

From: Amit Kapila <[email-masked]>
Date: Wed, 22 Mar 2023 11:33:33 +0530

On Thu, Sep 15, 2022 at 10:39 AM Bharath Rupireddy
<[email-masked]> wrote:
>
> On Mon, Jul 25, 2022 at 6:31 PM Bharath Rupireddy
> <[email-masked]> wrote:
> >
> > Here's the v6 patch, a much simpler one - no changes to any of the
> > existing function APIs. Please see the sample logs at [1]. There's a
> > bit of duplicate code in the v6 patch, if the overall approach looks
> > okay, I can remove that too in the next version of the patch.
>
> I modified the log_replication_commands description in guc_tables.c.
> Please review the v7 patch further.
>

I see that you have modified the patch to address the comments from
Alvaro. Personally, I feel it would be better to add such a message at
a centralized location instead of spreading these in different callers
of slot acquire/release functionality to avoid getting these missed in
the new callers in the future. However, if Alvaro and others think
that the current style is better then we should go ahead and do it
that way. I hope that we should be able to decide on this and get it
into PG16. Anyone else would like to weigh in here?

-- 
With Regards,
Amit Kapila.

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 23 Mar 2023 11:07:26 +0100

> into PG16. Anyone else would like to weigh in here?

I like Peter Smith's suggestion downthread.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 23 Mar 2023 23:03:38 +0530

> I like Peter Smith's suggestion downthread.

+1. Please review the attached v8 patch further.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: "Euler Taveira" <[email-masked]>
Date: Thu, 23 Mar 2023 18:40:26 -0300

> +1. Please review the attached v8 patch further.
If you are adding separate functions as suggested, you should add a comment at
the top of ReplicationSlotAcquire() and ReplicationSlotRelease() functions
saying that LogReplicationSlotAquired() and LogReplicationSlotReleased()
functions should be called  respectively after it.

My suggestion is that the functions should have the same name with a "Log"
prefix. On of them has a typo "Aquired" in its name. Hence,
LogReplicationSlotAcquire() and LogReplicationSlotRelease() as names. It is
easier to find if someone is grepping by the origin function.

I prefer a sentence that includes a verb.

  physical replication slot \"%s\" is acquired
  logical replication slot \"%s\" is released

Isn't the PID important for this use case? If so, of course, you can rely on
log_line_prefix (%p) but if the PID is crucial for an investigation then it
should also be included in the message.

--
Euler Taveira
EDB   https://www.enterprisedb.com/