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

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