On Fri, Feb 06, 2026 at 06:22:00PM +0530, Ashutosh Bapat wrote: > While writing a test for shared buffer resizing work, I noticed that > injection_points_attach() with a custom function does not work as > expected when locally attached. See new isolation test > local_custom_injection.spec in the attached patch for a reproducer. In > that test, even if we call injection_points_set_local() before calling > injection_points_attach() with a custom injection point, calling > injection_points_run() from another session still invokes the > injection callback (but should not). This happens because unlike > injection_points_attach(), injection_points_attach_func() does not > construct and pass an InjectionPointCondition object to > InjectionPointAttach(). injection_points_attach() uses the > private_data argument to pass InjectionPointCondition, whereas > injection_points_attach_func() uses private_data to pass the > user-provided attach-time argument.
injection_points_attach_func() ignores local conditions on purpose. It does not make sense to push a local condition into its private data stored in shared memory *because* it is possible to push down a custom private_data area with the function call. It's still a useful wrapper for other modules who don't want to duplicate an attach() call. > Before proceeding with it, I wanted to check whether the approach looks good. A local injection point is a concept proper to the module injection_points. In short, I don't really want to expand this API more than what we already have, sticking its philosophy and the footprint of the backend code with two key concepts: 1) We need to be able to register some data when attached, which is what the private_data area stored in shmem is about. This area can be defined by the caller of InjectionPointAttach(), with private_data and private_data_size. It's a kind of static state, that does not change for the time a point is defined. 2) We need to be able to pass down to a callback data at runtime. That's what the *arg argument of InjectionPointRun() and InjectionPointCached() is about, and what AIO tests rely on. This is a kind of dynamic state, that changes depending on what you have on your stack when a point is run. If you think about it, one does not really need more than that: it is already possible to push down data to the callbacks for a static state, depending on when a point is atached, and a dynamic state, which is what the argument pointer is about. That's also enough for stack manipulations within a callback, something that the AIO test module does. The concept introduced in your patch makes the backend API bloated for no gain: the new arguments are just a way to duplicate what concept 1) does by pushing a static condition at attach. This is demonstrated in the patch by the complexity of using two variables and four arguments that serve the same purposes. For your case with the shared buffer resizing, I'd suggest to disable install_check on it anyway if you manipulate the buffer pool directly on a cluster, due to its potential cluster-wide effect. This also means that local points are moot to have in a SQL test, either isolation of direct queries in sql/: they would never run concurrently. If you really need something that's allowed to run under installcheck with a SQL or isolation test, I would suggest to add a test directly in src/test/modules/injection_points/ with a dedicated callback. If you want to do something more complex, like what AIO does, you can just disable install_check and have one or more complicated callbacks that do what you want. I cannot say which one is more adapted than the other for your case, but I suspect that the second option is preferable, especially if you want to test various tricky scenarios in isolation of each other. injection_points has a kind of good balance now, with only a minimal set of callbacks, even if these can mean more footprint in the backend code when it comes to manipulate the stack (see f1e251be80a0 as a funky case to speed up tests). The same balance exists for the backend code APIs. -- Michael
signature.asc
Description: PGP signature
