Обсуждение: Refactor replication origin state reset helpers

On Wed, Dec 24, 2025 at 6:58 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
> Hi Hacker,
>
> While reviewing patches [1] and [2], I noticed some duplicate code of clearing replication origin states, I am
proposinga small patch that removes the duplicate code blocks by introducing a couple helper functions. No functional
changeat all. 
>

The new functions bring together the global variables that need to be
reset under certain conditions. The functions will help not to miss
resetting some variable. However, this can be a mild backpatching
pain. So, I am +.5 on this.

If we go this route, we at least need to declare the new functions as
static inline and move them to a header file instead of .c file.

Further, does it make sense to put together all the state variables
into a single structure?

It's also quite easy to confuse between these functions and
replorigin_session_reset(). It's not clear where the boundaries of the
latter end and where those of the new ones start. I think the latter
deals with the shared memory structures while the new ones deal with
the backend local state. And then there's replorigin_reset() which
adds to the confusion. That function doesn't call
replorigin_session_reset() which the other two callers of
replorigin_session_clear_state() call. Why? I think there is more to
clean here than what's in the patch. That doesn't mean that we cannot
accept this patch without larger cleanup, but it should not add to the
existing confusion.

--
Best Wishes,
Ashutosh Bapat

> On Dec 25, 2025, at 13:05, Chao Li <li.evan.chao@gmail.com> wrote:
>
> With these broader changes, this patch is no longer trivial, so I just added it to CF:

https://commitfest.postgresql.org/patch/6345/

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On 2025-Dec-24, Ashutosh Bapat wrote:

> If we go this route, we at least need to declare the new functions as
> static inline and move them to a header file instead of .c file.

Hmm, why would we make them static inline instead of standard (extern)
functions?  We use static inline functions when we want to avoid the
overhead of a function call in a hot code path, but I doubt that's the
case here.  Am I mistaken on this?

> Further, does it make sense to put together all the state variables
> into a single structure?

Yeah -- keeping the threaded-backend project in mind, moving them to a
single struct seems to make sense.  I think it's a separate patch though
because it'd be more invasive than Chao's initial patch, as those
variables are used in many places.

> It's also quite easy to confuse between these functions and
> replorigin_session_reset(). It's not clear where the boundaries of the
> latter end and where those of the new ones start. I think the latter
> deals with the shared memory structures while the new ones deal with
> the backend local state. And then there's replorigin_reset() which
> adds to the confusion. That function doesn't call
> replorigin_session_reset() which the other two callers of
> replorigin_session_clear_state() call. Why? I think there is more to
> clean here than what's in the patch. That doesn't mean that we cannot
> accept this patch without larger cleanup, but it should not add to the
> existing confusion.

Good points.  A decrease in the total quantity of cruft would be a good
outcome of a patch in this area.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Update: super-fast reaction on the Postgres bugs mailing list. The report
was acknowledged [...], and a fix is under discussion.
The wonders of open-source !"
             https://twitter.com/gunnarmorling/status/1596080409259003906

On 2025-Dec-29, Álvaro Herrera wrote:

> On 2025-Dec-24, Ashutosh Bapat wrote:

> > It's also quite easy to confuse between these functions and
> > replorigin_session_reset(). It's not clear where the boundaries of the
> > latter end and where those of the new ones start. I think the latter
> > deals with the shared memory structures while the new ones deal with
> > the backend local state. And then there's replorigin_reset() which
> > adds to the confusion. That function doesn't call
> > replorigin_session_reset() which the other two callers of
> > replorigin_session_clear_state() call. Why? I think there is more to
> > clean here than what's in the patch. That doesn't mean that we cannot
> > accept this patch without larger cleanup, but it should not add to the
> > existing confusion.

I think we should just rename the worker.c function to have a less
generic-sounding name (so that it becomes more obvious that it's a
worker.c-specific function); have both replorigin_session_clear_state()
and replorigin_xact_clear_state() be a single function, with a boolean
flag to indicate whether to reset replorigin_session_origin or not; and
have replorigin_session_reset() call replorigin_session_clear_state()
internally rather than require every single of its callers do it
separately, which IMO makes no sense.  With these three changes I think
the code would become quite clear.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
Thou shalt check the array bounds of all strings (indeed, all arrays), for
surely where thou typest "foo" someone someday shall type
"supercalifragilisticexpialidocious" (5th Commandment for C programmers)

On Mon, Dec 29, 2025 at 8:14 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2025-Dec-24, Ashutosh Bapat wrote:
>
> > If we go this route, we at least need to declare the new functions as
> > static inline and move them to a header file instead of .c file.
>
> Hmm, why would we make them static inline instead of standard (extern)
> functions?  We use static inline functions when we want to avoid the
> overhead of a function call in a hot code path, but I doubt that's the
> case here.  Am I mistaken on this?
>

I wasn't aware that we are using static inline only in hot code paths.
Looking around I see most of the static inline functions are from
modules which are used in hot code paths. So, yeah that seems to be
the convention. I also see some exceptions like those in
basebackup_sink.h - I don't think all of those are used in hot code
paths.

In this case, we are moving three assignments into their own
functions. CPU instructions to call extern functions will be
significant compared to CPU instructions for those assignments. static
inline functions, OTOH, would have similar performance as the existing
code while providing modularization. If you feel that's not a good
enough reason, I am ok keeping them extern.

> > Further, does it make sense to put together all the state variables
> > into a single structure?
>
> Yeah -- keeping the threaded-backend project in mind, moving them to a
> single struct seems to make sense.  I think it's a separate patch though
> because it'd be more invasive than Chao's initial patch, as those
> variables are used in many places.
>

Agreed.

--
Best Wishes,
Ashutosh Bapat

