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
