Re: Refactor replication origin state reset helpers - Mailing list pgsql-hackers

> 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. 

>
> 0002
> -------
> - replorigin = (replorigin_session_origin != InvalidRepOriginId &&
> - replorigin_session_origin != DoNotReplicateId);
> + replorigin = (replorigin_xact_state.origin != InvalidRepOriginId &&
> + replorigin_xact_state.origin != DoNotReplicateId);
>
> There's another small deduplication opportunity here. Define function
> replorigin_xact_origin_isvalid() to check these two conditions and use
> that function here and in other places like
> RecordTransactionCommitPrepared(). I would go as far as making the
> function static inline, and use it instead of replorigin variable,
> whose name is certainly misleading - it doesn't talk about transaction
> at all. With static inline there, optimizer may be able to eliminate
> the multiple function call overhead.

Thanks for pointing out the idea. I can look into that tomorrow.

>
> /*
> * Record commit timestamp. The value comes from plain commit timestamp
> * if replorigin is not enabled, or replorigin already set a value for us
> - * in replorigin_session_origin_timestamp otherwise.
> + * in replorigin_xact_state.origin_timestamp otherwise.
>
> suggestion "Record commit timestamp. Use one in replorigin_xact_state
> if set, otherwise use plain commit timestamp.". This reads better and
> is closer to the code.". If you agree, please change other similar
> comments too.
>
> @@ -5928,12 +5928,12 @@ XactLogCommitRecord(TimestampTz commit_time,
> }
> /* dump transaction origin information */
> - if (replorigin_session_origin != InvalidRepOriginId)
> + if (replorigin_xact_state.origin != InvalidRepOriginId)
> {
>
> * Dump transaction origin information. We need this during recovery to
> * update the replication origin progress.
> */
> - if (replorigin_session_origin != InvalidRepOriginId)
> + if (replorigin_xact_state.origin != InvalidRepOriginId)
> {
>
> /* followed by the record's origin, if any */
> if ((curinsert_flags & XLOG_INCLUDE_ORIGIN) &&
> - replorigin_session_origin != InvalidRepOriginId)
> + replorigin_xact_state.origin != InvalidRepOriginId)
>
> Not a change in this patch but the refactoring makes it more visible.
> What about the case of replorigin_session_origin == DoNotReplicateId?
> Should there be a comment explaining why it's excluded here?
>
> replorigin_session_setup(originid, MyLogicalRepWorker->leader_pid);
> - replorigin_session_origin = originid;
> + replorigin_xact_state.origin = originid;
>
> replorigin_session_setup() is always followed by
> replorigin_xact_state.origin assignment. abort/commit handlers set the
> other two members. Do we want to create replorigin_xact_set_origin()
> and replorigin_xact_set_lsn_timestamp() functions to encapsulate these
> assignments? Those two will serve as counterparts to
> replorigin_xact_clear(). Or would that be an overkill?
>
> * successfully and that we don't need to do so again. In combination with
> - * setting up replorigin_session_origin_lsn and replorigin_session_origin
> + * setting up replorigin_xact_state.origin_lsn and replorigin_xact_state.origin
>
> I think just replorigin_xact_state should suffice for the sake of brevity.
>
> static void
> on_exit_clear_state(int code, Datum arg)
> {
> - replorigin_session_clear(true);
> + replorigin_xact_clear(true);
>
> The prologue still states clear origin, but it should mention
> transaction state instead.
> }
>

I will look into these as well tomorrow.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/