[pgpool-hackers: 3902] Re: sigusr1_interrupt_processor() does not process all pending requests

[Tatsuo Ishii]

>> Looking into the code of sigusr1_interrupt_processor(), it seems
>> sigusr1_interrupt_processor() is assumed that it processes all the
>> request details in sigalFlags array even if there are multiple details
>> in the array. But actually it's not.
> 
> I confirmed that sigusr1_interrupt_processor() can handle multiple
> requests. That's good. However there are race condition which could
> cause this:
> 
>> t-ishii$ grep signalFlags pgpool[0-2]/log/pgpool.log
>> pgpool1/log/pgpool.log:2021-05-18 15:25:58: main pid 12696: LOG:  signalFlags: 3 is true
>> pgpool2/log/pgpool.log:2021-05-18 15:25:58: main pid 12700: LOG:  signalFlags: 3 is true
>> pgpool2/log/pgpool.log:2021-05-18 15:26:04: main pid 12700: LOG:  signalFlags: 3 is true
> 
> Because while or after execution of sigusr1_interrupt_processor(),
> other process can register new requests into the signalFlags array
> because there is no interlocking mechanism between the signal sender
> signal_user1_to_parent_with_reason() and
> sigusr1_interrupt_processor().
> 
>> I don't know if this actually gives some bad effects to pgpool or not
>> but I think at least we need to confirm that.
> 
> It does. In CHECK_REQUEST:
> 
> 		if (sigusr1_request) \
> 		{ \
> 			sigusr1_interrupt_processor(); \
> 			sigusr1_request = 0; \
> 
> If other process sends SIGUSR1 while sigusr1_interrupt_processor() is
> running, sigusr1_request is set to 1 by the signal handler. But after that:
> 
> 			sigusr1_request = 0; \
> 
> is executed and sigusr1_interrupt_processor() will not be called until
> the next signal arrives. If the next signal does not arrive or arrives
> after long period, that would be a problem.
> 
> To fix it, I think the code fragment above should be changed like
> this:
> 
> 		if (sigusr1_request) \
> 		{ \
> 			do {\
> 				sigusr1_request = 0; \
> 				sigusr1_interrupt_processor(); \
> 			} while (sigusr1_request == 1); \
> 		} \
> 
> This way, the condition statement (sigusr1_request == 1) will catch
> the arrival of the signal while sigusr1_interrupt_processor() was
> running, and it will be processed next execution of
> sigusr1_interrupt_processor().
> 
> Note that similar code fragment also exists in Pgpoolmain. This also
> should be changed to:
> 
> 		if (sigusr1_request)
> 		{
> 			do {
> 				sigusr1_request = 0;
> 				sigusr1_interrupt_processor();
> 			} while (sigusr1_request == 1);
> 		}
> 
> Also I have changed some log message level from DEBUG1 to LOG because
> they are important information.
> 
> Comments are welcome.

Fix pushed to master and all supported branches.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp