Re: Review: support for multiplexing SIGUSR1

Hi Jaime,

On Fri, Jul 17, 2009 at 6:31 AM, Jaime
Casanova<jcasanov(at)systemguards(dot)com(dot)ec> wrote:
> I'm reviewing this patch:
> http://archives.postgresql.org/message-id/3f0b79eb0907022341m1d36a841x19c3e2a5a6906b5b@mail.gmail.com

Thanks for reviewing the patch!

> something that make me nervous is this:
> /*
> * Note: Since there's no locking, it's possible that the target
> * process detaches from shared memory and exits right after this
> * test, before we set the flag and send signal. And the signal slot
> * might even be recycled by a new process, so it's remotely possible
> * that we set a flag for a wrong process. That's OK, all the signals
> * are such that no harm is done if they're mistakenly fired.
> */
>
> can we signal a wrong process and still "be fine"?

Umm... the old flag might be set to a new process wongly as follows.
1. The target process exits right after SendProcSignal passed that test.
2. A new process is assigned to the same ProcSignalSlot, and reset it.
3. SendProcSignal sets the old flag to the slot.

I think that locking is required here to get around this problem. How about
introducing a new slock variable "slock_t pss_lck" into ProcSignalSlot strust,
which protects pss_pid and pss_signalFlags? SendProcSignal and
ProcSignalInit should use the slock.

> besides, seems like SendProcSignal is still attached to SIGUSR1 only,
> is this fine?

Yes, I think that multiplexing of one signal is enough. Why do you think
that SendProcSignal should be attached to also the other signals?

> Another thing that took my attention, i don't think this is safe (it
> assumes only one auxiliary process of any type, don't know if we have
> various of the same kind but...):
>
> + /*
> + * Assign backend ID to auxiliary processes like backends, in order to
> + * allow multiplexing signal to auxiliary processes. Since backends use
> + * ID in the range from 1 to MaxBackends (inclusive), we assign
> + * auxiliary processes with MaxBackends + AuxProcType + 1 as
> an unique ID.
> + */
> + MyBackendId = MaxBackends + auxType + 1;
> + MyProc->backendId = MyBackendId;

That assumption is OK for now, but might be unacceptable in the near future.
So, I'll use the index of AuxiliaryProcs instead of auxType, which is assigned
in InitAuxiliaryProcess. Is this OK?

Regards,

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