From: Noah Misch <noah@leadboat.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Weird test mixup
Date: Thu, 9 May 2024 16:39:00 -0700
Message-ID: <20240509233900.4e.nmisch@google.com> (raw)
In-Reply-To: <Zjx9-2swyNg6E1y1@paquier.xyz>
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>
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.
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com, michael@paquier.xyz, hlinnaka@iki.fi, tgl@sss.pgh.pa.us
Subject: Re: Weird test mixup
In-Reply-To: <20240509233900.4e.nmisch@google.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox