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? 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 = 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.". 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 = (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. Good point. Added replorigin_xact_origin_isvalid() in 0002 as you suggested. /* * 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 != 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? Added a comment to the 3 places. 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? Actually, when I added the new structure, I had the idea of adding replorigin_xact_set_origin() and replorigin_xact_set_lsn_timestamp() as I saw a lot of duplicate code for the assignment. But I hesitated to do that because I worried people might consider that’s 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 replorigin_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_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. 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. BTW, Ashutosh, this is the CF entry: https://commitfest.postgresql.org/patch/6345/, you may mark a reviewer there. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
v8-0001-Refactor-replication-origin-state-reset-helpers.patch
Description: Binary data
v8-0002-Consolidate-replication-origin-session-globals-in.patch
Description: Binary data
