On Tue, Jun 25, 2024 at 11:14:57AM +0900, Michael Paquier wrote: > On Mon, Jun 24, 2024 at 01:29:38PM +0300, Heikki Linnakangas wrote: > > InjectionPointRun() acquires InjectionPointLock, looks up the hash entry, > > and releases the lock: > > > > > LWLockAcquire(InjectionPointLock, LW_SHARED); > > > entry_by_name = (InjectionPointEntry *) > > > hash_search(InjectionPointHash, name, > > > HASH_FIND, &found); > > > LWLockRelease(InjectionPointLock); > > > > Later, it reads fields from the entry it looked up: > > > > > /* not found in local cache, so load and register */ > > > snprintf(path, MAXPGPATH, "%s/%s%s", pkglib_path, > > > entry_by_name->library, DLSUFFIX); > > > > Isn't that a straightforward race condition, if the injection point is > > detached in between? > > This is a feature, not a bug :) > > Jokes apart, this is a behavior that Noah was looking for so as it is > possible to detach a point to emulate what a debugger would do with a > breakpoint for some of his tests with concurrent DDL bugs, so not > taking a lock while running a point is important. It's true, though, > that we could always delay the LWLock release once the local cache is > loaded, but would it really matter?
I think your last sentence is what Heikki is saying should happen, and I agree. Yes, it matters. As written, InjectionPointRun() could cache an entry_by_name->function belonging to a different injection point.
