Re: Refactor replication origin state reset helpers - Mailing list pgsql-hackers

On Jan 15, 2026, at 17:51, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:

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

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.

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

Now, 0001 adds a helper and does some small refactoring; 0002 is purely for encapsulating the state into a structure without any extra change.

Hopefully, v11 is ready to go.

Best regards,