Re: Injection point locking

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.