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

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:

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()?

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 = {
+   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