[pgpool-hackers: 3901] 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.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sigusr1.diff
Type: text/x-patch
Size: 1658 bytes
Desc: not available
URL: <http://www.pgpool.net/pipermail/pgpool-hackers/attachments/20210520/f45d5efb/attachment.bin>