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>
In-Reply-To: <CAD21AoDo_JGZPooDmbXheJrCBbEHh_WTgi31B_qNXr3-=JV93A@mail.gmail.com>
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Thu, 8 Jan 2026 15:14:38 +0530
Message-ID: <CAExHW5v9WYiTScYjXbqW-ttatRwUwhiaKw-n5YSBykc3hGyQng@mail.gmail.com>
Subject: Re: Refactor replication origin state reset helpers
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Chao Li <li.evan.chao@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/CAExHW5v9WYiTScYjXbqW-ttatRwUwhiaKw-n5YSBykc3hGyQng%40mail.gmail.com>
Precedence: bulk

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@gmail.c=
om> wrote:
>
> On Wed, Jan 7, 2026 at 5:15=E2=80=AFPM Chao Li <li.evan.chao@gmail.com> w=
rote:
>
> 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?

0002
-------
- replorigin =3D (replorigin_session_origin !=3D InvalidRepOriginId &&
- replorigin_session_origin !=3D DoNotReplicateId);
+ replorigin =3D (replorigin_xact_state.origin !=3D InvalidRepOriginId &&
+ replorigin_xact_state.origin !=3D 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.

/*
* 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 !=3D InvalidRepOriginId)
+ if (replorigin_xact_state.origin !=3D InvalidRepOriginId)
{

* Dump transaction origin information. We need this during recovery to
* update the replication origin progress.
*/
- if (replorigin_session_origin !=3D InvalidRepOriginId)
+ if (replorigin_xact_state.origin !=3D InvalidRepOriginId)
{

/* followed by the record's origin, if any */
if ((curinsert_flags & XLOG_INCLUDE_ORIGIN) &&
- replorigin_session_origin !=3D InvalidRepOriginId)
+ replorigin_xact_state.origin !=3D InvalidRepOriginId)

Not a change in this patch but the refactoring makes it more visible.
What about the case of replorigin_session_origin =3D=3D DoNotReplicateId?
Should there be a comment explaining why it's excluded here?

replorigin_session_setup(originid, MyLogicalRepWorker->leader_pid);
- replorigin_session_origin =3D originid;
+ replorigin_xact_state.origin =3D 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.o=
rigin

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

--=20
Best Wishes,
Ashutosh Bapat