On Jan 15, 2026, at 17:51, Ashutosh Bapat <[email protected]> wrote:
Hi Ashutosh, Thanks for your review again. On Thu, Jan 15, 2026 at 5:30 AM Chao Li <[email protected]> wrote: On Thu, Jan 15, 2026 at 2:56 AM Masahiko Sawada <[email protected]> wrote: On Sun, Jan 11, 2026 at 5:41 PM Chao Li <[email protected]> wrote: --- 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 = InvalidXLogRecPtr; + replorigin_xact_state.origin_timestamp = 0; + if (clear_origin) + replorigin_xact_state.origin = InvalidRepOriginId; +} Why does this function need to move to origin.h from origin.c? That’s because, per Ashutosh’s suggestion, I added two static inline helpers replorigin_xact_set_origin() and replorigin_xact_set_lsn_timestamp(), and I thought replorigin_xact_clear() should stay close with them. But looks like they don’t have to be inline as they are not on hot paths. So I moved them all to origin.c and only extern them. I am fine with it being non-static-inline. +/* + * Clear the per-transaction replication origin state. + * + * replorigin_session_origin is also cleared if clear_origin is set. + */ +void +replorigin_xact_clear(bool clear_origin) Nitpick. This file exposes a few functions. The function with name replogrigin_* deal with replication origin itself. The functions with name replorigin_session_* deal with the session state of replication origin. It feels like the new function is dealing with per-transaction state within a given session. Does it make sense to name it replorigin_session_xact_clear() instead of replorigin_xact_clear()? Just a thought. We’ve already gone back and forth on this function for several rounds, so I’d prefer not to make further changes here. -static void replorigin_reset(int code, Datum arg); +static void on_exit_clear_state(int code, Datum arg); Another nitpick. change the name to on_exit_clear_xact_state() to be more clear about what state is being cleared? Renamed as your suggestion. 0001 looks good to me. Thanks for confirming. Thank you for updating the patch. I'm not even sure that we need to have setter functions like replorigin_xact_set_{origin,lsn_timestamp} given that replorigin_xact_state is exposed. While the reset helper function helps us as it removes duplicated codes and some potential accidents like wrongly setting -1 as an invalid timestamp etc. these setter functions don't so much. So I think we can have the patch just consolidating the separated variables. What do you think? +1. Some comments on 0002. + * Note that DoNotReplicateId is intentionally excluded here. There are other comments like this, but I don't see value. Even without applying this patch comment should have explained why it is excluded. But there was no such explanation and adding this comment without explanation doesn't add value. I think it's better to leave it as it is for now. Reverted the comment. /* - * Callback on exit to reset the origin state. + * Callback on exit to clear transaction-level replication origin state. +/* Per-transaction replication origin state manipulation */ extern void replorigin_xact_clear(bool clear_origin); I think this change should be included in the first patch where we added/renamed the functions. This patch should then deal with encapsulating the state in a structure. Make sense. Other than this comment, I also moved the other one to 0001. Maybe we should just commit the two patches as a single commit. But I am ok if we commit them separately, but let's have a clear distinction between what each patch does. Now, 0001 adds a helper and does some small refactoring; 0002 is purely for encapsulating the state into a structure without any extra change. Hopefully, v11 is ready to go. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
v11-0002-Consolidate-replication-origin-session-globals-i.patch
Description: Binary data
v11-0001-Refactor-replication-origin-state-reset-helpers.patch
Description: Binary data
