On Tue, Jun 25, 2024 at 09:10:06AM -0700, Noah Misch wrote:
> 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.
That's true, we could delay the release of the lock to happen just
before a callback is run.
Now, how much do people wish to see for the postmaster bits mentioned
upthread? Taking a spinlock for so long is not going to work, so we
could just remove it and let developers deal with that and feed on the
flexibility with the lock removal to allow this stuff in more areas.
All the existing tests are OK with that, and I think that also the
case of what you have proposed for the concurrency issues with
in-place updates of catalogs. Or we could live with a no-lock path
when going through that with the postmaster, but that's a bit weird.
Note that with the current callbacks in the module, assuming that a
point is added within BackendStartup() in the postmaster like the
attached, an ERROR is promoted to a FATAL, taking down the cluster. A
NOTICE of course works find. Waits with conditional variables are not
really OK. How much are you looking for here?
The shmem state being initialized in the DSM registry is not something
that's going to work in the context of the postmaster, but we could
tweak the module so as it can be loaded, initializing the shared state
with the shmem hooks and falling back to a DSM registry when the
library is not loaded with shared_preload_libraries. For example, see
the POC attached, where I've played with injection points in
BackendStartup(), which is the area I'm guessing Heikki was looking
at.
--
Michael