Re: Refactor replication origin state reset helpers

On Thu, Jan 8, 2026 at 6:32 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Jan 8, 2026, at 17:44, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > Hi Masahiko,
> >
> > Thanks for updating the patches. Here are some more comments.
> >
> > On Thu, Jan 8, 2026 at 7:17 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >>
> >> On Wed, Jan 7, 2026 at 5:15 PM Chao Li <li.evan.chao@gmail.com> wrote:
> >>
> >> I've made some cosmetic changes to both patches (comments and the
> >> commit messages). Please review them and let me know what you think.
> >
> > 0001
> > -------
> >
> > +/*
> > + * Clear session replication origin state.
> > + *
> > + * replorigin_session_origin is also cleared if clear_origin is set.
> > + */
> > +void
> > +replorigin_session_clear(bool clear_origin)
> > +{
> > + replorigin_session_origin_lsn = InvalidXLogRecPtr;
> > + replorigin_session_origin_timestamp = 0;
> > + if (clear_origin)
> > + replorigin_session_origin = InvalidRepOriginId;
> > +}
> >
> > All the other replorigin_session_* functions deal with
> > session_replication_state, but this function does not deal with it. I
> > see that in the next patch this function has been renamed as
> > replorigin_xact_clear() which seems more appropriate. Do you intend to
> > squash these two patches when committing?
> >
> > @@ -1482,8 +1493,8 @@ pg_replication_origin_xact_reset(PG_FUNCTION_ARGS)
> > {
> > replorigin_check_prerequisites(true, false);
> > - replorigin_session_origin_lsn = InvalidXLogRecPtr;
> > - replorigin_session_origin_timestamp = 0;
> > + /* Clear only origin_lsn and origin_timestamp */
> > + replorigin_session_clear(false);
> >
> > The comment can explain why we are not clearing
> > replorigin_session_origin here. Something like "This function is
> > cancel the effects of pg_replication_origin_xact_setup(), which only
> > sets origin_lsn and origin_timestamp, so we only clear those two
> > fields here.".
> >
> > Next comment does not apply to this patch, but the inconsistency I am
> > speaking about becomes apparent now. This function resets the state
> > setup by pg_replication_origin_xact_setup(), which checks for
> > session_replication_state being non-NULL. So I expected
> > pg_replication_origin_xact_reset() also to check for the same
> > condition or at least Assert it. Why is it not doing so?
>
> Hi Ashutosh,
>
> Thanks for your follow-up review.
>
> IMO, we don’t have to no more on 0001. As 0002 is a big refactoring, where we group the 3 global variables into a
structureand rename the help function, 0001 can be considered as a preparation commit that does some simple cleanup. So
that,we can put main focus on the real refactoring in 0002. 
>

In that case at least the function name change should be part of 0001.

--
Best Wishes,
Ashutosh Bapat