Re: [PATCH] Fix for infinite signal loop in parallel scan

On Sat, Sep 8, 2018 at 3:57 AM Chris Travers <chris(dot)travers(at)adjust(dot)com> wrote:
> Attached is the patch we are fully testing at Adjust.

Thanks!

> I have run make check on Linux and MacOS, and make check-world on Linux (check-world fails on MacOS on all versions and all branches due to ecpg failures).

FWIW it's entirely possible to get make check-world passing on a Mac.
Maybe post the problem you're seeing to a new thread?

> ...

> In the past it had been suggested we do PG_TRY(); and PG_CATCH(), but given that it is not consistent whether we can raise an error or whether we MUST raise an error, I don't see how this approach can work. As far as I can see, we MUST raise an error in the appropriate spot if and only if elevel is set to a sufficient level.

Yeah, your way looks a bit nicer than something involving PG_TRY().

> Is there any feedback on this approach before I add it to the next commitfest?

Please go ahead and add it. Being a bug fix, we'll commit it sooner
than the open commitfest anyway, but it's useful to have it in there.

+ if (errno == EINTR && elevel >= ERROR)
+ CHECK_FOR_INTERRUPTS();

I think we might as well just CHECK_FOR_INTERRUPTS() unconditionally.
In this branch elevel is always ERROR as you noted, and the code
around there is confusing enough already.

+ } while (rc == EINTR && !(ProcDiePending || QueryCancelPending));

There seems to be a precedent for checking QueryCancelPending directly
to break out of a loop in regcomp.c and syncrep.c. So this seems OK.
Hmm, I wonder if there should be an INTERRUPT_PENDING() macro that
hides those details, but allows you to break out of a loop and then do
some cleanup before CHECK_FOR_INTERRUPT().

--
Thomas Munro
http://www.enterprisedb.com