Thread

Re: Refactor replication origin state reset helpers

Chao Li <li.evan.chao@gmail.com> — 2025-12-25T05:05:51Z

Hi Ashutosh,

Thanks for your review.

On Wed, Dec 24, 2025 at 6:57 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

> 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.
https://www.highgo.com/