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> <CAEoWx2mL=s4ROe6fjOr2=YbUrrZOVQVVnqb8tK2C7sCgvnGUHg@mail.gmail.com>
In-Reply-To: <CAEoWx2mL=s4ROe6fjOr2=YbUrrZOVQVVnqb8tK2C7sCgvnGUHg@mail.gmail.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Fri, 9 Jan 2026 14:08:19 -0800
Message-ID: <CAD21AoALhGyb=5b+j4ZNQpdMER5XDR92j9Nbouz3M0Hc=X2mpA@mail.gmail.com>
Subject: Re: Refactor replication origin state reset helpers
To: Chao Li <li.evan.chao@gmail.com>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@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/CAD21AoALhGyb%3D5b%2Bj4ZNQpdMER5XDR92j9Nbouz3M0Hc%3DX2mpA%40mail.gmail.com>
Precedence: bulk

On Thu, Jan 8, 2026 at 10:50=E2=80=AFPM Chao Li <li.evan.chao@gmail.com> wr=
ote:
>
>
>
> On Jan 8, 2026, at 17:44, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> w=
rote:
>
> 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=
.com> 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?
>
>
> Renamed the help function to the same name as in 0002.
>
>
> @@ -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.".
>
>
> Updated the comment in 0001.
>
>
> 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?
>
>
> Added the same check in 0002.
>
>
> 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.
>
>
> Good point. Added replorigin_xact_origin_isvalid() in 0002 as you suggest=
ed.
>
>
> /*
> * 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.
>
>
> Agreed. Updated the comment in both places.
>
>
> @@ -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?
>
>
> Added a comment to the 3 places.
>
> 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?
>
>
> Actually, when I added the new structure, I had the idea of adding replor=
igin_xact_set_origin() and replorigin_xact_set_lsn_timestamp() as I saw a l=
ot of duplicate code for the assignment. But I hesitated to do that because=
 I worried people might consider that=E2=80=99s too much.
>
> Given you raised the same idea, let me add the helpers as static inline. =
We now have 3 static inline helpers, I guess we have no reason to leave rep=
lorigin_xact_clear() alone as external. So I made it static inline as well.
>
>
> * successfully and that we don't need to do so again. In combination with
> - * setting up replorigin_session_origin_lsn and replorigin_session_origi=
n
> + * 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=
.
>
>
> Updated the comment to eliminate the duplication.
>
>
> 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.
> }
>
>
> Good catch. Updated the header comment.
>
> All comments are addressed in v8.

Thank you for updating the patch!

The 0001 patch looks good to me.

I have some comments on the 0002 patch:

-   replorigin =3D (replorigin_session_origin !=3D InvalidRepOriginId &&
-                 replorigin_session_origin !=3D DoNotReplicateId);
+   replorigin =3D replorigin_xact_origin_isvalid();

I'm not sure it's a good refactoring TBH. While it can remove some
lines, we still need to check if replorigin_xact_state.origin !=3D
InvalidRepOriginId at several places. For example,

Datum
pg_replication_origin_session_is_setup(PG_FUNCTION_ARGS)
{
    replorigin_check_prerequisites(false, false);

    PG_RETURN_BOOL(replorigin_xact_state.origin !=3D InvalidRepOriginId);
}

and

    /*
     * Dump transaction origin information. We need this during recovery to
     * update the replication origin progress.
     *
     * Note that DoNotReplicateId is intentionally excluded here.
     */
    if (replorigin_xact_state.origin !=3D InvalidRepOriginId)

The readers might be confused why we cannot use
replorigin_xact_origin_isvalid() here in spite of its function name,
replorigin_xact_origin_isvalid, sounds quite suitable. The comment
mentions the fact that DoNotReplicated is excluded but doesn't mention
why.

Also, even if we want this kind of change, it should be implemented in
a separate patch because it's not related to the main idea of
consolidating the replication origin global variables.

---
-    * Record commit timestamp.  The value comes from plain commit timestam=
p
-    * if replorigin is not enabled, or replorigin already set a value for =
us
-    * in replorigin_session_origin_timestamp otherwise.
+    * Record commit timestamp. Use one in replorigin_xact_state if set,
+    * otherwise use plain commit timestamp.

and

-        * Record commit timestamp.  The value comes from plain commit
-        * timestamp if there's no replication origin; otherwise, the
-        * timestamp was already set in replorigin_session_origin_timestamp=
 by
-        * replication.
+        * Record commit timestamp. Use one in replorigin_xact_state if set=
,
+        * otherwise use plain commit timestamp.

I think the previous comments are clearer, so I'm not sure we need to
change these comments by this patch. Can we just change
replorigin_session_origin_timestamp to
replorigin_xact_state.origin_timestamp?

---
+   if (session_replication_state =3D=3D NULL)
+       ereport(ERROR,
+               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                errmsg("no replication origin is configured")));

I'm not sure what the original author intended but given that
pg_replication_origin_xact_reset() just clears the local state I'm not
sure that we should strictly require the session_replication_state to
be set up nor it improves the behavior. If we raise an error in that
case, it might be more user-friendly but we can live even without it.

---
In origin.h:

+/*
+ * Clear the per-transaction replication origin state.
+ *
+ * replorigin_session_origin is also cleared if clear_origin is set.
+ */
+static inline void
+replorigin_xact_clear(bool clear_origin)
+{
+   replorigin_xact_state.origin_lsn =3D InvalidXLogRecPtr;
+   replorigin_xact_state.origin_timestamp =3D 0;
+   if (clear_origin)
+       replorigin_xact_state.origin =3D InvalidRepOriginId;
+}

Why does this function need to move to origin.h from origin.c?

Regards,

--=20
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com