Hi Ashutosh, Thanks for your review.
On Wed, Dec 24, 2025 at 6:57 PM Ashutosh Bapat <[email protected]> wrote: > On Wed, Dec 24, 2025 at 6:58 AM Chao Li <[email protected]> wrote: > > > > Hi Hacker, > > > > While reviewing patches [1] and [2], I noticed some duplicate code of > clearing replication origin states, I am proposing a small patch that > removes the duplicate code blocks by introducing a couple helper functions. > No functional change at all. > > > > The new functions bring together the global variables that need to be > reset under certain conditions. The functions will help not to miss > resetting some variable. However, this can be a mild backpatching > pain. So, I am +.5 on this. > > 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. > I like the idea of making the helpers static inline and moving them to origin.h to stay close to the 3 global variables, which improves readability as well. See attached v2 for the change. > Further, does it make sense to put together all the state variables > into a single structure? > I hesitate to go that far, because the 3 global variables are all exported. So, if we really want to do that, that should belong to a dedicated thread. > It's also quite easy to confuse between these functions and > replorigin_session_reset(). It's not clear where the boundaries of the > latter end and where those of the new ones start. I think the latter > deals with the shared memory structures while the new ones deal with > the backend local state. And then there's replorigin_reset() which > adds to the confusion. That function doesn't call > replorigin_session_reset() which the other two callers of > replorigin_session_clear_state() call. Why? I think there is more to > clean here than what's in the patch. That doesn't mean that we cannot > accept this patch without larger cleanup, but it should not add to the > existing confusion. > Good point. I am trying to resolve the confusions in v2. For replorigin_reset(), it's easy to address. As this function is local static and the only purpose is to satisfy the pg_on_exit_callback contract, I just renamed it to on_exit_clear_state() in v2. For replorigin_session_reset(), there are only 2 callers and both callers call replorigin_session_clear_state() immediately after replorigin_session_reset(). So in v2, I made replorigin_session_reset() to call replorigin_session_clear_state(), and added some comments in replorigin_session_reset(). Accordingly, replorigin_session_reset() is used to reset states set by replorigin_session_setup(), and every call of replorigin_session_setup() is immediately followed by "replorigin_session_origin = originid;", so I moved this assignment into replorigin_session_setup(). With these broader changes, this patch is no longer trivial, so I just added it to CF: Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
v2-0001-Refactor-replication-origin-state-reset-helpers.patch
Description: Binary data
