Feedback-ID: ie3de48e3:Fastmail
Date: Mon, 29 Dec 2025 12:00:45 -0300
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: Chao Li <li.evan.chao@gmail.com>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Refactor replication origin state reset helpers
Message-ID: <202512291457.jy7mlturovmy@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202512291435.5edkf3hnichv@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/202512291457.jy7mlturovmy%40alvherre.pgsql>
Precedence: bulk

On 2025-Dec-29, Álvaro Herrera wrote:

> On 2025-Dec-24, Ashutosh Bapat wrote:

> > 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.

I think we should just rename the worker.c function to have a less
generic-sounding name (so that it becomes more obvious that it's a
worker.c-specific function); have both replorigin_session_clear_state()
and replorigin_xact_clear_state() be a single function, with a boolean
flag to indicate whether to reset replorigin_session_origin or not; and
have replorigin_session_reset() call replorigin_session_clear_state()
internally rather than require every single of its callers do it
separately, which IMO makes no sense.  With these three changes I think
the code would become quite clear.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
Thou shalt check the array bounds of all strings (indeed, all arrays), for
surely where thou typest "foo" someone someday shall type
"supercalifragilisticexpialidocious" (5th Commandment for C programmers)