On Mon, Dec 29, 2025 at 11:17 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
> On Tue, Dec 30, 2025 at 1:07 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>
>>
>> On Tue, Dec 30, 2025 at 12:48 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>>>
>>> On Mon, Dec 29, 2025 at 8:14 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>>> >
>>> > On 2025-Dec-24, Ashutosh Bapat wrote:
>>> >
>>> > > If we go this route, we at least need to declare the new functions as
>>> > > static inline and move them to a header file instead of .c file.
>>> >
>>> > Hmm, why would we make them static inline instead of standard (extern)
>>> > functions?  We use static inline functions when we want to avoid the
>>> > overhead of a function call in a hot code path, but I doubt that's the
>>> > case here.  Am I mistaken on this?
>>> >
>>>
>>> I wasn't aware that we are using static inline only in hot code paths.
>>> Looking around I see most of the static inline functions are from
>>> modules which are used in hot code paths. So, yeah that seems to be
>>> the convention. I also see some exceptions like those in
>>> basebackup_sink.h - I don't think all of those are used in hot code
>>> paths.
>>>
>>> In this case, we are moving three assignments into their own
>>> functions. CPU instructions to call extern functions will be
>>> significant compared to CPU instructions for those assignments. static
>>> inline functions, OTOH, would have similar performance as the existing
>>> code while providing modularization. If you feel that's not a good
>>> enough reason, I am ok keeping them extern.
>>>
>>> > > Further, does it make sense to put together all the state variables
>>> > > into a single structure?
>>> >
>>> > Yeah -- keeping the threaded-backend project in mind, moving them to a
>>> > single struct seems to make sense.  I think it's a separate patch though
>>> > because it'd be more invasive than Chao's initial patch, as those
>>> > variables are used in many places.
>>> >
>>
>>
>> Attached v3 patch set. Comparing to v2, the changes are:
>>
>> 0001:
>> * Combine the two cleanup functions into one and control them by a bool flag.
>> * Change the helper function to be extern.
>> * Move out cleanup from reset function.
>>
>> 0002: Consolidate replication origin session globals into a single state struct.
>
>
> Fixed a bug in v4.
>

I've reviewed both patches. Here are some comments:

0001 patch:

+/*
+ * Clear session replication origin state.
+ *
+ * If xact_only is true, only clear the per-transaction state.
+ */
+void
+replorigin_session_clear_state(bool xact_only)
+{
+   replorigin_session_origin_lsn = InvalidXLogRecPtr;
+   replorigin_session_origin_timestamp = 0;
+   if (!xact_only)
+       replorigin_session_origin = InvalidRepOriginId;
+}

Given that we already have session_replication_state, I am concerned
that the name replorigin_session_clear_state creates ambiguity. Could
we rename it to something like replorigin_session_clear()?

Additionally, I feel that the term "per-transaction state" in the
comments does not accurately describe these two fields. How about
renaming the xact_only parameter to clear_origin? This would make it
explicit that setting the flag to true clears
replorigin_session_origin as well.

0002 patch:

+typedef struct RepOriginSessionState
+{
+   RepOriginId origin;
+   XLogRecPtr  origin_lsn;
+   TimestampTz origin_timestamp;
+}          RepOriginSessionState;
+
+extern PGDLLIMPORT RepOriginSessionState replorigin_session_state;

replorigin_session_state is quite confusable with the existing
session_replication_state. Given that these values are used to add the
additional information to the transaction, how about the name
something like "replorigin_xact_state" or "replorigin_xact_origin"?

--
+RepOriginSessionState replorigin_session_state = {
+   InvalidRepOriginId, InvalidXLogRecPtr, 0
+};

I think using designated initializers here would be better for
readability and robustness against future struct changes.

Regards,


--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

> On Jan 7, 2026, at 15:21, Chao Li <li.evan.chao@gmail.com> wrote:
>
>
> On Wed, Jan 7, 2026 at 8:30 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Mon, Dec 29, 2025 at 11:17 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >
> > On Tue, Dec 30, 2025 at 1:07 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >>
> >>
> >> On Tue, Dec 30, 2025 at 12:48 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
> >>>
> >>> On Mon, Dec 29, 2025 at 8:14 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> >>> >
> >>> > On 2025-Dec-24, Ashutosh Bapat wrote:
> >>> >
> >>> > > If we go this route, we at least need to declare the new functions as
> >>> > > static inline and move them to a header file instead of .c file.
> >>> >
> >>> > Hmm, why would we make them static inline instead of standard (extern)
> >>> > functions?  We use static inline functions when we want to avoid the
> >>> > overhead of a function call in a hot code path, but I doubt that's the
> >>> > case here.  Am I mistaken on this?
> >>> >
> >>>
> >>> I wasn't aware that we are using static inline only in hot code paths.
> >>> Looking around I see most of the static inline functions are from
> >>> modules which are used in hot code paths. So, yeah that seems to be
> >>> the convention. I also see some exceptions like those in
> >>> basebackup_sink.h - I don't think all of those are used in hot code
> >>> paths.
> >>>
> >>> In this case, we are moving three assignments into their own
> >>> functions. CPU instructions to call extern functions will be
> >>> significant compared to CPU instructions for those assignments. static
> >>> inline functions, OTOH, would have similar performance as the existing
> >>> code while providing modularization. If you feel that's not a good
> >>> enough reason, I am ok keeping them extern.
> >>>
> >>> > > Further, does it make sense to put together all the state variables
> >>> > > into a single structure?
> >>> >
> >>> > Yeah -- keeping the threaded-backend project in mind, moving them to a
> >>> > single struct seems to make sense.  I think it's a separate patch though
> >>> > because it'd be more invasive than Chao's initial patch, as those
> >>> > variables are used in many places.
> >>> >
> >>
> >>
> >> Attached v3 patch set. Comparing to v2, the changes are:
> >>
> >> 0001:
> >> * Combine the two cleanup functions into one and control them by a bool flag.
> >> * Change the helper function to be extern.
> >> * Move out cleanup from reset function.
> >>
> >> 0002: Consolidate replication origin session globals into a single state struct.
> >
> >
> > Fixed a bug in v4.
> >
>
> I've reviewed both patches. Here are some comments:
>
> Thank you very much for the review.
>
> 0001 patch:
>
> +/*
> + * Clear session replication origin state.
> + *
> + * If xact_only is true, only clear the per-transaction state.
> + */
> +void
> +replorigin_session_clear_state(bool xact_only)
> +{
> +   replorigin_session_origin_lsn = InvalidXLogRecPtr;
> +   replorigin_session_origin_timestamp = 0;
> +   if (!xact_only)
> +       replorigin_session_origin = InvalidRepOriginId;
> +}
>
> Given that we already have session_replication_state, I am concerned
> that the name replorigin_session_clear_state creates ambiguity. Could
> we rename it to something like replorigin_session_clear()?
>
> Agreed.
>  Additionally, I feel that the term "per-transaction state" in the
> comments does not accurately describe these two fields. How about
> renaming the xact_only parameter to clear_origin? This would make it
> explicit that setting the flag to true clears
> replorigin_session_origin as well.
>
> Fixed.
>
> 0002 patch:
>
> +typedef struct RepOriginSessionState
> +{
> +   RepOriginId origin;
> +   XLogRecPtr  origin_lsn;
> +   TimestampTz origin_timestamp;
> +}          RepOriginSessionState;
> +
> +extern PGDLLIMPORT RepOriginSessionState replorigin_session_state;
>
> replorigin_session_state is quite confusable with the existing
> session_replication_state. Given that these values are used to add the
> additional information to the transaction, how about the name
> something like "replorigin_xact_state" or "replorigin_xact_origin"?
>
>
> Okay, I take replorigin_xact_state, and I renamed the structure name as well.
>  Given replorigin_session_clear() only sets members of RepOriginXactState, I also renamed it to
replorigin_xact_clear().
>
> --
> +RepOriginSessionState replorigin_session_state = {
> +   InvalidRepOriginId, InvalidXLogRecPtr, 0
> +};
>
> I think using designated initializers here would be better for
> readability and robustness against future struct changes.
>
>  Fixed.
>
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
<v5-0001-Refactor-replication-origin-state-reset-helpers.patch><v5-0002-Consolidate-replication-origin-session-globals-in.patch>

