public inbox for [email protected]
help / color / mirror / Atom feedFrom: Masahiko Sawada <[email protected]>
To: Chao Li <[email protected]>
Cc: Ashutosh Bapat <[email protected]>
Cc: Álvaro Herrera <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: Refactor replication origin state reset helpers
Date: Tue, 6 Jan 2026 16:29:28 -0800
Message-ID: <CAD21AoD1HCtzN1WKXOPpifVTprzC=fkTPqPDuO9Z6oAZLXSYLA@mail.gmail.com> (raw)
In-Reply-To: <CAEoWx2=7xcxQPJgnPfWsKj2SzCNiBaxOXa1u9fSKME1Yd=r=fQ@mail.gmail.com>
References: <CAExHW5usKxNkgGdM+6cw-RiDN55MLSmoUC1LBi6tQ3eYMuZm0Q@mail.gmail.com>
<[email protected]>
<CAExHW5vYPkRW2CeeQX+z230LNb8kqKgxbFxmbSMNiHVQdvEP8Q@mail.gmail.com>
<CAEoWx2mT+gu8H-KhaeNodk4T7csX-_wwqRew=-t3zo6b6jXfaQ@mail.gmail.com>
<CAEoWx2=7xcxQPJgnPfWsKj2SzCNiBaxOXa1u9fSKME1Yd=r=fQ@mail.gmail.com>
On Mon, Dec 29, 2025 at 11:17 PM Chao Li <[email protected]> wrote:
>
> On Tue, Dec 30, 2025 at 1:07 PM Chao Li <[email protected]> wrote:
>>
>>
>> On Tue, Dec 30, 2025 at 12:48 PM Ashutosh Bapat <[email protected]> wrote:
>>>
>>> On Mon, Dec 29, 2025 at 8:14 PM Álvaro Herrera <[email protected]> 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
view thread (3+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Refactor replication origin state reset helpers
In-Reply-To: <CAD21AoD1HCtzN1WKXOPpifVTprzC=fkTPqPDuO9Z6oAZLXSYLA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox