Re: [Review] pgbench duration option

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> sys/time.h and unistd.h are #included just a few lines after that, but
> within a #ifndef WIN32 block. I don't think the patch added any
> codepaths where we'd need those header files on Windows, so I presume
> that was just an oversight and those two extra #includes can be removed?
> I don't have a Windows environment to test it myself.

Well, if we did need them the buildfarm would soon tell us, no?

> Also, should we be using pqsignal at all? It's not clear to me what it
> is, to be honest, but there's a note in pqsignal.h that says "This
> shouldn't be in libpq, but the monitor and some other things need it..."

Yeah, it has more portable semantics than just plain signal().

That note isn't offbase I suppose --- now that we have src/port/ it
would have been better to put it there. But moving it now would be an
ABI break for libpq.so, and I don't think it's worth it.

Are people satisfied that the Windows part of the patch is okay?
I'm not able to review it meaningfully. One thing that looks suspicious
is that handle_sig_alarm doesn't look like it's needed for Windows;
it'd be better if win32_timer_callback just did timer_exceeded = true
for itself. (This might explain the bogus #include requirements?)

regards, tom lane