Subject: Re: Refactor replication origin state reset helpers

From: Masahiko Sawada <[email-masked]>
Date: Wed, 7 Jan 2026 17:46:33 -0800

On Wed, Jan 7, 2026 at 5:15 PM Chao Li <[email-masked]> wrote:
>
>
> On Wed, Jan 7, 2026 at 4:49 PM Chao Li <[email-masked]> 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 logic change. So I would guess the CI failure was intermittent. I don’t know how to rerun the CI test other than bumping the patch 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

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 8 Jan 2026 15:14:38 +0530

Hi Masahiko,

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

> 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

---

From: Chao Li <[email-masked]>
Date: Thu, 8 Jan 2026 21:02:11 +0800

> On Jan 8, 2026, at 17:44, Ashutosh Bapat <[email-masked]> wrote:
> 
> Hi Masahiko,
> 

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 structure and 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.

> the multiple function call overhead.

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

> transaction state instead.

I will look into these as well tomorrow.

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

---

From: Ashutosh Bapat <[email-masked]>
Date: Thu, 8 Jan 2026 20:55:32 +0530

> 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 structure and rename the help function, 0001 can be considered as a preparation com…

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

-- 
Best Wishes,
Ashutosh Bapat

---

From: Chao Li <[email-masked]>
Date: Fri, 9 Jan 2026 14:50:26 +0800

On Jan 8, 2026, at 17:44, Ashutosh Bapat <[email-masked]>
wrote:

Hi Masahiko,

On Thu, Jan 8, 2026 at 7:17 AM Masahiko Sawada <[email-masked]>
wrote:

0001
-------

+/*

Renamed the help function to the same name as in 0002.

Updated the comment in 0001.

Added the same check in 0002.

0002
-------

Good point. Added replorigin_xact_origin_isvalid() in 0002 as you suggested.

/*

Agreed. Updated the comment in both places.

Added a comment to the 3 places.

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
because I 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 we have 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

Updated the comment to eliminate the duplication.

static void

Good catch. Updated the header comment.

All comments are addressed in v8.

BTW, Ashutosh, this is the CF entry:
https://commitfest.postgresql.org/patch/6345/, you may mark a reviewer
there.

Best regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 9 Jan 2026 14:08:19 -0800

On Thu, Jan 8, 2026 at 10:50 PM Chao Li <[email-masked]> wrote:
> 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 because I 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 we have no reason to leave replorigin_xact_clear() alone as external. So I made it static inline as well.
>
>

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,

---

From: Chao Li <[email-masked]>
Date: Mon, 12 Jan 2026 09:41:45 +0800

On Jan 10, 2026, at 06:08, Masahiko Sawada <[email-masked]> wrote:

Hi Masahiko,

0001
-------

+/*

0002
-------

/*

static void

Thanks for the review.

Datum

and

   /*

Fair enough. Let me revert the change related to
replorigin_xact_origin_isvalid.

---
-    * if replorigin is not enabled, or replorigin already set a value for
us

and

-        * timestamp was already set in replorigin_session_origin_timestamp
by

Reverted to the old comments and updated the variable name in the comments.

---

Fair point. I reverted that. If we really want to add the check, let’s do
that in a separate thread.

---
In origin.h:

+/*

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 extern them.

PFA v9.

Best regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 14 Jan 2026 10:56:18 -0800

On Sun, Jan 11, 2026 at 5:41 PM Chao Li <[email-masked]> wrote:
> 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 extern them.

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,

---

From: Chao Li <[email-masked]>
Date: Thu, 15 Jan 2026 08:00:31 +0800

On Thu, Jan 15, 2026 at 2:56 AM Masahiko Sawada <[email-masked]>
wrote:

> 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 extern them.
>
No problem. I reverted the two helpers in v10.

Best regards,