From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Dependency between bgw_notify_pid and bgw_flags |
Date: | 2015-09-09 13:27:58 |
Message-ID: | CA+TgmoYOrZpURLJJ2Tsx9ifkPie+JcVnUMF5pZ=bCeHUg6ShAQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2015-09-09 13:55:10 | Re: DBT-3 with SF=20 got failed |
Previous Message | Fujii Masao | 2015-09-09 13:13:04 | Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file |