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.
