Re: [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance

> Here is the full patch of the timer implemenation with threading safty
> added. Basic test is by several rounds of "make check" and threading
> safty test is by a SQL file with many lines of "set
> statement_timeout =
> x". I don't know if there are any corner cases that I should
> consider, if
> any, let me know.

Here's another version of this patch ;-) I've based it on your patch, so
the changes to ovalue etc should sitill be there.

If we're going to create a separate thread, there is no need to deal
with APCs at all, I beleive. We can just use the existing timeout
functionality in WaitForSingleObjectEx(), which simplifies the code a
bit.

That is assuming we don't need sub-millisecond accuracy, because WFSO
only provides milliseconds (waitabletimer provides 1/10th of a
microsecond).

Tested with both serial and parallel regression tests and with a manual
set statement_timeout test, just as yours. And finally, also tested the
deadlock detection which I beleive also uses setitimer().

//Magnus

PS. Qingqing: it helps at least me if you can post your patch as an
attached file instead of inline. That makes absolutely sure my mailer
(yeah, I know...) doesn't get a chance to break the lines.

"Magnus Hagander" <mha(at)sollentuna(dot)net> writes:
> Here's another version of this patch ;-) I've based it on your patch, so
> the changes to ovalue etc should sitill be there.

This looks fairly reasonable to me, but I'm feeling a bit gun-shy after
the previous fiasco. Before we consider applying it so late in beta,
I'd like to get Merlin or another one of the win32 hackers to sign off
on the patch too.

regards, tom lane

"Magnus Hagander" <mha(at)sollentuna(dot)net> writes:
> Here's another version of this patch ;-) I've based it on your patch, so
> the changes to ovalue etc should sitill be there.

In the spirit of incremental improvement ... I've taken Magnus' version
and added the proposed change to re-enable Qingqing's patch by skipping
WaitForSingleObjectEx altogether in the CHECK_FOR_INTERRUPTS code path.
I also removed WaitForSingleObjectEx in pgwin32_poll_signals(), which
AFAICS should be just like CHECK_FOR_INTERRUPTS. I think this is what
we are proposing to actually apply to 8.1beta4. I can't test it though,
so please check it over...

regards, tom lane

On Sun, 23 Oct 2005, Tom Lane wrote:

> "Magnus Hagander" <mha(at)sollentuna(dot)net> writes:
>
> In the spirit of incremental improvement ... I've taken Magnus' version
> and added the proposed change to re-enable Qingqing's patch by skipping
> WaitForSingleObjectEx altogether in the CHECK_FOR_INTERRUPTS code path.
> I also removed WaitForSingleObjectEx in pgwin32_poll_signals(), which
> AFAICS should be just like CHECK_FOR_INTERRUPTS. I think this is what
> we are proposing to actually apply to 8.1beta4. I can't test it though,
> so please check it over...
>
Questions:

Are we asserting that

UNBLOCKED_SIGNAL_QUEUE() != 0
then
WaitForSingleObjectEx(0)==WAIT_OBJECT_0

If so, we can put this assertion in. Seems there is some race. In
pg_queue_signal(), we do it like this:

enter_critical_section();
mask the signal;
leave_critical_section();
SetEvent();

That is, we may detect the value first before we got event. So at least
the above assertion is not correct. This may cause other problems, just
for a quick feedback.

Regards,
Qingqing

Qingqing Zhou <zhouqq(at)cs(dot)toronto(dot)edu> writes:
> Are we asserting that

> UNBLOCKED_SIGNAL_QUEUE() != 0
> then
> WaitForSingleObjectEx(0)==WAIT_OBJECT_0

No.

> If so, we can put this assertion in.

Only if you want it to crash every so often.

The "race condition" is that a signal delivered right about the time the
check is made may be serviced before the event is set, meaning that
after the dust settles the event will still be set when there's nothing
to do. This was true before, too, and will have no impact worse than
causing an extra entry to dispatch_signals later on.

regards, tom lane

Tom Lane wrote:

>"Magnus Hagander" <mha(at)sollentuna(dot)net> writes:
>
>
>>Here's another version of this patch ;-) I've based it on your patch, so
>>the changes to ovalue etc should sitill be there.
>>
>>
>
>In the spirit of incremental improvement ... I've taken Magnus' version
>and added the proposed change to re-enable Qingqing's patch by skipping
>WaitForSingleObjectEx altogether in the CHECK_FOR_INTERRUPTS code path.
>I also removed WaitForSingleObjectEx in pgwin32_poll_signals(), which
>AFAICS should be just like CHECK_FOR_INTERRUPTS. I think this is what
>we are proposing to actually apply to 8.1beta4. I can't test it though,
>so please check it over...
>
>
>

This patch passes regression and demonstrates the expected speedup on my
machine.

cheers

andrew

"Andrew Dunstan" <andrew(at)dunslane(dot)net> wrote
>
> This patch passes regression and demonstrates the expected speedup on my
> machine.
>

Looks good from here:
- several rounds of regression test
- psql -f set_time_out.sql
- pg_ctl signal sending test
- psql deadlock test

Is there any way to test socket?

Regards,
Qingqing