Re: waitpid in pg_basebackup

Hi,

waitpid() is used with "#ifdef HAVE_WAITPID" in reaper(), but NOT in
BaseBackup().
Why not? We can ensure that all platforms which PostgreSQL supports
have waitpid()?
If so, can we get rid of "#ifdef HAVE_WAITPID" in reaper()?

Regards,

--
Fujii Masao

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> waitpid() is used with "#ifdef HAVE_WAITPID" in reaper(), but NOT in
> BaseBackup().
> Why not? We can ensure that all platforms which PostgreSQL supports
> have waitpid()?
> If so, can we get rid of "#ifdef HAVE_WAITPID" in reaper()?

The Single Unix Spec V2 (1997) specifies both waitpid() and wait3(),
but says the latter is "legacy" and new applications should use only
waitpid(); furthermore it documents that platforms need not support
wait3() (they can just return ENOSYS instead of making it work).

I notice that pgbench also uses waitpid() unconditionally in its
"#ifndef ENABLE_THREAD_SAFETY" section. That's been there for
quite some time, making it even less likely that there are still
any platforms where waitpid() isn't available.

I agree, let's drop the support for waitpid() not being present.
It looks to me like we could remove the macro maze in reaper()
completely, if we fixed win32_waitpid() to not have an API randomly
different from the real waitpid(). That would be a noticeable
readability gain there.

regards, tom lane

I wrote:
> I agree, let's drop the support for waitpid() not being present.

BTW, some digging in the commit logs shows that postmaster.c's
existing support for using wait3 in place of waitpid was added in
commit a5494a2d92a2752c610b8b668a7d33478e90c160, "Various patches for
nextstep by GregorHoffleit". NextStep is presumably quite dead by
now, and anyway we officially pulled support for it as of 9.2.

I will go ahead and remove that code.

regards, tom lane

On Fri, Jul 6, 2012 at 2:01 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> I agree, let's drop the support for waitpid() not being present.
>
> BTW, some digging in the commit logs shows that postmaster.c's
> existing support for using wait3 in place of waitpid was added in
> commit a5494a2d92a2752c610b8b668a7d33478e90c160, "Various patches for
> nextstep by GregorHoffleit". NextStep is presumably quite dead by
> now, and anyway we officially pulled support for it as of 9.2.
>
> I will go ahead and remove that code.

Thanks!

BTW, I was just implementing the patch ;) Patch attached.
Note that I've not tested the patch on Windows environment
because I don't have that....

Regards,

--
Fujii Masao

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> On Fri, Jul 6, 2012 at 2:01 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I will go ahead and remove that code.

> Thanks!

> BTW, I was just implementing the patch ;) Patch attached.

Oh, I'd already done it when I got your message :-(. Looks like we
arrived at the same answers, though, except for slightly different
choices for how to do the Windows bit.

> Note that I've not tested the patch on Windows environment
> because I don't have that....

Me either; I'll check the buildfarm results later.

regards, tom lane