Re: Injection point locking

On 25/06/2024 05:25, Noah Misch wrote:
> On Mon, Jun 24, 2024 at 11:03:09AM -0400, Tom Lane wrote:
>> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>>> ... I can't do that, because InjectionPointRun() requires a PGPROC
>>> entry, because it uses an LWLock. That also makes it impossible to use
>>> injection points in the postmaster. Any chance we could allow injection
>>> points to be triggered without a PGPROC entry? Could we use a simple
>>> spinlock instead?
> 
> That sounds fine to me.  If calling hash_search() with a spinlock feels too
> awful, a list to linear-search could work.

>>> With a fast path for the case that no injection points
>>> are attached or something?
>>
>> Even taking a spinlock in the postmaster is contrary to project
>> policy.  Maybe we could look the other way for debug-only code,
>> but it seems like a pretty horrible precedent.
> 
> If you're actually using an injection point in the postmaster, that would be
> the least of concerns.  It is something of a concern for running an injection
> point build while not attaching any injection point.  One solution could be a
> GUC to control whether the postmaster participates in injection points.
> Another could be to make the data structure readable with atomics only.

I came up with the attached. It replaces the shmem hash table with an 
array that's scanned linearly. On each slot in the array, there's a 
generation number that indicates whether the slot is in use, and allows 
detecting concurrent modifications without locks. The attach/detach 
operations still hold the LWLock, but InjectionPointRun() is now 
lock-free, so it can be used without a PGPROC entry.

It's now usable from postmaster too. However, it's theoretically 
possible that if shared memory is overwritten with garbage, the garbage 
looks like a valid injection point with a name that matches one of the 
injection points that postmaster looks at. That seems unlikely enough 
that I think we can accept the risk. To close that gap 100% I think a 
GUC is the only solution.

Note that until we actually add an injection point to a function that 
runs in the postmaster, there's no risk. If we're uneasy about that, we 
could add an assertion to InjectionPointRun() to prevent it from running 
in the postmaster, so that we don't cross that line inadvertently.

Thoughts?

-- 
Heikki Linnakangas
Neon (https://neon.tech)