On Wed, Feb 4, 2026 at 11:32 AM shveta malik <[email protected]> wrote:
>
> On Wed, Feb 4, 2026 at 12:38 AM Heikki Linnakangas <[email protected]> wrote:
> >
>
> >
> > 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?
>
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.
> > 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.
>
Either message is fine with me in this situation.
--
With Regards,
Amit Kapila.
