MIME-Version: 1.0
References: <CAExHW5usKxNkgGdM+6cw-RiDN55MLSmoUC1LBi6tQ3eYMuZm0Q@mail.gmail.com>
 <202512291435.5edkf3hnichv@alvherre.pgsql> <CAExHW5vYPkRW2CeeQX+z230LNb8kqKgxbFxmbSMNiHVQdvEP8Q@mail.gmail.com>
 <CAEoWx2mT+gu8H-KhaeNodk4T7csX-_wwqRew=-t3zo6b6jXfaQ@mail.gmail.com>
 <CAEoWx2=7xcxQPJgnPfWsKj2SzCNiBaxOXa1u9fSKME1Yd=r=fQ@mail.gmail.com>
 <CAD21AoD1HCtzN1WKXOPpifVTprzC=fkTPqPDuO9Z6oAZLXSYLA@mail.gmail.com>
 <CAEoWx2nsZeL1A_tbzoBCRXC1R2uBpYuQTN3dPy=dDPhpvvsRNA@mail.gmail.com>
 <26279DAC-5ADA-4E30-B866-5DED0EEC2BEF@gmail.com> <CAEoWx2moawAzQLaHu=C9PEkro7tmXXq792ODM0=bM9rtoE_E2g@mail.gmail.com>
 <CAD21AoDo_JGZPooDmbXheJrCBbEHh_WTgi31B_qNXr3-=JV93A@mail.gmail.com>
 <CAExHW5v9WYiTScYjXbqW-ttatRwUwhiaKw-n5YSBykc3hGyQng@mail.gmail.com> <349AF48A-06DE-4016-B23A-F4A2834484C3@gmail.com>
In-Reply-To: <349AF48A-06DE-4016-B23A-F4A2834484C3@gmail.com>
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Thu, 8 Jan 2026 20:55:32 +0530
Message-ID: <CAExHW5ubP0Q5PyYmcoU7wWV-K4h62zbxcEwCmFxm+a-5_HiqOQ@mail.gmail.com>
Subject: Re: Refactor replication origin state reset helpers
To: Chao Li <li.evan.chao@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>, =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>, 
	Postgres hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAExHW5ubP0Q5PyYmcoU7wWV-K4h62zbxcEwCmFxm%2Ba-5_HiqOQ%40mail.gmail.com>
Precedence: bulk

On Thu, Jan 8, 2026 at 6:32=E2=80=AFPM Chao Li <li.evan.chao@gmail.com> wro=
te:
>
>
>
> > 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=E2=80=AFAM Masahiko Sawada <sawada.mshk@gma=
il.com> wrote:
> >>
> >> On Wed, Jan 7, 2026 at 5:15=E2=80=AFPM 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 =3D InvalidXLogRecPtr;
> > + replorigin_session_origin_timestamp =3D 0;
> > + if (clear_origin)
> > + replorigin_session_origin =3D 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 =3D InvalidXLogRecPtr;
> > - replorigin_session_origin_timestamp =3D 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=E2=80=99t have to no more on 0001. As 0002 is a big refactori=
ng, where we group the 3 global variables into a structure and rename the h=
elp 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.

--=20
Best Wishes,
Ashutosh Bapat