The CF CI failed a test case, but I don’t think that’s mine. I just did a self-review for v5 and couldn’t find any
logicchange. So I would guess the CI failure was intermittent. I don’t know how to rerun the CI test other than bumping
thepatch version and re-posting. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Wed, Jan 7, 2026 at 5:15 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
> On Wed, Jan 7, 2026 at 4:49 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>
>>
>>
<v5-0001-Refactor-replication-origin-state-reset-helpers.patch><v5-0002-Consolidate-replication-origin-session-globals-in.patch>
>>
>> The CF CI failed a test case, but I don’t think that’s mine. I just did a self-review for v5 and couldn’t find any
logicchange. So I would guess the CI failure was intermittent. I don’t know how to rerun the CI test other than bumping
thepatch version and re-posting. 
>
>
> V6 is only a rebase, nothing new.

Thank you for updating the patches.

I've made some cosmetic changes to both patches (comments and the
commit messages). Please review them and let me know what you think.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

> On Jan 8, 2026, at 09:46, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Jan 7, 2026 at 5:15 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>
>>
>> On Wed, Jan 7, 2026 at 4:49 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>>
>>>
>>>
<v5-0001-Refactor-replication-origin-state-reset-helpers.patch><v5-0002-Consolidate-replication-origin-session-globals-in.patch>
>>>
>>> The CF CI failed a test case, but I don’t think that’s mine. I just did a self-review for v5 and couldn’t find any
logicchange. So I would guess the CI failure was intermittent. I don’t know how to rerun the CI test other than bumping
thepatch version and re-posting. 
>>
>>
>> V6 is only a rebase, nothing new.
>
> Thank you for updating the patches.
>
> I've made some cosmetic changes to both patches (comments and the
> commit messages). Please review them and let me know what you think.
>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com
>
<v7-0002-Consolidate-replication-origin-session-globals-in.patch><v7-0001-Refactor-replication-origin-state-reset-helpers.patch>

Only issue:
```
-/* external variables */
-RepOriginId replorigin_session_origin = InvalidRepOriginId; /* assumed identity */
-XLogRecPtr    replorigin_session_origin_lsn = InvalidXLogRecPtr;
-TimestampTz replorigin_session_origin_timestamp = 0;
+/* Global variable for per-transaction replication origin state */
+RepOriginXactState replorigin_xact_state = {
+    .origin = InvalidRepOriginId,    /* assumed identify */
+    .origin_lsn = InvalidXLogRecPtr,
+    .origin_timestamp = 0
+};
```

“identify" should be “identity".

And a nitpick:
```
+/* Global variable for per-transaction replication origin state */
```

Maybe change “for” to “holding”. But this is really a nitpick, it’s up to you.

Otherwise v7 looks good. Commit messages are now neater.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Hi Masahiko,

Thanks for updating the patches. Here are some more comments.

On Thu, Jan 8, 2026 at 7:17 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Jan 7, 2026 at 5:15 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
> I've made some cosmetic changes to both patches (comments and the
> commit messages). Please review them and let me know what you think.

0001
-------

+/*
+ * Clear session replication origin state.
+ *
+ * replorigin_session_origin is also cleared if clear_origin is set.
+ */
+void
+replorigin_session_clear(bool clear_origin)
+{
+ replorigin_session_origin_lsn = InvalidXLogRecPtr;
+ replorigin_session_origin_timestamp = 0;
+ if (clear_origin)
+ replorigin_session_origin = InvalidRepOriginId;
+}

All the other replorigin_session_* functions deal with
session_replication_state, but this function does not deal with it. I
see that in the next patch this function has been renamed as
replorigin_xact_clear() which seems more appropriate. Do you intend to
squash these two patches when committing?

