Re: Review: support for multiplexing SIGUSR1 - Mailing list pgsql-hackers

Hi,

Thanks for reviewing the patch!

On Mon, Jul 27, 2009 at 2:43 AM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
> 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.

OK, I'll change the mechanism to assign a ProcSignalSlot to a process,
independently from ProcState, but similarly to ProcState: a process gets
a ProcSignalSlot from newly-introduced freelist at startup, and returns it
to freelist at exit. Since this assignment and return require the lock, I'll
introduce a new LWLock for them.

>  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?

Yes, I'm planning to use this mechanism for communication between
a process which can generate the WAL records and a WAL sender process,
in Synch Rep. Since not only a backend but also some auxiliary processes
(e.g., bgwriter) can generate the WAL records, processes which don't have
a ProcState need to receive the multiplexed signal, too.

> 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.

This makes sense. I'll get rid of a spinlock from the patch and add the usage
note as above.

>  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?

In, at least, currently-intended use case (i.e., Synch Rep), such wrong signal
is not fatal because it's only used as a hint.

> 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.

Umm... the patch should cover a notify interrupt which currently uses
SIGUSR2?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center