On Wed, Sep 9, 2015 at 8:14 AM, Ashutosh Bapat <ashutosh.ba...@enterprisedb.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers