Subject: Re: [Patch] add new parameter to pg_replication_origin_session_setup

From: shveta malik <[email-masked]>
Date: Wed, 4 Feb 2026 11:32:29 +0530

On Wed, Feb 4, 2026 at 12:38 AM Heikki Linnakangas <[email-masked]> wrote:
>
> The new error message is not great:
>
> postgres=# select pg_replication_origin_session_setup('myorigin', 12345678);
> ERROR:  could not find replication state slot for replication origin
> with OID 1 which was acquired by 12345678
>
> Firstly, replication origin is not an OID. Secondly, it's a little
> confusing because the "replication state slot" is in fact present.
> However, it's currently inactive, i.e. not "acquired" by the given PID.
>
> I propose to change that to:
>
> postgres=# select pg_replication_origin_session_setup('myorigin', 12345678);
> ERROR:  replication origin with ID 1 is not active for PID 12345678
>
> That's more in line with this neighboring message:
>
> ERROR:  replication origin with ID 1 is already active for PID 701228

Thank You for your suggestions here. The suggested error message looks
better. Thus patch001 LGTM.

>
> I also wonder if the error code is appropriate. That error uses
> ERRCODE_OBJECT_IN_USE, but if the problem is that the origin is
> currently *not* active, that seems backwards. I didn't change that in
> the attached patch, but it's something to think about.

I agree that ERRCODE_OBJECT_IN_USE doesn’t seem like the best fit in
below code. IMO, ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE would be
more suitable.But let's hear from others.

if (curstate->acquired_by != acquired_by)
{
ereport(ERROR,
(errcode(ERRCODE_OBJECT_IN_USE),
errmsg("replication origin with ID %d is not active for PID %d",
curstate->roident, acquired_by)));
}

>
> The second patch rearranges the if-else statements to check those
> conditions. I found the current logic hard to follow, this makes them
> feel more natural, in my opinion at least.

I’m not fully convinced that it makes the code clearer or better to
understand. For example, consider the following error case:

else if (curstate->acquired_by == 0 && curstate->refcount > 0)
    ereport(ERROR,
            (errcode(ERRCODE_OBJECT_IN_USE),
             errmsg("replication origin with ID %d is already active
in another process",
                    curstate->roident)));

The condition was self explanatory earlier. The condition has now been
rewritten (or reduced) to:
if (acquired_by == 0 && curstate->refcount > 0)

However, this is not exactly the same as the earlier logic. On closer
inspection, the condition 'curstate->acquired_by == 0', while no
longer stated explicitly, was implicitly guaranteed by the preceding
error block:

if (curstate->acquired_by != 0)
    ERROR;

One way to make this clearer would be to structure the logic as:

if (curstate->acquired_by != 0)
    ERROR;
else if (curstate->refcount > 0)
    ERROR;

Even then, it is still not clear why this error (the case where
curstate->refcount > 0 and curstate->acquired_by == 0) is raised only
when acquired_by == 0, or how the same situation is expected to be
handled when acquired_by != 0. The earlier code was clearer in this
regard (at least to me), as it first handled the
'curstate->acquired_by != acquired_by' case, making the overall logic
somehow easier to understand. Perhaps we can try by adding a comment
or restructure in some other way if possible and needed?

> It has one user-visible
> effect: If you call the function with acquired_pid != 0 and the origin
> has no state slot, *and* there are no free slots, you previously got
> this error:
>
> postgres=# select pg_replication_origin_session_setup('other', 123);
> ERROR:  could not find free replication state slot for replication
> origin with ID 2
> HINT:  Increase "max_active_replication_origins" and try again.
>
> Now you get this:
>
> postgres=# select pg_replication_origin_session_setup('other', 123);
> ERROR:  cannot use PID 123 for inactive replication origin with ID 2
>
> Both error messages are more or less appropriate in that situation, but
> I think the new behavior is slightly better. The fact that the origin is
> inactive feels like the bigger problem here.

I am okay with this change though. Let's see what others have to say.

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Wed, 11 Feb 2026 15:40:50 +0530

> or restructure in some other way if possible and needed?

I see your point but one advantage with the proposed code change is
that it started to appear that we can extend this part of code easily
in the future as it separates most of the handling related to when a
user has given acquired_by parameter's value as zero and non-zero.

> I am okay with this change though. Let's see what others have to say.

Either message is fine with me in this situation.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Wed, 11 Feb 2026 17:09:26 +0530

> user has given acquired_by parameter's value as zero and non-zero.

Okay, yes. So I am okay with it. The slight change I suggested (if to
else-if) and a comment will make it more clean.

---

From: Michael Paquier <[email-masked]>
Date: Wed, 25 Mar 2026 11:45:37 +0900

On Mon, Feb 16, 2026 at 02:22:57PM +0530, Amit Kapila wrote:
> Heikki, would you like to take care of improvements proposed by you?
> Otherwise, I am happy to take care of them.

This thread has stalled.  Heikki, Amit, are there any actions planned
by the end of the release cycle?
--
Michael

---

From: Amit Kapila <[email-masked]>
Date: Wed, 25 Mar 2026 08:32:51 +0530

> by the end of the release cycle?

I'll take care of this in the next few days unless I see any
response/feedback from Heikki or others.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 25 Mar 2026 13:51:14 +0530

> else-if) and a comment will make it more clean.

I have tried to address both your suggestions in the attached. See, if
this looks okay to you now?

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Wed, 25 Mar 2026 15:03:27 +0530

> this looks okay to you now?

LGTM now, thanks!

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Thu, 26 Mar 2026 12:10:08 +0530

> > this looks okay to you now?

Pushed.

-- 
With Regards,
Amit Kapila.