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

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