public inbox for [email protected]
help / color / mirror / Atom feedFrom: Ashutosh Bapat <[email protected]>
To: Chao Li <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Álvaro Herrera <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: Refactor replication origin state reset helpers
Date: Thu, 8 Jan 2026 20:55:32 +0530
Message-ID: <CAExHW5ubP0Q5PyYmcoU7wWV-K4h62zbxcEwCmFxm+a-5_HiqOQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAExHW5usKxNkgGdM+6cw-RiDN55MLSmoUC1LBi6tQ3eYMuZm0Q@mail.gmail.com>
<[email protected]>
<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>
<[email protected]>
<CAEoWx2moawAzQLaHu=C9PEkro7tmXXq792ODM0=bM9rtoE_E2g@mail.gmail.com>
<CAD21AoDo_JGZPooDmbXheJrCBbEHh_WTgi31B_qNXr3-=JV93A@mail.gmail.com>
<CAExHW5v9WYiTScYjXbqW-ttatRwUwhiaKw-n5YSBykc3hGyQng@mail.gmail.com>
<[email protected]>
On Thu, Jan 8, 2026 at 6:32 PM Chao Li <[email protected]> wrote:
>
>
>
> > On Jan 8, 2026, at 17:44, Ashutosh Bapat <[email protected]> wrote:
> >
> > Hi Masahiko,
> >
> > Thanks for updating the patches. Here are some more comments.
> >
> > On Thu, Jan 8, 2026 at 7:17 AM Masahiko Sawada <[email protected]> wrote:
> >>
> >> On Wed, Jan 7, 2026 at 5:15 PM Chao Li <[email protected]> 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 structure and 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
view thread (10+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Refactor replication origin state reset helpers
In-Reply-To: <CAExHW5ubP0Q5PyYmcoU7wWV-K4h62zbxcEwCmFxm+a-5_HiqOQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox