On Wed, Jan 7, 2026 at 8:30 AM Masahiko Sawada <[email protected]> wrote:
> 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: > 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
Description: Binary data
v5-0002-Consolidate-replication-origin-session-globals-in.patch
Description: Binary data
