Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: Refactor replication origin state reset helpers
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <CAEoWx2nsZeL1A_tbzoBCRXC1R2uBpYuQTN3dPy=dDPhpvvsRNA@mail.gmail.com>
Date: Wed, 7 Jan 2026 16:49:02 +0800
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>,
 =?utf-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>,
 Postgres hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <26279DAC-5ADA-4E30-B866-5DED0EEC2BEF@gmail.com>
References: <CAExHW5usKxNkgGdM+6cw-RiDN55MLSmoUC1LBi6tQ3eYMuZm0Q@mail.gmail.com>
 <202512291435.5edkf3hnichv@alvherre.pgsql>
 <CAExHW5vYPkRW2CeeQX+z230LNb8kqKgxbFxmbSMNiHVQdvEP8Q@mail.gmail.com>
 <CAEoWx2mT+gu8H-KhaeNodk4T7csX-_wwqRew=-t3zo6b6jXfaQ@mail.gmail.com>
 <CAEoWx2=7xcxQPJgnPfWsKj2SzCNiBaxOXa1u9fSKME1Yd=r=fQ@mail.gmail.com>
 <CAD21AoD1HCtzN1WKXOPpifVTprzC=fkTPqPDuO9Z6oAZLXSYLA@mail.gmail.com>
 <CAEoWx2nsZeL1A_tbzoBCRXC1R2uBpYuQTN3dPy=dDPhpvvsRNA@mail.gmail.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/26279DAC-5ADA-4E30-B866-5DED0EEC2BEF%40gmail.com>
Precedence: bulk


> On Jan 7, 2026, at 15:21, Chao Li <li.evan.chao@gmail.com> wrote:
>=20
>=20
> On Wed, Jan 7, 2026 at 8:30=E2=80=AFAM Masahiko Sawada =
<sawada.mshk@gmail.com> wrote:
> On Mon, Dec 29, 2025 at 11:17=E2=80=AFPM Chao Li =
<li.evan.chao@gmail.com> wrote:
> >
> > On Tue, Dec 30, 2025 at 1:07=E2=80=AFPM Chao Li =
<li.evan.chao@gmail.com> wrote:
> >>
> >>
> >> On Tue, Dec 30, 2025 at 12:48=E2=80=AFPM Ashutosh Bapat =
<ashutosh.bapat.oss@gmail.com> wrote:
> >>>
> >>> On Mon, Dec 29, 2025 at 8:14=E2=80=AFPM =C3=81lvaro 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.
> >
>=20
> I've reviewed both patches. Here are some comments:
>=20
> Thank you very much for the review.
> =20
> 0001 patch:
>=20
> +/*
> + * 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 =3D InvalidXLogRecPtr;
> +   replorigin_session_origin_timestamp =3D 0;
> +   if (!xact_only)
> +       replorigin_session_origin =3D InvalidRepOriginId;
> +}
>=20
> 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()?
>=20
> Agreed.=20
>  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.
>=20
> Fixed.
> =20
> 0002 patch:
>=20
> +typedef struct RepOriginSessionState
> +{
> +   RepOriginId origin;
> +   XLogRecPtr  origin_lsn;
> +   TimestampTz origin_timestamp;
> +}          RepOriginSessionState;
> +
> +extern PGDLLIMPORT RepOriginSessionState replorigin_session_state;
>=20
> 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"?
>=20
>=20
> 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().
>=20
> --
> +RepOriginSessionState replorigin_session_state =3D {
> +   InvalidRepOriginId, InvalidXLogRecPtr, 0
> +};
>=20
> I think using designated initializers here would be better for
> readability and robustness against future struct changes.
>=20
>  Fixed.=20
>=20
>=20
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/ =
<v5-0001-Refactor-replication-origin-state-reset-helpers.patch><v5-0002-Co=
nsolidate-replication-origin-session-globals-in.patch>

The CF CI failed a test case, but I don=E2=80=99t think that=E2=80=99s =
mine. I just did a self-review for v5 and couldn=E2=80=99t find any =
logic change. So I would guess the CI failure was intermittent. I =
don=E2=80=99t know how to rerun the CI test other than bumping the patch =
version and re-posting.

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