Re: Dependency between bgw_notify_pid and bgw_flags

On Wed, Sep 9, 2015 at 8:14 AM, Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> PFA patch with improved test module and fix for a bug.
>
> bgworker_sigusr1_handler() should set the latch when set_latch_on_sigusr1 is
> true, similar to procsignal_sigusr1_handler(). Without this fix, if a
> background worker without DATABASE_CONNECTION flag calls
> WaitForBackgroundWorker*() functions, those functions wait indefinitely as
> the latch is never set upon receipt of SIGUSR1.

This is hardly an insurmountable problem given that they can replace
bgworker_sigusr1_handler() with another handler whenever they like.
It might be a good idea anyway, but not without saving and restoring
errno.

>> Thanks. I don't think this test module is suitable for commit for a
>> number of reasons, including the somewhat hackish use of exit(0)
>> instead of proper error reporting
> I have changed this part of code.

Maybe I should have been more clear: I don't really want a test module
for this. Yeah, there was a bug, but we fixed it, and I don't see
that it's going to come back. We might have a different one, but this
module is so special-purpose that it won't catch that. If there are
some +1s to the idea of this test module from other people then I am
not unwilling to reconsider, but my opinion is that this is the wrong
thing to spend time on. If we had some plan to build out a test
framework here that would really thoroughly exercise these code paths,
that might be valuable -- I'm not opposed to the idea of trying to
have better test coverage of the bgworker code. But I just don't see
that this particular module does enough to make it worthwhile.

Considering that, I'm not really excited about spending a lot of time
on review right now, but FWIW I still think the error handling in here
is weak. Why elog() and not ereport()? Yeah, this is not user-facing
stuff exactly because it's just a test module, but where else do we
use elog() like this? Why elog(WARNING) and proc_exit(1) instead of
elog(FATAL) or elog(PANIC)? The messages don't follow the style
guidelines either, like "found launcher in unexpected state (expected
status %d, got %d), exiting." -- that doesn't look anything like our
usual style for reporting errors.

>> , the fact that you didn't integrate
>> it into the Makefile structure properly
>
> What was missing? I am able to make {check,clean,install) from the
> directory. I can also make -C <dirpath> check from repository's root.

make check-world won't run it, right? So there would be no BF coverage.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company