Re: Refactor replication origin state reset helpers

On 2025-Dec-24, Ashutosh Bapat wrote:

> If we go this route, we at least need to declare the new functions as
> static inline and move them to a header file instead of .c file.

Hmm, why would we make them static inline instead of standard (extern)
functions? We use static inline functions when we want to avoid the
overhead of a function call in a hot code path, but I doubt that's the
case here. Am I mistaken on this?

> Further, does it make sense to put together all the state variables
> into a single structure?

Yeah -- keeping the threaded-backend project in mind, moving them to a
single struct seems to make sense. I think it's a separate patch though
because it'd be more invasive than Chao's initial patch, as those
variables are used in many places.

> It's also quite easy to confuse between these functions and
> replorigin_session_reset(). It's not clear where the boundaries of the
> latter end and where those of the new ones start. I think the latter
> deals with the shared memory structures while the new ones deal with
> the backend local state. And then there's replorigin_reset() which
> adds to the confusion. That function doesn't call
> replorigin_session_reset() which the other two callers of
> replorigin_session_clear_state() call. Why? I think there is more to
> clean here than what's in the patch. That doesn't mean that we cannot
> accept this patch without larger cleanup, but it should not add to the
> existing confusion.

Good points. A decrease in the total quantity of cruft would be a good
outcome of a patch in this area.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Update: super-fast reaction on the Postgres bugs mailing list. The report
was acknowledged [...], and a fix is under discussion.
The wonders of open-source !"
https://twitter.com/gunnarmorling/status/1596080409259003906