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? 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. /* * 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. } -- Best Wishes, Ashutosh Bapat
