On Wed, Dec 24, 2025 at 6:58 AM Chao Li <li.evan.chao@gmail.com> 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.
