Thread: HandleParallelMessages contains CHECK_FOR_INTERRUPTS?

$SUBJECT seems like a pretty bad idea, because it implies a recursive
entry to ProcessInterrupts and thence to HandleParallelMessages itself.
By what reasoning is that call necessary where it's placed?
        regards, tom lane

Tom Lane wrote:
> $SUBJECT seems like a pretty bad idea, because it implies a recursive
> entry to ProcessInterrupts and thence to HandleParallelMessages itself.
> By what reasoning is that call necessary where it's placed?

I notice you just removed the CHECK_FOR_INTERRUPTS in
HandleParallelMessages().  Did you notice that HandleParallelMessages
calls shm_mq_receive(), which calls shm_mq_receive_bytes(), which
contains a CHECK_FOR_INTERRUPTS() call?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> $SUBJECT seems like a pretty bad idea, because it implies a recursive
>> entry to ProcessInterrupts and thence to HandleParallelMessages itself.
>> By what reasoning is that call necessary where it's placed?

> I notice you just removed the CHECK_FOR_INTERRUPTS in
> HandleParallelMessages().  Did you notice that HandleParallelMessages
> calls shm_mq_receive(), which calls shm_mq_receive_bytes(), which
> contains a CHECK_FOR_INTERRUPTS() call?

I figured there were probably other cases of recursion; it's unlikely
we could prevent them altogether.  But a check in a function that's
called *only* from ProcessInterrupts seems pretty dubious.

I wonder whether we should make use of HOLD_INTERRUPTS/RESUME_INTERRUPTS
to avoid the recursion scenario here.  It's not really clear to me whether
this stack of function would behave sanely if it's invoked recursively
when the outer level is partway through reading a message.  At the very
least, it seems like there might be a risk for out-of-order message
processing (inner recursion reads and processes next message while outer
level has read but not yet processed previous message).
        regards, tom lane

I wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> I notice you just removed the CHECK_FOR_INTERRUPTS in
>> HandleParallelMessages().  Did you notice that HandleParallelMessages
>> calls shm_mq_receive(), which calls shm_mq_receive_bytes(), which
>> contains a CHECK_FOR_INTERRUPTS() call?

After study, I believe that that CHECK_FOR_INTERRUPTS() is unreachable
given that HandleParallelMessages passes nowait = true.  But it's not
unlikely that future changes in shm_mq.c might introduce such calls that
are reachable.

> I wonder whether we should make use of HOLD_INTERRUPTS/RESUME_INTERRUPTS
> to avoid the recursion scenario here.

I concluded that that would be good future-proofing, whether or not it's
strictly necessary today, so I pushed it.
        regards, tom lane