@@ -1482,8 +1493,8 @@ pg_replication_origin_xact_reset(PG_FUNCTION_ARGS)
{
replorigin_check_prerequisites(true, false);
- replorigin_session_origin_lsn = InvalidXLogRecPtr;
- replorigin_session_origin_timestamp = 0;
+ /* Clear only origin_lsn and origin_timestamp */
+ replorigin_session_clear(false);

The comment can explain why we are not clearing
replorigin_session_origin here. Something like "This function is
cancel the effects of pg_replication_origin_xact_setup(), which only
sets origin_lsn and origin_timestamp, so we only clear those two
fields here.".

Next comment does not apply to this patch, but the inconsistency I am
speaking about becomes apparent now. This function resets the state
setup by pg_replication_origin_xact_setup(), which checks for
session_replication_state being non-NULL. So I expected
pg_replication_origin_xact_reset() also to check for the same
condition or at least Assert it. Why is it not doing so?

0002
-------
- replorigin = (replorigin_session_origin != InvalidRepOriginId &&
- replorigin_session_origin != DoNotReplicateId);
+ replorigin = (replorigin_xact_state.origin != InvalidRepOriginId &&
+ replorigin_xact_state.origin != DoNotReplicateId);

There's another small deduplication opportunity here. Define function
replorigin_xact_origin_isvalid() to check these two conditions and use
that function here and in other places like
RecordTransactionCommitPrepared(). I would go as far as making the
function static inline, and use it instead of replorigin variable,
whose name is certainly misleading - it doesn't talk about transaction
at all. With static inline there, optimizer may be able to eliminate
the multiple function call overhead.

/*
* Record commit timestamp. The value comes from plain commit timestamp
* if replorigin is not enabled, or replorigin already set a value for us
- * in replorigin_session_origin_timestamp otherwise.
+ * in replorigin_xact_state.origin_timestamp otherwise.

suggestion "Record commit timestamp. Use one in replorigin_xact_state
if set, otherwise use plain commit timestamp.". This reads better and
is closer to the code.". If you agree, please change other similar
comments too.

@@ -5928,12 +5928,12 @@ XactLogCommitRecord(TimestampTz commit_time,
}
/* dump transaction origin information */
- if (replorigin_session_origin != InvalidRepOriginId)
+ if (replorigin_xact_state.origin != InvalidRepOriginId)
{

* Dump transaction origin information. We need this during recovery to
* update the replication origin progress.
*/
- if (replorigin_session_origin != InvalidRepOriginId)
+ if (replorigin_xact_state.origin != InvalidRepOriginId)
{

/* followed by the record's origin, if any */
if ((curinsert_flags & XLOG_INCLUDE_ORIGIN) &&
- replorigin_session_origin != InvalidRepOriginId)
+ replorigin_xact_state.origin != InvalidRepOriginId)

Not a change in this patch but the refactoring makes it more visible.
What about the case of replorigin_session_origin == DoNotReplicateId?
Should there be a comment explaining why it's excluded here?

replorigin_session_setup(originid, MyLogicalRepWorker->leader_pid);
- replorigin_session_origin = originid;
+ replorigin_xact_state.origin = originid;

replorigin_session_setup() is always followed by
replorigin_xact_state.origin assignment. abort/commit handlers set the
other two members. Do we want to create replorigin_xact_set_origin()
and replorigin_xact_set_lsn_timestamp() functions to encapsulate these
assignments? Those two will serve as counterparts to
replorigin_xact_clear(). Or would that be an overkill?

* successfully and that we don't need to do so again. In combination with
- * setting up replorigin_session_origin_lsn and replorigin_session_origin
+ * setting up replorigin_xact_state.origin_lsn and replorigin_xact_state.origin

I think just replorigin_xact_state should suffice for the sake of brevity.

static void
on_exit_clear_state(int code, Datum arg)
{
- replorigin_session_clear(true);
+ replorigin_xact_clear(true);

The prologue still states clear origin, but it should mention
transaction state instead.
}

--
Best Wishes,
Ashutosh Bapat

> On Jan 8, 2026, at 17:44, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>
> Hi Masahiko,
>
> Thanks for updating the patches. Here are some more comments.
>
> On Thu, Jan 8, 2026 at 7:17 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>
>> On Wed, Jan 7, 2026 at 5:15 PM Chao Li <li.evan.chao@gmail.com> wrote:
>>
>> I've made some cosmetic changes to both patches (comments and the
>> commit messages). Please review them and let me know what you think.
>
> 0001
> -------
>
> +/*
> + * Clear session replication origin state.
> + *
> + * replorigin_session_origin is also cleared if clear_origin is set.
> + */
> +void
> +replorigin_session_clear(bool clear_origin)
> +{
> + replorigin_session_origin_lsn = InvalidXLogRecPtr;
> + replorigin_session_origin_timestamp = 0;
> + if (clear_origin)
> + replorigin_session_origin = InvalidRepOriginId;
> +}
>
> All the other replorigin_session_* functions deal with
> session_replication_state, but this function does not deal with it. I
> see that in the next patch this function has been renamed as
> replorigin_xact_clear() which seems more appropriate. Do you intend to
> squash these two patches when committing?
>
> @@ -1482,8 +1493,8 @@ pg_replication_origin_xact_reset(PG_FUNCTION_ARGS)
> {
> replorigin_check_prerequisites(true, false);
> - replorigin_session_origin_lsn = InvalidXLogRecPtr;
> - replorigin_session_origin_timestamp = 0;
> + /* Clear only origin_lsn and origin_timestamp */
> + replorigin_session_clear(false);
>
> The comment can explain why we are not clearing
> replorigin_session_origin here. Something like "This function is
> cancel the effects of pg_replication_origin_xact_setup(), which only
> sets origin_lsn and origin_timestamp, so we only clear those two
> fields here.".
>
> Next comment does not apply to this patch, but the inconsistency I am
> speaking about becomes apparent now. This function resets the state
> setup by pg_replication_origin_xact_setup(), which checks for
> session_replication_state being non-NULL. So I expected
> pg_replication_origin_xact_reset() also to check for the same
> condition or at least Assert it. Why is it not doing so?

Hi Ashutosh,

Thanks for your follow-up review.

IMO, we don’t have to no more on 0001. As 0002 is a big refactoring, where we group the 3 global variables into a
structureand rename the help function, 0001 can be considered as a preparation commit that does some simple cleanup. So
that,we can put main focus on the real refactoring in 0002. 

>
> 0002
> -------
> - replorigin = (replorigin_session_origin != InvalidRepOriginId &&
> - replorigin_session_origin != DoNotReplicateId);
> + replorigin = (replorigin_xact_state.origin != InvalidRepOriginId &&
> + replorigin_xact_state.origin != DoNotReplicateId);
>
> There's another small deduplication opportunity here. Define function
> replorigin_xact_origin_isvalid() to check these two conditions and use
> that function here and in other places like
> RecordTransactionCommitPrepared(). I would go as far as making the
> function static inline, and use it instead of replorigin variable,
> whose name is certainly misleading - it doesn't talk about transaction
> at all. With static inline there, optimizer may be able to eliminate
> the multiple function call overhead.

Thanks for pointing out the idea. I can look into that tomorrow.

>
> /*
> * Record commit timestamp. The value comes from plain commit timestamp
> * if replorigin is not enabled, or replorigin already set a value for us
> - * in replorigin_session_origin_timestamp otherwise.
> + * in replorigin_xact_state.origin_timestamp otherwise.
>
> suggestion "Record commit timestamp. Use one in replorigin_xact_state
> if set, otherwise use plain commit timestamp.". This reads better and
> is closer to the code.". If you agree, please change other similar
> comments too.
>
> @@ -5928,12 +5928,12 @@ XactLogCommitRecord(TimestampTz commit_time,
> }
> /* dump transaction origin information */
> - if (replorigin_session_origin != InvalidRepOriginId)
> + if (replorigin_xact_state.origin != InvalidRepOriginId)
> {
>
> * Dump transaction origin information. We need this during recovery to
> * update the replication origin progress.
> */
> - if (replorigin_session_origin != InvalidRepOriginId)
> + if (replorigin_xact_state.origin != InvalidRepOriginId)
> {
>
> /* followed by the record's origin, if any */
> if ((curinsert_flags & XLOG_INCLUDE_ORIGIN) &&
> - replorigin_session_origin != InvalidRepOriginId)
> + replorigin_xact_state.origin != InvalidRepOriginId)
>
> Not a change in this patch but the refactoring makes it more visible.
> What about the case of replorigin_session_origin == DoNotReplicateId?
> Should there be a comment explaining why it's excluded here?
>
> replorigin_session_setup(originid, MyLogicalRepWorker->leader_pid);
> - replorigin_session_origin = originid;
> + replorigin_xact_state.origin = originid;
>
> replorigin_session_setup() is always followed by
> replorigin_xact_state.origin assignment. abort/commit handlers set the
> other two members. Do we want to create replorigin_xact_set_origin()
> and replorigin_xact_set_lsn_timestamp() functions to encapsulate these
> assignments? Those two will serve as counterparts to
> replorigin_xact_clear(). Or would that be an overkill?
>
> * successfully and that we don't need to do so again. In combination with
> - * setting up replorigin_session_origin_lsn and replorigin_session_origin
> + * setting up replorigin_xact_state.origin_lsn and replorigin_xact_state.origin
>
> I think just replorigin_xact_state should suffice for the sake of brevity.
>
> static void
> on_exit_clear_state(int code, Datum arg)
> {
> - replorigin_session_clear(true);
> + replorigin_xact_clear(true);
>
> The prologue still states clear origin, but it should mention
> transaction state instead.
> }
>

I will look into these as well tomorrow.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Thu, Jan 8, 2026 at 6:32 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Jan 8, 2026, at 17:44, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > Hi Masahiko,
> >
> > Thanks for updating the patches. Here are some more comments.
> >
> > On Thu, Jan 8, 2026 at 7:17 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >>
> >> On Wed, Jan 7, 2026 at 5:15 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >>
> >> I've made some cosmetic changes to both patches (comments and the
> >> commit messages). Please review them and let me know what you think.
> >
> > 0001
> > -------
> >
> > +/*
> > + * Clear session replication origin state.
> > + *
> > + * replorigin_session_origin is also cleared if clear_origin is set.
> > + */
> > +void
> > +replorigin_session_clear(bool clear_origin)
> > +{
> > + replorigin_session_origin_lsn = InvalidXLogRecPtr;
> > + replorigin_session_origin_timestamp = 0;
> > + if (clear_origin)
> > + replorigin_session_origin = InvalidRepOriginId;
> > +}
> >
> > All the other replorigin_session_* functions deal with
> > session_replication_state, but this function does not deal with it. I
> > see that in the next patch this function has been renamed as
> > replorigin_xact_clear() which seems more appropriate. Do you intend to
> > squash these two patches when committing?
> >
> > @@ -1482,8 +1493,8 @@ pg_replication_origin_xact_reset(PG_FUNCTION_ARGS)
> > {
> > replorigin_check_prerequisites(true, false);
> > - replorigin_session_origin_lsn = InvalidXLogRecPtr;
> > - replorigin_session_origin_timestamp = 0;
> > + /* Clear only origin_lsn and origin_timestamp */
> > + replorigin_session_clear(false);
> >
> > The comment can explain why we are not clearing
> > replorigin_session_origin here. Something like "This function is
> > cancel the effects of pg_replication_origin_xact_setup(), which only
> > sets origin_lsn and origin_timestamp, so we only clear those two
> > fields here.".
> >
> > Next comment does not apply to this patch, but the inconsistency I am
> > speaking about becomes apparent now. This function resets the state
> > setup by pg_replication_origin_xact_setup(), which checks for
> > session_replication_state being non-NULL. So I expected
> > pg_replication_origin_xact_reset() also to check for the same
> > condition or at least Assert it. Why is it not doing so?
>
> Hi Ashutosh,
>
> Thanks for your follow-up review.
>
> IMO, we don’t have to no more on 0001. As 0002 is a big refactoring, where we group the 3 global variables into a
structureand rename the help function, 0001 can be considered as a preparation commit that does some simple cleanup. So
that,we can put main focus on the real refactoring in 0002. 
>

In that case at least the function name change should be part of 0001.

--
Best Wishes,
Ashutosh Bapat

On Thu, Jan 8, 2026 at 10:50 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> On Jan 8, 2026, at 17:44, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>
> Hi Masahiko,
>
> Thanks for updating the patches. Here are some more comments.
>
> On Thu, Jan 8, 2026 at 7:17 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
>
> On Wed, Jan 7, 2026 at 5:15 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
> I've made some cosmetic changes to both patches (comments and the
> commit messages). Please review them and let me know what you think.
>
>
> 0001
> -------
>
> +/*
> + * Clear session replication origin state.
> + *
> + * replorigin_session_origin is also cleared if clear_origin is set.
> + */
> +void
> +replorigin_session_clear(bool clear_origin)
> +{
> + replorigin_session_origin_lsn = InvalidXLogRecPtr;
> + replorigin_session_origin_timestamp = 0;
> + if (clear_origin)
> + replorigin_session_origin = InvalidRepOriginId;
> +}
>
> All the other replorigin_session_* functions deal with
> session_replication_state, but this function does not deal with it. I
> see that in the next patch this function has been renamed as
> replorigin_xact_clear() which seems more appropriate. Do you intend to
> squash these two patches when committing?
>
>
> Renamed the help function to the same name as in 0002.
>
>
> @@ -1482,8 +1493,8 @@ pg_replication_origin_xact_reset(PG_FUNCTION_ARGS)
> {
> replorigin_check_prerequisites(true, false);
> - replorigin_session_origin_lsn = InvalidXLogRecPtr;
> - replorigin_session_origin_timestamp = 0;
> + /* Clear only origin_lsn and origin_timestamp */
> + replorigin_session_clear(false);
>
> The comment can explain why we are not clearing
> replorigin_session_origin here. Something like "This function is
> cancel the effects of pg_replication_origin_xact_setup(), which only
> sets origin_lsn and origin_timestamp, so we only clear those two
> fields here.".
>
>
> Updated the comment in 0001.
>
>
> Next comment does not apply to this patch, but the inconsistency I am
> speaking about becomes apparent now. This function resets the state
> setup by pg_replication_origin_xact_setup(), which checks for
> session_replication_state being non-NULL. So I expected
> pg_replication_origin_xact_reset() also to check for the same
> condition or at least Assert it. Why is it not doing so?
>
>
> Added the same check in 0002.
>
>
> 0002
> -------
> - replorigin = (replorigin_session_origin != InvalidRepOriginId &&
> - replorigin_session_origin != DoNotReplicateId);
> + replorigin = (replorigin_xact_state.origin != InvalidRepOriginId &&
> + replorigin_xact_state.origin != DoNotReplicateId);
>
> There's another small deduplication opportunity here. Define function
> replorigin_xact_origin_isvalid() to check these two conditions and use
> that function here and in other places like
> RecordTransactionCommitPrepared(). I would go as far as making the
> function static inline, and use it instead of replorigin variable,
> whose name is certainly misleading - it doesn't talk about transaction
> at all. With static inline there, optimizer may be able to eliminate
> the multiple function call overhead.
>
>
> Good point. Added replorigin_xact_origin_isvalid() in 0002 as you suggested.
>
>
> /*
> * Record commit timestamp. The value comes from plain commit timestamp
> * if replorigin is not enabled, or replorigin already set a value for us
> - * in replorigin_session_origin_timestamp otherwise.
> + * in replorigin_xact_state.origin_timestamp otherwise.
>
> suggestion "Record commit timestamp. Use one in replorigin_xact_state
> if set, otherwise use plain commit timestamp.". This reads better and
> is closer to the code.". If you agree, please change other similar
> comments too.
>
>
> Agreed. Updated the comment in both places.
>
>
> @@ -5928,12 +5928,12 @@ XactLogCommitRecord(TimestampTz commit_time,
> }
> /* dump transaction origin information */
> - if (replorigin_session_origin != InvalidRepOriginId)
> + if (replorigin_xact_state.origin != InvalidRepOriginId)
> {
>
> * Dump transaction origin information. We need this during recovery to
> * update the replication origin progress.
> */
> - if (replorigin_session_origin != InvalidRepOriginId)
> + if (replorigin_xact_state.origin != InvalidRepOriginId)
> {
>
> /* followed by the record's origin, if any */
> if ((curinsert_flags & XLOG_INCLUDE_ORIGIN) &&
> - replorigin_session_origin != InvalidRepOriginId)
> + replorigin_xact_state.origin != InvalidRepOriginId)
>
> Not a change in this patch but the refactoring makes it more visible.
> What about the case of replorigin_session_origin == DoNotReplicateId?
> Should there be a comment explaining why it's excluded here?
>
>
> Added a comment to the 3 places.
>
> replorigin_session_setup(originid, MyLogicalRepWorker->leader_pid);
> - replorigin_session_origin = originid;
> + replorigin_xact_state.origin = originid;
>
> replorigin_session_setup() is always followed by
> replorigin_xact_state.origin assignment. abort/commit handlers set the
> other two members. Do we want to create replorigin_xact_set_origin()
> and replorigin_xact_set_lsn_timestamp() functions to encapsulate these
> assignments? Those two will serve as counterparts to
> replorigin_xact_clear(). Or would that be an overkill?
>
>
> Actually, when I added the new structure, I had the idea of adding replorigin_xact_set_origin() and
replorigin_xact_set_lsn_timestamp()as I saw a lot of duplicate code for the assignment. But I hesitated to do that
becauseI worried people might consider that’s too much. 
>
> Given you raised the same idea, let me add the helpers as static inline. We now have 3 static inline helpers, I guess
wehave no reason to leave replorigin_xact_clear() alone as external. So I made it static inline as well. 
>
>
> * successfully and that we don't need to do so again. In combination with
> - * setting up replorigin_session_origin_lsn and replorigin_session_origin
> + * setting up replorigin_xact_state.origin_lsn and replorigin_xact_state.origin
>
> I think just replorigin_xact_state should suffice for the sake of brevity.
>
>
> Updated the comment to eliminate the duplication.
>
>
> static void
> on_exit_clear_state(int code, Datum arg)
> {
> - replorigin_session_clear(true);
> + replorigin_xact_clear(true);
>
> The prologue still states clear origin, but it should mention
> transaction state instead.
> }
>
>
> Good catch. Updated the header comment.
>
> All comments are addressed in v8.

Thank you for updating the patch!

The 0001 patch looks good to me.

I have some comments on the 0002 patch:

-   replorigin = (replorigin_session_origin != InvalidRepOriginId &&
-                 replorigin_session_origin != DoNotReplicateId);
+   replorigin = replorigin_xact_origin_isvalid();

I'm not sure it's a good refactoring TBH. While it can remove some
lines, we still need to check if replorigin_xact_state.origin !=
InvalidRepOriginId at several places. For example,

Datum
pg_replication_origin_session_is_setup(PG_FUNCTION_ARGS)
{
    replorigin_check_prerequisites(false, false);

    PG_RETURN_BOOL(replorigin_xact_state.origin != InvalidRepOriginId);
}

and

    /*
     * Dump transaction origin information. We need this during recovery to
     * update the replication origin progress.
     *
     * Note that DoNotReplicateId is intentionally excluded here.
     */
    if (replorigin_xact_state.origin != InvalidRepOriginId)

The readers might be confused why we cannot use
replorigin_xact_origin_isvalid() here in spite of its function name,
replorigin_xact_origin_isvalid, sounds quite suitable. The comment
mentions the fact that DoNotReplicated is excluded but doesn't mention
why.

Also, even if we want this kind of change, it should be implemented in
a separate patch because it's not related to the main idea of
consolidating the replication origin global variables.

---
-    * Record commit timestamp.  The value comes from plain commit timestamp
-    * if replorigin is not enabled, or replorigin already set a value for us
-    * in replorigin_session_origin_timestamp otherwise.
+    * Record commit timestamp. Use one in replorigin_xact_state if set,
+    * otherwise use plain commit timestamp.

and

-        * Record commit timestamp.  The value comes from plain commit
-        * timestamp if there's no replication origin; otherwise, the
-        * timestamp was already set in replorigin_session_origin_timestamp by
-        * replication.
+        * Record commit timestamp. Use one in replorigin_xact_state if set,
+        * otherwise use plain commit timestamp.

I think the previous comments are clearer, so I'm not sure we need to
change these comments by this patch. Can we just change
replorigin_session_origin_timestamp to
replorigin_xact_state.origin_timestamp?

---
+   if (session_replication_state == NULL)
+       ereport(ERROR,
+               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                errmsg("no replication origin is configured")));

I'm not sure what the original author intended but given that
pg_replication_origin_xact_reset() just clears the local state I'm not
sure that we should strictly require the session_replication_state to
be set up nor it improves the behavior. If we raise an error in that
case, it might be more user-friendly but we can live even without it.

---
In origin.h:

+/*
+ * Clear the per-transaction replication origin state.
+ *
+ * replorigin_session_origin is also cleared if clear_origin is set.
+ */
+static inline void
+replorigin_xact_clear(bool clear_origin)
+{
+   replorigin_xact_state.origin_lsn = InvalidXLogRecPtr;
+   replorigin_xact_state.origin_timestamp = 0;
+   if (clear_origin)
+       replorigin_xact_state.origin = InvalidRepOriginId;
+}

Why does this function need to move to origin.h from origin.c?

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Sun, Jan 11, 2026 at 5:41 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
> ---
> In origin.h:
>
> +/*
> + * Clear the per-transaction replication origin state.
> + *
> + * replorigin_session_origin is also cleared if clear_origin is set.
> + */
> +static inline void
> +replorigin_xact_clear(bool clear_origin)
> +{
> +   replorigin_xact_state.origin_lsn = InvalidXLogRecPtr;
> +   replorigin_xact_state.origin_timestamp = 0;
> +   if (clear_origin)
> +       replorigin_xact_state.origin = InvalidRepOriginId;
> +}
>
> Why does this function need to move to origin.h from origin.c?
>
>
> That’s because, per Ashutosh’s suggestion, I added two static inline helpers replorigin_xact_set_origin() and
replorigin_xact_set_lsn_timestamp(),and I thought replorigin_xact_clear() should stay close with them. 
>
> But looks like they don’t have to be inline as they are not on hot paths. So I moved them all to origin.c and only
externthem. 

Thank you for updating the patch.

I'm not even sure that we need to have setter functions like
replorigin_xact_set_{origin,lsn_timestamp} given that
replorigin_xact_state is exposed. While the reset helper function
helps us as it removes duplicated codes and some potential accidents
like wrongly setting -1 as an invalid timestamp etc. these setter
functions don't so much. So I think we can have the patch just
consolidating the separated variables. What do you think?

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Thu, Jan 15, 2026 at 5:30 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
>
>
> On Thu, Jan 15, 2026 at 2:56 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>>
>> On Sun, Jan 11, 2026 at 5:41 PM Chao Li <li.evan.chao@gmail.com> wrote:
>> >
>> > ---
>> > In origin.h:
>> >
>> > +/*
>> > + * Clear the per-transaction replication origin state.
>> > + *
>> > + * replorigin_session_origin is also cleared if clear_origin is set.
>> > + */
>> > +static inline void
>> > +replorigin_xact_clear(bool clear_origin)
>> > +{
>> > +   replorigin_xact_state.origin_lsn = InvalidXLogRecPtr;
>> > +   replorigin_xact_state.origin_timestamp = 0;
>> > +   if (clear_origin)
>> > +       replorigin_xact_state.origin = InvalidRepOriginId;
>> > +}
>> >
>> > Why does this function need to move to origin.h from origin.c?
>> >
>> >
>> > That’s because, per Ashutosh’s suggestion, I added two static inline helpers replorigin_xact_set_origin() and
replorigin_xact_set_lsn_timestamp(),and I thought replorigin_xact_clear() should stay close with them. 
>> >
>> > But looks like they don’t have to be inline as they are not on hot paths. So I moved them all to origin.c and only
externthem. 

I am fine with it being non-static-inline.

+/*
+ * Clear the per-transaction replication origin state.
+ *
+ * replorigin_session_origin is also cleared if clear_origin is set.
+ */
+void
+replorigin_xact_clear(bool clear_origin)

Nitpick. This file exposes a few functions. The function with name
replogrigin_* deal with replication origin itself. The functions with
name replorigin_session_* deal with the session state of replication
origin. It feels like the new function is dealing with per-transaction
state within a given session. Does it make sense to name it
replorigin_session_xact_clear() instead of replorigin_xact_clear()?
Just a thought.

+ /* Do not clear the session origin */

The new comment hardly adds any value. It will be better to explain
why we aren't clearing the session origin here. But since the earlier
code didn't have a comment, it's okay to leave it as is.
-static void replorigin_reset(int code, Datum arg);
+static void on_exit_clear_state(int code, Datum arg);

Another nitpick. change the name to on_exit_clear_xact_state() to be
more clear about what state is being cleared?

0001 looks good to me.

>>
>> Thank you for updating the patch.
>>
>> I'm not even sure that we need to have setter functions like
>> replorigin_xact_set_{origin,lsn_timestamp} given that
>> replorigin_xact_state is exposed. While the reset helper function
>> helps us as it removes duplicated codes and some potential accidents
>> like wrongly setting -1 as an invalid timestamp etc. these setter
>> functions don't so much. So I think we can have the patch just
>> consolidating the separated variables. What do you think?

+1.

Some comments on 0002.

+ * Note that DoNotReplicateId is intentionally excluded here.

There are other comments like this, but I don't see value. Even
without applying this patch comment should have explained why it is
excluded. But there was no such explanation and adding this comment
without explanation doesn't add value. I think it's better to leave it
as it is for now.

/*
- * Callback on exit to reset the origin state.
+ * Callback on exit to clear transaction-level replication origin state.
+/* Per-transaction replication origin state manipulation */
extern void replorigin_xact_clear(bool clear_origin);

I think this change should be included in the first patch where we
added/renamed the functions. This patch should then deal with
encapsulating the state in a structure.

Maybe we should just commit the two patches as a single commit. But I
am ok if we commit them separately, but let's have a clear distinction
between what each patch does.

--
Best Wishes,
Ashutosh Bapat

Hi,

I didn't read the patches, but in 0002, the Suggested-by should mention
Ashutosh, not me, because this was his idea, not mine.  I only +1'd it.

Cheers

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El sudor es la mejor cura para un pensamiento enfermo" (Bardia)

> On Jan 15, 2026, at 19:22, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> Hi,
>
> I didn't read the patches, but in 0002, the Suggested-by should mention
> Ashutosh, not me, because this was his idea, not mine.  I only +1'd it.
>

Thanks for pointing out that. I will fix the info in next version.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Fri, Jan 16, 2026 at 7:37 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> On Jan 15, 2026, at 17:51, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>
>
> Hi Ashutosh,
>
> Thanks for your review again.
>
>
> On Thu, Jan 15, 2026 at 5:30 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
> On Thu, Jan 15, 2026 at 2:56 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
>
> On Sun, Jan 11, 2026 at 5:41 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
> ---
> In origin.h:
>
> +/*
> + * Clear the per-transaction replication origin state.
> + *
> + * replorigin_session_origin is also cleared if clear_origin is set.
> + */
> +static inline void
> +replorigin_xact_clear(bool clear_origin)
> +{
> +   replorigin_xact_state.origin_lsn = InvalidXLogRecPtr;
> +   replorigin_xact_state.origin_timestamp = 0;
> +   if (clear_origin)
> +       replorigin_xact_state.origin = InvalidRepOriginId;
> +}
>
> Why does this function need to move to origin.h from origin.c?
>
>
> That’s because, per Ashutosh’s suggestion, I added two static inline helpers replorigin_xact_set_origin() and
replorigin_xact_set_lsn_timestamp(),and I thought replorigin_xact_clear() should stay close with them. 
>
> But looks like they don’t have to be inline as they are not on hot paths. So I moved them all to origin.c and only
externthem. 
>
>
> I am fine with it being non-static-inline.
>
> +/*
> + * Clear the per-transaction replication origin state.
> + *
> + * replorigin_session_origin is also cleared if clear_origin is set.
> + */
> +void
> +replorigin_xact_clear(bool clear_origin)
>
> Nitpick. This file exposes a few functions. The function with name
> replogrigin_* deal with replication origin itself. The functions with
> name replorigin_session_* deal with the session state of replication
> origin. It feels like the new function is dealing with per-transaction
> state within a given session. Does it make sense to name it
> replorigin_session_xact_clear() instead of replorigin_xact_clear()?
> Just a thought.
>
>
> We’ve already gone back and forth on this function for several rounds, so I’d prefer not to make further changes
here.
>

Ok. Let's leave this to committer's judgement.

>
> Hopefully, v11 is ready to go.

I don't have any further comments.

--
Best Wishes,
Ashutosh Bapat