Fujii Masao <masao.fujii@gmail.com> writes:
> I updated the patch to solve two problems which you pointed.
> Here is the changes:
> * Prevented the obsolete flag to being set to a new process, by using
> newly-introduced spinlock.
> * Used the index of AuxiliaryProcs instead of auxType, to assign
> backend ID to an auxiliary process.
Neither of these changes seem like a good idea to me. The use of a
spinlock renders the mechanism unsafe for use from the postmaster ---
we do not wish the postmaster to risk getting stuck if the contents of
shared memory have become corrupted, eg, there is a spinlock that looks
taken. And you've completely mangled the concept of BackendId.
MyBackendId is by definition the same as the index of the process's
entry in the sinval ProcState array. This means that (1) storing it in
the ProcState entry is silly, and (2) assigning values that don't
correspond to an actual ProcState entry is dangerous.
If we want to be able to signal processes that don't have a ProcState
entry, it would be better to assign an independent index instead of
overloading BackendId like this. I'm not sure though whether we care
about being able to signal such processes. It's certainly not necessary
for catchup interrupts, but it might be for future applications of the
mechanism. Do we have a clear idea of what the future applications are?
As for the locking issue, I'm inclined to just specify that uses of the
mechanism must be such that receiving a signal that wasn't meant for you
isn't fatal. In the case of catchup interrupts the worst that can
happen is you do a little bit of useless work. Are there any intended
uses where it would be seriously bad to get a misdirected signal?
I agree with Jaime that the patch would be more credible if it covered
more than one signal usage at the start --- these questions make it
clear that the design can't happen in a vacuum without intended usages
in mind.
regards, tom lane