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.