public inbox for [email protected]help / color / mirror / Atom feed
Re: [Patch] add new parameter to pg_replication_origin_session_setup 6+ messages / 2 participants [nested] [flat]
* Re: [Patch] add new parameter to pg_replication_origin_session_setup @ 2026-02-04 06:02 shveta malik <[email protected]> 2026-02-11 10:10 ` Re: [Patch] add new parameter to pg_replication_origin_session_setup Amit Kapila <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: shveta malik @ 2026-02-04 06:02 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Amit Kapila <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; [email protected] <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Doruk Yilmaz <[email protected]>; shveta malik <[email protected]> On Wed, Feb 4, 2026 at 12:38 AM Heikki Linnakangas <[email protected]> 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 ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [Patch] add new parameter to pg_replication_origin_session_setup 2026-02-04 06:02 Re: [Patch] add new parameter to pg_replication_origin_session_setup shveta malik <[email protected]> @ 2026-02-11 10:10 ` Amit Kapila <[email protected]> 2026-02-11 11:39 ` Re: [Patch] add new parameter to pg_replication_origin_session_setup shveta malik <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Amit Kapila @ 2026-02-11 10:10 UTC (permalink / raw) To: shveta malik <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; [email protected] <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Doruk Yilmaz <[email protected]> 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. ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [Patch] add new parameter to pg_replication_origin_session_setup 2026-02-04 06:02 Re: [Patch] add new parameter to pg_replication_origin_session_setup shveta malik <[email protected]> 2026-02-11 10:10 ` Re: [Patch] add new parameter to pg_replication_origin_session_setup Amit Kapila <[email protected]> @ 2026-02-11 11:39 ` shveta malik <[email protected]> 2026-03-25 08:21 ` Re: [Patch] add new parameter to pg_replication_origin_session_setup Amit Kapila <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: shveta malik @ 2026-02-11 11:39 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; [email protected] <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Doruk Yilmaz <[email protected]>; shveta malik <[email protected]> On Wed, Feb 11, 2026 at 3:41 PM Amit Kapila <[email protected]> wrote: > > 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. 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. > > > > 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. ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [Patch] add new parameter to pg_replication_origin_session_setup 2026-02-04 06:02 Re: [Patch] add new parameter to pg_replication_origin_session_setup shveta malik <[email protected]> 2026-02-11 10:10 ` Re: [Patch] add new parameter to pg_replication_origin_session_setup Amit Kapila <[email protected]> 2026-02-11 11:39 ` Re: [Patch] add new parameter to pg_replication_origin_session_setup shveta malik <[email protected]> @ 2026-03-25 08:21 ` Amit Kapila <[email protected]> 2026-03-25 09:33 ` Re: [Patch] add new parameter to pg_replication_origin_session_setup shveta malik <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Amit Kapila @ 2026-03-25 08:21 UTC (permalink / raw) To: shveta malik <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; [email protected] <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Doruk Yilmaz <[email protected]> On Wed, Feb 11, 2026 at 5:09 PM shveta malik <[email protected]> wrote: > > On Wed, Feb 11, 2026 at 3:41 PM Amit Kapila <[email protected]> wrote: > > > > > > 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. > > 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. > I have tried to address both your suggestions in the attached. See, if this looks okay to you now? -- With Regards, Amit Kapila. Attachments: [application/octet-stream] v2-0001-Simplify-replorigin_session_setup.patch (4.3K, 2-v2-0001-Simplify-replorigin_session_setup.patch) download | inline diff: From 494b31e79dce2a08fb56cee2b6af521242be043f Mon Sep 17 00:00:00 2001 From: Amit Kapila <[email protected]> Date: Wed, 25 Mar 2026 13:42:52 +0530 Subject: [PATCH v3] Simplify replorigin_session_setup() --- src/backend/replication/logical/origin.c | 78 ++++++++++++++---------- 1 file changed, 46 insertions(+), 32 deletions(-) diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c index 26afd8f0af9..26c3725aa68 100644 --- a/src/backend/replication/logical/origin.c +++ b/src/backend/replication/logical/origin.c @@ -1186,55 +1186,69 @@ replorigin_session_setup(ReplOriginId node, int acquired_by) if (curstate->roident != node) continue; - else if (curstate->acquired_by != 0 && acquired_by == 0) + if (acquired_by == 0) { - ereport(ERROR, - (errcode(ERRCODE_OBJECT_IN_USE), - errmsg("replication origin with ID %d is already active for PID %d", - curstate->roident, curstate->acquired_by))); + /* With acquired_by == 0, we need the origin to be free */ + if (curstate->acquired_by != 0) + { + ereport(ERROR, + (errcode(ERRCODE_OBJECT_IN_USE), + errmsg("replication origin with ID %d is already active for PID %d", + curstate->roident, curstate->acquired_by))); + } + else if (curstate->refcount > 0) + { + /* + * The origin is in use, but PID is not recorded. This can + * happen if the process that originally acquired the origin + * exited without releasing it. To ensure correctness, other + * processes cannot acquire the origin until all processes + * currently using it have released it. + */ + ereport(ERROR, + (errcode(ERRCODE_OBJECT_IN_USE), + errmsg("replication origin with ID %d is already active in another process", + curstate->roident))); + } } - - else if (curstate->acquired_by != acquired_by) + else { - ereport(ERROR, - (errcode(ERRCODE_OBJECT_IN_USE), - errmsg("could not find replication state slot for replication origin with OID %u which was acquired by %d", - node, acquired_by))); + /* + * With acquired_by != 0, we need the origin to be active by the + * given PID + */ + 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))); + /* + * Here, it is okay to have refcount > 0 as more than one process + * can safely re-use the origin. + */ } - /* - * The origin is in use, but PID is not recorded. This can happen if - * the process that originally acquired the origin exited without - * releasing it. To ensure correctness, other processes cannot acquire - * the origin until all processes currently using it have released it. - */ - 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))); - /* ok, found slot */ session_replication_state = curstate; break; } - - if (session_replication_state == NULL && free_slot == -1) - ereport(ERROR, - (errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED), - errmsg("could not find free replication state slot for replication origin with ID %d", - node), - errhint("Increase \"max_active_replication_origins\" and try again."))); - else if (session_replication_state == NULL) + if (session_replication_state == NULL) { - if (acquired_by) + if (acquired_by != 0) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("cannot use PID %d for inactive replication origin with ID %d", acquired_by, node))); /* initialize new slot */ + if (free_slot == -1) + ereport(ERROR, + (errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED), + errmsg("could not find free replication state slot for replication origin with ID %d", + node), + errhint("Increase \"max_active_replication_origins\" and try again."))); + session_replication_state = &replication_states[free_slot]; Assert(!XLogRecPtrIsValid(session_replication_state->remote_lsn)); Assert(!XLogRecPtrIsValid(session_replication_state->local_lsn)); -- 2.52.0.windows.1 ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [Patch] add new parameter to pg_replication_origin_session_setup 2026-02-04 06:02 Re: [Patch] add new parameter to pg_replication_origin_session_setup shveta malik <[email protected]> 2026-02-11 10:10 ` Re: [Patch] add new parameter to pg_replication_origin_session_setup Amit Kapila <[email protected]> 2026-02-11 11:39 ` Re: [Patch] add new parameter to pg_replication_origin_session_setup shveta malik <[email protected]> 2026-03-25 08:21 ` Re: [Patch] add new parameter to pg_replication_origin_session_setup Amit Kapila <[email protected]> @ 2026-03-25 09:33 ` shveta malik <[email protected]> 2026-03-26 06:40 ` Re: [Patch] add new parameter to pg_replication_origin_session_setup Amit Kapila <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: shveta malik @ 2026-03-25 09:33 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; [email protected] <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Doruk Yilmaz <[email protected]>; shveta malik <[email protected]> On Wed, Mar 25, 2026 at 1:51 PM Amit Kapila <[email protected]> wrote: > > On Wed, Feb 11, 2026 at 5:09 PM shveta malik <[email protected]> wrote: > > > > On Wed, Feb 11, 2026 at 3:41 PM Amit Kapila <[email protected]> wrote: > > > > > > > > > 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. > > > > 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. > > > > I have tried to address both your suggestions in the attached. See, if > this looks okay to you now? > LGTM now, thanks! thanks Shveta ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [Patch] add new parameter to pg_replication_origin_session_setup 2026-02-04 06:02 Re: [Patch] add new parameter to pg_replication_origin_session_setup shveta malik <[email protected]> 2026-02-11 10:10 ` Re: [Patch] add new parameter to pg_replication_origin_session_setup Amit Kapila <[email protected]> 2026-02-11 11:39 ` Re: [Patch] add new parameter to pg_replication_origin_session_setup shveta malik <[email protected]> 2026-03-25 08:21 ` Re: [Patch] add new parameter to pg_replication_origin_session_setup Amit Kapila <[email protected]> 2026-03-25 09:33 ` Re: [Patch] add new parameter to pg_replication_origin_session_setup shveta malik <[email protected]> @ 2026-03-26 06:40 ` Amit Kapila <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Amit Kapila @ 2026-03-26 06:40 UTC (permalink / raw) To: shveta malik <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; [email protected] <[email protected]>; Zhijie Hou (Fujitsu) <[email protected]>; Doruk Yilmaz <[email protected]> On Wed, Mar 25, 2026 at 3:03 PM shveta malik <[email protected]> wrote: > > On Wed, Mar 25, 2026 at 1:51 PM Amit Kapila <[email protected]> wrote: > > > > On Wed, Feb 11, 2026 at 5:09 PM shveta malik <[email protected]> wrote: > > > > > > On Wed, Feb 11, 2026 at 3:41 PM Amit Kapila <[email protected]> wrote: > > > > > > > > > > > > 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. > > > > > > 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. > > > > > > > I have tried to address both your suggestions in the attached. See, if > > this looks okay to you now? > > > > LGTM now, thanks! > Pushed. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-03-26 06:40 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-02-04 06:02 Re: [Patch] add new parameter to pg_replication_origin_session_setup shveta malik <[email protected]> 2026-02-11 10:10 ` Amit Kapila <[email protected]> 2026-02-11 11:39 ` shveta malik <[email protected]> 2026-03-25 08:21 ` Amit Kapila <[email protected]> 2026-03-25 09:33 ` shveta malik <[email protected]> 2026-03-26 06:40 ` Amit Kapila <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox