MIME-Version: 1.0
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>
In-Reply-To: <CAEoWx2=7xcxQPJgnPfWsKj2SzCNiBaxOXa1u9fSKME1Yd=r=fQ@mail.gmail.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Tue, 6 Jan 2026 16:29:28 -0800
Message-ID: <CAD21AoD1HCtzN1WKXOPpifVTprzC=fkTPqPDuO9Z6oAZLXSYLA@mail.gmail.com>
Subject: Re: Refactor replication origin state reset helpers
To: Chao Li <li.evan.chao@gmail.com>
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-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAD21AoD1HCtzN1WKXOPpifVTprzC%3DfkTPqPDuO9Z6oAZLXSYLA%40mail.gmail.com>
Precedence: bulk

On Mon, Dec 29, 2025 at 11:17=E2=80=AFPM Chao Li <li.evan.chao@gmail.com> w=
rote:
>
> 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@k=
urilemu.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 th=
e
>>> > 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 tho=
ugh
>>> > 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:

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 =3D InvalidXLogRecPtr;
+   replorigin_session_origin_timestamp =3D 0;
+   if (!xact_only)
+       replorigin_session_origin =3D 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()?

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.

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

--
+RepOriginSessionState replorigin_session_state =3D {
+   InvalidRepOriginId, InvalidXLogRecPtr, 0
+};

I think using designated initializers here would be better for
readability and robustness against future struct changes.

Regards,


--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com