Re: Refactor replication origin state reset helpers

On Jan 10, 2026, at 06:08, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

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 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

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?