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.