Date: Thu, 9 May 2024 16:39:00 -0700
From: Noah Misch <noah@leadboat.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, Tom Lane <tgl@sss.pgh.pa.us>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Weird test mixup
Message-ID: <20240509233900.4e.nmisch@google.com>
References: <ZjNAUN5Hek-MyVWv@paquier.xyz>
 <20240502193555.a1.nmisch@google.com>
 <ZjgsaYs44ici4QMW@paquier.xyz>
 <20240506212324.2e.nmisch@google.com>
 <ZjmBPeB6LQeHVHZ7@paquier.xyz>
 <20240507185310.d3@rfd.leadboat.com>
 <Zjwa4lX6wADRAili@paquier.xyz>
 <20240509031553.47@rfd.leadboat.com>
 <ZjxVcbsIy4tq3uhV@paquier.xyz>
 <Zjx9-2swyNg6E1y1@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zjx9-2swyNg6E1y1@paquier.xyz>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20240509233900.4e.nmisch%40google.com>
Precedence: bulk

On Thu, May 09, 2024 at 04:40:43PM +0900, Michael Paquier wrote:
> So I like your suggestion.  This version closes the race window
> between the shmem exit detach in backend A and a concurrent backend B
> running a point local to A so as B will never run the local point of
> A.  However, it can cause a failure in the shmem exit callback of
> backend A if backend B does a detach of a point local to A because A
> tracks its local points with a static list in its TopMemoryContext, at
> least in the attached.  The second case could be solved by tracking
> the list of local points in the module's InjectionPointSharedState,
> but is this case really worth the complications it adds in the module
> knowing that the first case would be solid enough?  Perhaps not.
> 
> Another idea I have for the second case is to make
> InjectionPointDetach() a bit "softer", by returning a boolean status 
> rather than fail if the detach cannot be done, so as the shmem exit
> callback can still loop through the entries it has in store.

The return-bool approach sounds fine.  Up to you whether to do in this patch,
else I'll do it when I add the test.

> It could
> always be possible that a concurrent backend does a detach followed by
> an attach with the same name, causing the shmem exit callback to drop
> a point it should not, but that's not really a plausible case IMO :)

Agreed.  It's reasonable to expect test cases to serialize backend exits,
attach calls, and detach calls.  If we need to fix that later, we can use
attachment serial numbers.

> --- a/src/test/modules/injection_points/injection_points.c
> +++ b/src/test/modules/injection_points/injection_points.c

> +typedef enum InjectionPointConditionType
> +{
> +	INJ_CONDITION_INVALID = 0,

I'd name this INJ_CONDITION_UNCONDITIONAL or INJ_CONDITION_ALWAYS.  INVALID
sounds like a can't-happen event or an injection point that never runs.
Otherwise, the patch looks good and makes src/test/modules/gin safe for
installcheck.  Thanks.