Win32 CHECK_FOR_INTERRUPTS() performance tweak

Qingqing Zhou <zhouqq@cs.toronto.edu> writes:

This patch improves the win32 CHECK_FOR_INTERRUPTS() performance by
testing if any unblocked signals are queued before check
pgwin32_signal_event. This avoids an unnecessary system call.

http://archives.postgresql.org/pgsql-patches/2005-10/msg00191.php

This looks to me like a pretty important performance tweak for Windows.
Can any of the people who worked on the Windows signal implementation
look it over and confirm it's OK?

regards, tom lane

This patch improves the win32 CHECK_FOR_INTERRUPTS() performance by
testing if any unblocked signals are queued before check
pgwin32_signal_event. This avoids an unnecessary system call.

http://archives.postgresql.org/pgsql-patches/2005-10/msg00191.php

This looks to me like a pretty important performance tweak
for Windows.
Can any of the people who worked on the Windows signal
implementation look it over and confirm it's OK?

I think it looks good. (Haven't tested it yet, but from reading it.)

It seems it's safe to skip the interlocking that we do. If we miss one
signal for some reason, we will find it on the next hit to
CHECK_FOR_INTERRUPTS(). And if we get an "extra hit", we still
re-examine the stuff when we're locked, so it shouldn't be a big
problem.

Do you have any numbers on how much performance increases with it? I
agree that it looks like it could be a significant help in some cases,
but it'd be great to have numbers...

I'm a little bit concerned about doing it this late in beta, but it does
look safe to me. When it's that late, it'd be good to have one more
person review it though :)

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

Do you have any numbers on how much performance increases with it?

A rough estimate is that it would cost more than half as much as EXPLAIN
ANALYZE does: that imposes two extra syscalls per ExecProcNode, while
this adds one. There are other high-frequency CHECK_FOR_INTERRUPTS
calls that I can't quantify as easily. My guess is that it's costing us
less than a factor of 2, but well more than 10%, on typical queries
... so definitely worth fixing for 8.1 if we can convince ourselves
it's correct.

regards, tom lane

I can't get the postgres to link with the patch...
Am I missing something?
Merlin

Info: resolving _pg_signal_mask by linking to __imp__pg_signal_mask
(auto-import)
Info: resolving _pg_signal_queue by linking to __imp__pg_signal_queue
(auto-import)
fu000001.o(.idata$3+0xc): undefined reference to `libpostgres_a_iname'
fu000003.o(.idata$3+0xc): undefined reference to `libpostgres_a_iname'
nmth000000.o(.idata$4+0x0): undefined reference to `_nm__pg_signal_mask'
nmth000002.o(.idata$4+0x0): undefined reference to
`_nm__pg_signal_queue'

"Merlin Moncure" <merlin.moncure@rcsonline.com> writes:

I can't get the postgres to link with the patch...
Am I missing something?

Probably those variables need "extern DLLIMPORT".

regards, tom lane

"Tom Lane" <tgl@sss.pgh.pa.us> wrote

... so definitely worth fixing for 8.1 if we can convince ourselves
it's correct.

Despite the performance, there is one thing I am not exactly sure. Shall we
add "volatile" quanlifier to at least pg_signal_queue? The dangerous place
is PGSemaphoreLock(). If the compiler cache this value somehow, then we are
in trouble, but the original way (check event directly) does not have this
problem.

Regards,
Qingqing

I can't get the postgres to link with the patch...
Am I missing something?
Merlin

False alarm. I had to rerun configure which copies win32.h in various
places, as Qingqing noted.

Merlin

I can't get the postgres to link with the patch...
Am I missing something?
Merlin

False alarm. I had to rerun configure which copies win32.h in various
places, as Qingqing noted.

Not false alarm :) Only with DLLIMPORT can I link all the libraries.
Will have performance #s up in a bit.

Merlin

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:

Shall we add "volatile" quanlifier to at least pg_signal_queue?

If that's changed by a separate thread, "volatile" seems essential.
What about the mask variable?

regards, tom lane

Qingqing Zhou wrote:

"Tom Lane" <tgl@sss.pgh.pa.us> wrote

... so definitely worth fixing for 8.1 if we can convince ourselves
it's correct.

Despite the performance, there is one thing I am not exactly sure. Shall we
add "volatile" quanlifier to at least pg_signal_queue? The dangerous place
is PGSemaphoreLock(). If the compiler cache this value somehow, then we are
in trouble, but the original way (check event directly) does not have this
problem.

The fact this question is asked worries me a bit.

Also, I have a small style question - why use a nested if instead of
just saying

if (UNBLOCKED_SIGNAL_QUEUE() && WaitForSingleObjectEx(pgwin32_signal_event,0,TRUE) == WAIT_OBJECT_0)

?

cheers

andrew

Shall we add "volatile" quanlifier to at least pg_signal_queue?

If that's changed by a separate thread, "volatile" seems essential.
What about the mask variable?

Yes, that does seem right. Previously it would never be concurrently
modified, because it was always locked by the critical section, but now
we read it without locking, and we certainly don't want that optimized
away.

The mask is only ever written by the main thread, never by the signal
dispatching thread. So I think that one could do without.

//Magnus

I can't get the postgres to link with the patch...
Am I missing something?
Merlin

False alarm. I had to rerun configure which copies win32.h in

various

places, as Qingqing noted.

Not false alarm :) Only with DLLIMPORT can I link all the libraries.
Will have performance #s up in a bit.

I have a couple of cpu-bound performance tests that I just ran with and
without the patch. Everything is ran with n=1 until volatile issue is
sorted out but so far I am not seeing any performance
improvement...believe me I would like nothing more than to report
otherwise. Also my tests maybe don't stress the signal code much. I
observed both the running time measured from the client and the weighted
average cpu load on the server.

Maybe it make a difference with high user load.

Merlin

On Fri, 21 Oct 2005, Magnus Hagander wrote:

Shall we add "volatile" quanlifier to at least pg_signal_queue?

If that's changed by a separate thread, "volatile" seems essential.
What about the mask variable?

Yes, that does seem right. Previously it would never be concurrently
modified, because it was always locked by the critical section, but now
we read it without locking, and we certainly don't want that optimized
away.

The mask is only ever written by the main thread, never by the signal
dispatching thread. So I think that one could do without.

Agreed.

Regards,
Qingqing

"Merlin Moncure" <merlin.moncure@rcsonline.com> writes:

Will have performance #s up in a bit.

I have a couple of cpu-bound performance tests that I just ran with and
without the patch. Everything is ran with n=1 until volatile issue is
sorted out but so far I am not seeing any performance
improvement...

Hm, what were the tests exactly? Offhand I'd expect something like a
SELECT COUNT(*) on a large but not-too-wide table to show noticeable
improvement.

regards, tom lane

"Merlin Moncure" <merlin.moncure@rcsonline.com> writes:
Hm, what were the tests exactly? Offhand I'd expect something like a
SELECT COUNT(*) on a large but not-too-wide table to show noticeable
improvement.

regards, tom lane

I STAND CORRECTED! My tests were high volume record by record
iterators, etc. Read and drool, gentlemen.

Merlin

=============stock============

esp=# select count(*) from data1.line_file;
count
--------
321306
(1 row)

Time: 844.000 ms
esp=# select count(*) from data1.line_file;
count
--------
321306
(1 row)

Time: 843.000 ms
esp=# select count(*) from data1.line_file;
count
--------
321306
(1 row)

Time: 844.000 ms
esp=# \q
=============patched============
esp=# \timing
Timing is on.
esp=# select count(*) from data1.line_file;
count
--------
321306
(1 row)

Time: 453.000 ms
esp=# select count(*) from data1.line_file;
count
--------
321306
(1 row)

Time: 468.000 ms
esp=# select count(*) from data1.line_file;
count
--------
321306
(1 row)

Time: 469.000 ms

Hm, what were the tests exactly? Offhand I'd expect

something like a

SELECT COUNT(*) on a large but not-too-wide table to show

noticeable

improvement.

regards, tom lane

I STAND CORRECTED! My tests were high volume record by
record iterators, etc. Read and drool, gentlemen.

Wow, that's just great!

Was that with the volatile attribute or not?

//Magnus

"Merlin Moncure" <merlin.moncure@rcsonline.com> writes:

I STAND CORRECTED! My tests were high volume record by record
iterators, etc. Read and drool, gentlemen.

Looks good to me ;-) ...

If I recall the bidding correctly, the original patch needs DLLIMPORT
qualifiers attached to both of the variables, plus volatile attached to
pg_signal_queue. Do you want to send along the modified patch, or do
you think a non-Windows-hacker can get it right the first time?

regards, tom lane

On Fri, 21 Oct 2005, Merlin Moncure wrote:

"Merlin Moncure" <merlin.moncure@rcsonline.com> writes:
Hm, what were the tests exactly? Offhand I'd expect something like a
SELECT COUNT(*) on a large but not-too-wide table to show noticeable
improvement.

regards, tom lane

I STAND CORRECTED! My tests were high volume record by record
iterators, etc. Read and drool, gentlemen.

Great! Did you patch the "volatile" thing?

Regards,
Qingqing

Wow, that's just great!

Was that with the volatile attribute or not?

//Magnus

not. However (I'm assuming) this should not greatly impact things.
Good work. QQ: Can you fix the patch? I'm done till Monday.

Merlin

"Magnus Hagander" <mha@sollentuna.net> writes:

Was that with the volatile attribute or not?

I doubt volatile would make any visible performance difference --- the
CHECK_FOR_INTERRUPTS calls that are performance-critical are in places
where the compiler couldn't try to optimize away the fetches anyway.
The volatile qualifier is just needed to cover our rears in case a
CHECK_FOR_INTERRUPTS is used in some fairly small loop with no external
function calls.

regards, tom lane

QQ: Can you fix the patch? I'm done till Monday.

Sure, thanks for testing it. Below is the revised version.

Regards,
Qingqing

---

Index: backend/port/win32/signal.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/port/win32/signal.c,v
retrieving revision 1.12
diff -u -r1.12 signal.c
--- backend/port/win32/signal.c	15 Oct 2005 02:49:23 -0000	1.12
+++ backend/port/win32/signal.c	21 Oct 2005 05:32:52 -0000
@@ -19,11 +19,12 @@
 /* pg_signal_crit_sec is used to protect only pg_signal_queue. That is the only
  * variable that can be accessed from the signal sending threads! */
 static CRITICAL_SECTION pg_signal_crit_sec;
-static int	pg_signal_queue;

 static pqsigfunc pg_signal_array[PG_SIGNAL_COUNT];
 static pqsigfunc pg_signal_defaults[PG_SIGNAL_COUNT];
-static int	pg_signal_mask;
+
+DLLIMPORT volatile int pg_signal_queue;
+DLLIMPORT int pg_signal_mask;

DLLIMPORT HANDLE pgwin32_signal_event;
HANDLE pgwin32_initial_signal_pipe = INVALID_HANDLE_VALUE;
@@ -91,11 +92,10 @@
int i;

 	EnterCriticalSection(&pg_signal_crit_sec);
-	while (pg_signal_queue & ~pg_signal_mask)
+	while (UNBLOCKED_SIGNAL_QUEUE())
 	{
 		/* One or more unblocked signals queued for execution */
-
-		int			exec_mask = pg_signal_queue & ~pg_signal_mask;
+		int			exec_mask = UNBLOCKED_SIGNAL_QUEUE();

 		for (i = 0; i < PG_SIGNAL_COUNT; i++)
 		{
Index: include/port/win32.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/port/win32.h,v
retrieving revision 1.47
diff -u -r1.47 win32.h
--- include/port/win32.h	15 Oct 2005 02:49:45 -0000	1.47
+++ include/port/win32.h	21 Oct 2005 05:33:26 -0000
@@ -214,6 +214,12 @@

 /* In backend/port/win32/signal.c */
+extern DLLIMPORT volatile int pg_signal_queue;
+extern DLLIMPORT int pg_signal_mask;
+
+#define UNBLOCKED_SIGNAL_QUEUE()	\
+		(pg_signal_queue & ~pg_signal_mask)
+
 extern DLLIMPORT HANDLE pgwin32_signal_event;
 extern HANDLE pgwin32_initial_signal_pipe;

Index: include/miscadmin.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/miscadmin.h,v
retrieving revision 1.180
diff -u -r1.180 miscadmin.h
--- include/miscadmin.h	15 Oct 2005 02:49:41 -0000	1.180
+++ include/miscadmin.h	21 Oct 2005 05:33:56 -0000
@@ -87,8 +87,10 @@

 #define CHECK_FOR_INTERRUPTS() \
 do { \
-	if (WaitForSingleObjectEx(pgwin32_signal_event,0,TRUE) == WAIT_OBJECT_0) \
-		pgwin32_dispatch_queued_signals(); \
+	if (UNBLOCKED_SIGNAL_QUEUE())	{ \
+		if (WaitForSingleObjectEx(pgwin32_signal_event,0,TRUE) == WAIT_OBJECT_0) \
+			pgwin32_dispatch_queued_signals(); \
+	}	\
 	if (InterruptPending) \
 		ProcessInterrupts(); \
 } while(0)

Also, I have a small style question - why use a nested if instead of
just saying

if (UNBLOCKED_SIGNAL_QUEUE() &&
WaitForSingleObjectEx(pgwin32_signal_event,0,TRUE) == WAIT_OBJECT_0)

Yeah, this works but IMHO that style states things clearer,

Regards,
Qingqing

BTW, expanding on Andrew's comment, ISTM we could move the kernel call
out of the macro, ie make it look like

#define CHECK_FOR_INTERRUPTS() \
do { \
if (UNBLOCKED_SIGNAL_QUEUE()) \
pgwin32_check_queued_signals(); \
if (InterruptPending) \
ProcessInterrupts(); \
} while(0)

where pgwin32_check_queued_signals() is just

if (WaitForSingleObjectEx(pgwin32_signal_event,0,TRUE) == WAIT_OBJECT_0)
pgwin32_dispatch_queued_signals();

This would save a few bytes at each call site while not really costing
anything in performance.

regards, tom lane

I wrote:

BTW, expanding on Andrew's comment, ISTM we could move the kernel call
out of the macro, ie make it look like ...

I've applied the attached version of Qingqing's revised patch. I'm not
in a position to test it, and am about to go out for the evening, but if
anyone can check the committed version soon and verify that I didn't
break anything ...

(BTW, DLLIMPORT is only needed in extern declarations, not in the actual
definition of the variable.)

regards, tom lane

*** src/backend/port/win32/signal.c.orig	Fri Oct 14 22:59:56 2005
--- src/backend/port/win32/signal.c	Fri Oct 21 17:36:24 2005
***************
*** 15,32 ****

#include <libpq/pqsignal.h>

!
! /* pg_signal_crit_sec is used to protect only pg_signal_queue. That is the only
! * variable that can be accessed from the signal sending threads! */
static CRITICAL_SECTION pg_signal_crit_sec;
- static int pg_signal_queue;

static pqsigfunc pg_signal_array[PG_SIGNAL_COUNT];
static pqsigfunc pg_signal_defaults[PG_SIGNAL_COUNT];
- static int pg_signal_mask;
-
- DLLIMPORT HANDLE pgwin32_signal_event;
- HANDLE pgwin32_initial_signal_pipe = INVALID_HANDLE_VALUE;

  /* Signal handling thread function */
--- 15,40 ----

#include <libpq/pqsignal.h>

! /*
! * These are exported for use by the UNBLOCKED_SIGNAL_QUEUE() macro.
! * pg_signal_queue must be volatile since it is changed by the signal
! * handling thread and inspected without any lock by the main thread.
! * pg_signal_mask is only changed by main thread so shouldn't need it.
! */
! volatile int pg_signal_queue;
! int pg_signal_mask;
!
! HANDLE pgwin32_signal_event;
! HANDLE pgwin32_initial_signal_pipe = INVALID_HANDLE_VALUE;
!
! /*
! * pg_signal_crit_sec is used to protect only pg_signal_queue. That is the only
! * variable that can be accessed from the signal sending threads!
! */
static CRITICAL_SECTION pg_signal_crit_sec;

static pqsigfunc pg_signal_array[PG_SIGNAL_COUNT];
static pqsigfunc pg_signal_defaults[PG_SIGNAL_COUNT];

/* Signal handling thread function */
***************
*** 81,101 ****
(errmsg_internal("failed to set console control handler")));
}

! /* Dispatch all signals currently queued and not blocked
* Blocked signals are ignored, and will be fired at the time of
! * the sigsetmask() call. */
void
pgwin32_dispatch_queued_signals(void)
{
int i;

EnterCriticalSection(&pg_signal_crit_sec);
! while (pg_signal_queue & ~pg_signal_mask)
{
/* One or more unblocked signals queued for execution */
!
! int exec_mask = pg_signal_queue & ~pg_signal_mask;

  		for (i = 0; i < PG_SIGNAL_COUNT; i++)
  		{
--- 89,119 ----
  				(errmsg_internal("failed to set console control handler")));
  }

+ /*
+  * Support routine for CHECK_FOR_INTERRUPTS() macro
+  */
+ void
+ pgwin32_check_queued_signals(void)
+ {
+ 	if (WaitForSingleObjectEx(pgwin32_signal_event, 0, TRUE) == WAIT_OBJECT_0)
+ 		pgwin32_dispatch_queued_signals();
+ }

! /*
! * Dispatch all signals currently queued and not blocked
* Blocked signals are ignored, and will be fired at the time of
! * the sigsetmask() call.
! */
void
pgwin32_dispatch_queued_signals(void)
{
int i;

EnterCriticalSection(&pg_signal_crit_sec);
! while (UNBLOCKED_SIGNAL_QUEUE())
{
/* One or more unblocked signals queued for execution */
! int exec_mask = UNBLOCKED_SIGNAL_QUEUE();

  		for (i = 0; i < PG_SIGNAL_COUNT; i++)
  		{
*** src/include/miscadmin.h.orig	Fri Oct 14 23:00:22 2005
--- src/include/miscadmin.h	Fri Oct 21 17:36:18 2005
***************
*** 83,97 ****
  	if (InterruptPending) \
  		ProcessInterrupts(); \
  } while(0)
  #else							/* WIN32 */

#define CHECK_FOR_INTERRUPTS() \
do { \
! if (WaitForSingleObjectEx(pgwin32_signal_event,0,TRUE) == WAIT_OBJECT_0) \
! pgwin32_dispatch_queued_signals(); \
if (InterruptPending) \
ProcessInterrupts(); \
} while(0)
#endif /* WIN32 */

--- 83,99 ----
  	if (InterruptPending) \
  		ProcessInterrupts(); \
  } while(0)
+ 
  #else							/* WIN32 */

#define CHECK_FOR_INTERRUPTS() \
do { \
! if (UNBLOCKED_SIGNAL_QUEUE()) \
! pgwin32_check_queued_signals(); \
if (InterruptPending) \
ProcessInterrupts(); \
} while(0)
+
#endif /* WIN32 */

*** src/include/port/win32.h.orig	Fri Oct 14 23:00:30 2005
--- src/include/port/win32.h	Fri Oct 21 17:36:12 2005
***************
*** 214,224 ****

/* In backend/port/win32/signal.c */
! extern DLLIMPORT HANDLE pgwin32_signal_event;
extern HANDLE pgwin32_initial_signal_pipe;

void pgwin32_signal_initialize(void);
HANDLE pgwin32_create_signal_listener(pid_t pid);
void pgwin32_dispatch_queued_signals(void);
void pg_queue_signal(int signum);

--- 214,230 ----

/* In backend/port/win32/signal.c */
! extern DLLIMPORT volatile int pg_signal_queue;
! extern DLLIMPORT int pg_signal_mask;
! extern HANDLE pgwin32_signal_event;
extern HANDLE pgwin32_initial_signal_pipe;

+ #define UNBLOCKED_SIGNAL_QUEUE()	(pg_signal_queue & ~pg_signal_mask)
+ 
+ 
  void		pgwin32_signal_initialize(void);
  HANDLE		pgwin32_create_signal_listener(pid_t pid);
+ void		pgwin32_check_queued_signals(void);
  void		pgwin32_dispatch_queued_signals(void);
  void		pg_queue_signal(int signum);

Heads up - I have seen 2 regression hangs. I am checking further. Has
anyone else run the regression suite with any version of this patch?

cheers

andrew

Tom Lane wrote:

And the patch that was applied gives the same result.

What is more, I am not seeing the reported speedup - in fact I am seeing
no speedup worth mentioning.

This is on XP-Pro, with default postgres settings. The test sets were
somewhat larger than thos Magnus used - basically TPC-H lineitems and
orders tables (6m and 1.5m rows respectively).

cheers

andrew

Andrew Dunstan wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Heads up - I have seen 2 regression hangs. I am checking further. Has
anyone else run the regression suite with any version of this patch?

Hm, anyone else? It's pretty hard to see how the patch could break
the regression tests, because they don't exercise control-C response
time.

Andrew, did you do a full rebuild after applying the patch? I don't
quite see why that would be needed either, but people have seen weird
failures from partial rebuilds before ...

regards, tom lane

On 22/10/05 3:45 am, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Heads up - I have seen 2 regression hangs. I am checking further. Has
anyone else run the regression suite with any version of this patch?

Hm, anyone else? It's pretty hard to see how the patch could break
the regression tests, because they don't exercise control-C response
time.

Yeah, I had to kill the buildfarm run on Snake as it was still stuck in make
check after 8 and a bit hours. :-(

Regards, Dave

Heads up - I have seen 2 regression hangs. I am checking

further. Has

anyone else run the regression suite with any version of this patch?

Hm, anyone else? It's pretty hard to see how the patch could
break the regression tests, because they don't exercise
control-C response time.

Andrew, did you do a full rebuild after applying the patch?
I don't quite see why that would be needed either, but people
have seen weird failures from partial rebuilds before ...

I can unfortunatly conform that I'm also seeing this :-( I'm seeing it
in some kind of tight loop in the plpgsql regression test. Either that,
or it's just doing something *really* slowly. Doing some poking at it
with procexp I see it always being somewhere in a callstack that's
around:

... <- hang
... <- several more levels
postgres.exe!ExecProcNode
postgres.exe!ExecutorRun
postgres.exe!spi_printtup
postgres.exe!SPI_execute_plan
plpgsql.dll!plpgsql_compile
... <- several more levels, of course

If I kill the pl/pgsql test, everything else runs fine.
(And removing just this patch makes it run fine again, so it's definitly
this one that causes it)

//Magnus

And the patch that was applied gives the same result.

What is more, I am not seeing the reported speedup - in fact
I am seeing no speedup worth mentioning.

This is on XP-Pro, with default postgres settings. The test
sets were somewhat larger than thos Magnus used - basically
TPC-H lineitems and orders tables (6m and 1.5m rows respectively).

First, that was Merlin and not me :-)

Second, it didn't really show any improvement for him either in his
normal test. But when he re-ran it with just the count(*) test it showed
improvement. Did you run a count(*) test or some other test?

//Magnus

Magnus Hagander wrote:

And the patch that was applied gives the same result.

What is more, I am not seeing the reported speedup - in fact
I am seeing no speedup worth mentioning.

This is on XP-Pro, with default postgres settings. The test
sets were somewhat larger than thos Magnus used - basically
TPC-H lineitems and orders tables (6m and 1.5m rows respectively).

First, that was Merlin and not me :-)

My apologies to both of you. Or to at least one of you, anyway. :-)

Second, it didn't really show any improvement for him either in his
normal test. But when he re-ran it with just the count(*) test it showed
improvement. Did you run a count(*) test or some other test?

The tests were

select count(*) from lineitems;
select count(*) from orders;

The table definitions are:

CREATE TABLE orders (
o_orderkey INTEGER NOT NULL,
o_custkey INTEGER NOT NULL,
o_orderstatus CHAR(1) NOT NULL,
o_totalprice DECIMAL(15,2) NOT NULL,
o_orderdate DATE NOT NULL,
o_orderpriority CHAR(15) NOT NULL, -- R
o_clerk CHAR(15) NOT NULL, -- R
o_shippriority INTEGER NOT NULL,
o_comment VARCHAR(79) NOT NULL
);

CREATE TABLE lineitem (
l_orderkey INTEGER NOT NULL,
l_partkey INTEGER NOT NULL,
l_suppkey INTEGER NOT NULL,
l_linenumber INTEGER NOT NULL,
l_quantity DECIMAL(15,2) NOT NULL,
l_extendedprice DECIMAL(15,2) NOT NULL,
l_discount DECIMAL(15,2) NOT NULL,
l_tax DECIMAL(15,2) NOT NULL,
l_returnflag CHAR(1) NOT NULL,
l_linestatus CHAR(1) NOT NULL,
l_shipdate DATE NOT NULL,
l_commitdate DATE NOT NULL,
l_receiptdate DATE NOT NULL,
l_shipinstruct CHAR(25) NOT NULL, -- R
l_shipmode CHAR(10) NOT NULL, -- R
l_comment VARCHAR(44) NOT NULL
);

I could make the whole dataset available, but tarred and zipped it's
about 300Mb. The reason I used this dataset was that I wanted to see a
test that took many seconds, and Merlin's did not - I wanted to see how
any performance gain scaled.

But it looks to me like we need to leave this for 8.2 now anyway, unless
someone can quickly get to the bottom of why it's hanging.

cheers

andrew

And the patch that was applied gives the same result.

What is more, I am not seeing the reported speedup - in fact I am
seeing no speedup worth mentioning.

This is on XP-Pro, with default postgres settings. The test

sets were

somewhat larger than thos Magnus used - basically TPC-H

lineitems and

orders tables (6m and 1.5m rows respectively).

First, that was Merlin and not me :-)

My apologies to both of you. Or to at least one of you, anyway. :-)

:-)

Second, it didn't really show any improvement for him either in his
normal test. But when he re-ran it with just the count(*) test it
showed improvement. Did you run a count(*) test or some other test?

The tests were

<snip>

That certainly looks like a similar test. Just checking :-)

You were on XP - Merlin, what OS were you on? It could be possible
things have chenged there as well...

But it looks to me like we need to leave this for 8.2 now
anyway, unless someone can quickly get to the bottom of why
it's hanging.

Agreed. I definitly vote for backing out the patch so we don't block the
release. If we find the problem we put it back in before release, but if
it takes a while we just wait for 8.2.

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

I can unfortunatly conform that I'm also seeing this :-( I'm seeing it
in some kind of tight loop in the plpgsql regression test. Either that,
or it's just doing something *really* slowly. Doing some poking at it
with procexp I see it always being somewhere in a callstack that's
around:

Which test command is it executing exactly? I'm wondering about the
part of the test that exercises statement_timeout. Could we somehow
have broken the ability to detect timeout interrupts ... and if so, how?

Has anyone checked whether the backend still responds to SIGINT
with the patch in place?

regards, tom lane

I can unfortunatly conform that I'm also seeing this :-(

I'm seeing it

in some kind of tight loop in the plpgsql regression test. Either
that, or it's just doing something *really* slowly. Doing

some poking

at it with procexp I see it always being somewhere in a callstack
that's
around:

Which test command is it executing exactly? I'm wondering
about the part of the test that exercises statement_timeout.
Could we somehow have broken the ability to detect timeout
interrupts ... and if so, how?

Does that test run in the pl/pgsql test, using pl/pgsql code? If so it's
quite likely - it doesn't crash, and it does *something*. And it's
signal related... Could be if we waited long enough (>8 hours!) it goes
on with an error.

Has anyone checked whether the backend still responds to
SIGINT with the patch in place?

Nope, sorry.

If nobody else beats me, I can hopefully take alook at it tomorrow.
Just heading out now, so I can't check now.

//Magnus

Tom Lane wrote:

"Magnus Hagander" <mha@sollentuna.net> writes:

I can unfortunatly conform that I'm also seeing this :-( I'm seeing it
in some kind of tight loop in the plpgsql regression test. Either that,
or it's just doing something *really* slowly. Doing some poking at it
with procexp I see it always being somewhere in a callstack that's
around:

Which test command is it executing exactly? I'm wondering about the
part of the test that exercises statement_timeout. Could we somehow
have broken the ability to detect timeout interrupts ... and if so, how?

It appears to hang in blockme() - so your guess seems correct.

Has anyone checked whether the backend still responds to SIGINT
with the patch in place?

After it hung, I issued pg_ctl -m fast -w stop and it stopped cleanly.

cheers

andrew

"Magnus Hagander" <mha@sollentuna.net> writes:

Agreed. I definitly vote for backing out the patch so we don't block the
release. If we find the problem we put it back in before release, but if
it takes a while we just wait for 8.2.

After digging in the Microsoft documentation a little bit, I think I
see the problem: we implement SIGALRM timeouts by means of a "waitable
timer", and the system's action when the timeout expires is:

When a timer expires, the timer is set to the signaled state. If
the timer has a completion routine, it is queued to the thread
that set the timer. The completion routine remains in the APC
queue of the thread until the threads enters an alertable wait
state. At that time, the APC is dispatched and the completion
routine is called.

In other words, since the main thread is the one that set the timer,
it has to run the completion routine (which just sets the signal flag).
The completion routine will never be run if the main thread is in a
tight loop with no kernel calls. In particular, it was the frequent
execution of WaitForSingleObjectEx via CHECK_FOR_INTERRUPTS that allowed
this technique to work at all.

Isn't there some way we can get the timer completion routine to be run
by the signal thread instead? This coding seems pretty unreliable to me
even without QQ's patch.

regards, tom lane

I wrote:

Isn't there some way we can get the timer completion routine to be run
by the signal thread instead? This coding seems pretty unreliable to me
even without QQ's patch.

After further thought it seems like the right thing to do is to redesign
port/win32/timer.c so that it sets up a separate thread whose
responsibility is to wait for timeouts and deliver a SIGALRM signal back
to the main thread when they happen. It's probably a bit late to
consider doing this for 8.1 :-(

I've temporarily disabled Qingqing's patch by the expedient of removing
the UNBLOCKED_SIGNAL_QUEUE() check in the macro, so that the out-of-line
routine is always called (since we're going to do a kernel call anyway,
one extra layer of subroutine call doesn't seem very important). We can
put it back after fixing timer.c.

regards, tom lane

Andrew Dunstan <andrew@dunslane.net> writes:

I could make the whole dataset available, but tarred and zipped it's
about 300Mb. The reason I used this dataset was that I wanted to see a
test that took many seconds, and Merlin's did not - I wanted to see how
any performance gain scaled.

Well, you tried to "scale" into a domain where the performance is going
to be disk-I/O-limited, so I'm not sure it proves anything.

regards, tom lane

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

I could make the whole dataset available, but tarred and zipped it's
about 300Mb. The reason I used this dataset was that I wanted to see a
test that took many seconds, and Merlin's did not - I wanted to see how
any performance gain scaled.

Well, you tried to "scale" into a domain where the performance is going
to be disk-I/O-limited, so I'm not sure it proves anything.

Good point. I took a 5% random extract from the lineitems table and saw
the expected improvement.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

Tom Lane wrote:

Well, you tried to "scale" into a domain where the performance is going
to be disk-I/O-limited, so I'm not sure it proves anything.

Good point. I took a 5% random extract from the lineitems table and saw
the expected improvement.

Sounds better. Certainly there are cases where CHECK_FOR_INTERRUPTS
isn't going to be a meaningful drag on performance, but there are others
where it will be.

BTW, looking at the code some more, I am thinking that checking
pgwin32_signal_event should be completely unnecessary in
pgwin32_check_queued_signals; that is, if UNBLOCKED_SIGNAL_QUEUE() is
nonzero we might as well just enter pgwin32_dispatch_queued_signals
unconditionally. The only usefulness of calling WaitForSingleObjectEx
is to allow any pending APCs to be dispatched. Are there any other
APCs queued against the main thread besides the timer.c one?

regards, tom lane

Tom Lane wrote:

After further thought it seems like the right thing to do is to redesign
port/win32/timer.c so that it sets up a separate thread whose
responsibility is to wait for timeouts and deliver a SIGALRM signal back
to the main thread when they happen. It's probably a bit late to
consider doing this for 8.1 :-(

The hard part looks to be cancelling/changing the timer, which means
that we can't just create a set and forget listener thread for a given
timeout. Otherwise that seems to me the straightforward approach.

But maybe one of the Windows wizards has a better idea.

I doubt the changes would be very invasive - with luck just confined to
timer.c.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

The hard part looks to be cancelling/changing the timer, which means
that we can't just create a set and forget listener thread for a given
timeout. Otherwise that seems to me the straightforward approach.

Yeah. I think probably the cleanest way is to create a persistent
thread that manages the timer. We need a way for the main thread to
tell it to cancel the timer or change the setting. Dunno enough about
Windows' interthread communication primitives to propose details.

I doubt the changes would be very invasive - with luck just confined to
timer.c.

I don't see a need for anything else to know about it, either.

regards, tom lane

Andrew Dunstan <andrew@dunslane.net> writes:

The hard part looks to be cancelling/changing the timer, which means
that we can't just create a set and forget listener thread for a given
timeout. Otherwise that seems to me the straightforward approach.

Yeah. I think probably the cleanest way is to create a persistent
thread that manages the timer. We need a way for the main thread to
tell it to cancel the timer or change the setting. Dunno enough about
Windows' interthread communication primitives to propose details.

Oh my ... fortunately we got a timer test in regression.

I've come up with a quick patch implementing above discussions. Also,
seems by patching this, we can support setitimer(.,.,ovalue != NULL) --
because it is saved in the memory.

Regards,
Qingqing

---

Index: timer.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/port/win32/timer.c,v
retrieving revision 1.5
diff -u -r1.5 timer.c
--- timer.c	31 Dec 2004 22:00:37 -0000	1.5
+++ timer.c	23 Oct 2005 00:53:56 -0000
@@ -15,8 +15,16 @@

#include "libpq/pqsignal.h"

+/* Communication area of timer settings */
+typedef struct timerCA{
+	int which;
+	struct itimerval value;
+	HANDLE event;
+}timerCA;

+static timerCA timerCommArea;
 static HANDLE timerHandle = INVALID_HANDLE_VALUE;
+static HANDLE timerThreadHandle = INVALID_HANDLE_VALUE;

 static VOID CALLBACK
 timer_completion(LPVOID arg, DWORD timeLow, DWORD timeHigh)
@@ -28,16 +36,14 @@
 /*
  * Limitations of this implementation:
  *
- * - Does not support setting ovalue
  * - Does not support interval timer (value->it_interval)
  * - Only supports ITIMER_REAL
  */
-int
-setitimer(int which, const struct itimerval * value, struct itimerval * ovalue)
+static int
+do_setitimer(int which, const struct itimerval * value)
 {
 	LARGE_INTEGER dueTime;

- Assert(ovalue == NULL);
Assert(value != NULL);
Assert(value->it_interval.tv_sec == 0 && value->it_interval.tv_usec == 0);
Assert(which == ITIMER_REAL);
@@ -69,3 +75,56 @@

 	return 0;
 }
+
+/* Timer ticking thread */
+static DWORD WINAPI
+pg_timer_thread(LPVOID param)
+{
+	Assert(param == NULL);
+
+	for (;;)
+	{
+		if (WaitForSingleObjectEx(timerCommArea.event, INFINITE, TRUE) == WAIT_OBJECT_0)
+		{
+			do_setitimer(timerCommArea.which, &timerCommArea.value);
+			ResetEvent(timerCommArea.event);
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * Win32 setitimer emulation by creating a persistent thread
+ * to handle the timer setting and notification upon timeout.
+ */
+int
+setitimer(int which, const struct itimerval * value, struct itimerval * ovalue)
+{
+	Assert(value != NULL);
+
+	if (timerThreadHandle == INVALID_HANDLE_VALUE)
+	{
+		/* First call in this backend, create event and the timer thread */
+		timerCommArea.event = CreateEvent(NULL, TRUE, FALSE, NULL);
+		if (timerCommArea.event == NULL)
+			ereport(FATAL,
+				(errmsg_internal("failed to create timer event: %d", (int) GetLastError())));
+		MemSet(&timerCommArea.value, 0, sizeof(struct itimerval));
+
+		timerThreadHandle = CreateThread(NULL, 0, pg_timer_thread, NULL, 0, NULL);
+		if (timerThreadHandle == INVALID_HANDLE_VALUE)
+			ereport(FATAL,
+				(errmsg_internal("failed to create timer thread: %d", (int) GetLastError())));
+	}
+
+	/* Request the timer thread to change settings */
+	if (ovalue)
+		*ovalue = timerCommArea.value;
+	timerCommArea.which = which;
+	timerCommArea.value = *value;
+	SetEvent(timerCommArea.event);
+
+	/* Timer thread will handle possible errors */
+	return 0;
+}

Well, first, have you tested it with "make check"? I am not sure if
there's any great value in supporting a non null old value param.

Second, please note that the PostgreSQL community convention is for
patches as context diffs, not unidiffs.

I guess there are several ways to skin this cat - the way I had sort of
worked out reading the MSDN docs was to call QueueUserAPC on the timer
thread. I'd like to know what Magnus and Merlin especially think out it.

cheers

andrew

Qingqing Zhou wrote:

On Sat, 22 Oct 2005, Andrew Dunstan wrote:

Well, first, have you tested it with "make check"? I am not sure if
there's any great value in supporting a non null old value param.

Yeah, I've managed to install in my slow windows box and tested it.
Supporting ovalue is just the by-product. If it works, I will complete the
patch by adding threading safe critical section protect.

Second, please note that the PostgreSQL community convention is for
patches as context diffs, not unidiffs.

Ok.

I guess there are several ways to skin this cat - the way I had sort of
worked out reading the MSDN docs was to call QueueUserAPC on the timer
thread. I'd like to know what Magnus and Merlin especially think out it.

I am not sure - does this not require another thread in an alterable
state?

Regards,
Qingqing

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.

Regards,
Qingqing

---

Index: timer.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/port/win32/timer.c,v
retrieving revision 1.5
diff -c -r1.5 timer.c
*** timer.c	31 Dec 2004 22:00:37 -0000	1.5
--- timer.c	23 Oct 2005 03:04:53 -0000
***************
*** 15,22 ****
--- 15,31 ----

#include "libpq/pqsignal.h"

+ /* Communication area of timer settings */
+ typedef struct timerCA{
+ 	int which;
+ 	struct itimerval value;
+ 	HANDLE event;
+ 	CRITICAL_SECTION crit_sec;
+ }timerCA;

+ static timerCA timerCommArea;
  static HANDLE timerHandle = INVALID_HANDLE_VALUE;
+ static HANDLE timerThreadHandle = INVALID_HANDLE_VALUE;

static VOID CALLBACK
timer_completion(LPVOID arg, DWORD timeLow, DWORD timeHigh)
***************
*** 24,43 ****
pg_queue_signal(SIGALRM);
}

-
/*
* Limitations of this implementation:
*
- * - Does not support setting ovalue
* - Does not support interval timer (value->it_interval)
* - Only supports ITIMER_REAL
*/
! int
! setitimer(int which, const struct itimerval * value, struct itimerval * ovalue)
{
LARGE_INTEGER dueTime;

- 	Assert(ovalue == NULL);
  	Assert(value != NULL);
  	Assert(value->it_interval.tv_sec == 0 && value->it_interval.tv_usec == 0);
  	Assert(which == ITIMER_REAL);
--- 33,49 ----
  	pg_queue_signal(SIGALRM);
  }

/*
* Limitations of this implementation:
*
* - Does not support interval timer (value->it_interval)
* - Only supports ITIMER_REAL
*/
! static int
! do_setitimer(int which, const struct itimerval * value)
{
LARGE_INTEGER dueTime;

  	Assert(value != NULL);
  	Assert(value->it_interval.tv_sec == 0 && value->it_interval.tv_usec == 0);
  	Assert(which == ITIMER_REAL);
***************
*** 69,71 ****
--- 75,136 ----

  	return 0;
  }
+
+ /* Timer ticking thread */
+ static DWORD WINAPI
+ pg_timer_thread(LPVOID param)
+ {
+ 	Assert(param == NULL);
+
+ 	for (;;)
+ 	{
+ 		if (WaitForSingleObjectEx(timerCommArea.event, INFINITE, TRUE) == WAIT_OBJECT_0)
+ 		{
+ 			EnterCriticalSection(&timerCommArea.crit_sec);
+ 			do_setitimer(timerCommArea.which,
+ 						&timerCommArea.value);
+ 			ResetEvent(timerCommArea.event);
+ 			LeaveCriticalSection(&timerCommArea.crit_sec);
+ 		}
+ 	}
+
+ 	return 0;
+ }
+
+ /*
+  * Win32 setitimer emulation by creating a persistent thread
+  * to handle the timer setting and notification upon timeout.
+  */
+ int
+ setitimer(int which, const struct itimerval * value, struct itimerval * ovalue)
+ {
+ 	Assert(value != NULL);
+
+ 	if (timerThreadHandle == INVALID_HANDLE_VALUE)
+ 	{
+ 		/* First call in this backend, create event and the timer thread */
+ 		timerCommArea.event = CreateEvent(NULL, TRUE, FALSE, NULL);
+ 		if (timerCommArea.event == NULL)
+ 			ereport(FATAL,
+ 				(errmsg_internal("failed to create timer event: %d", (int) GetLastError())));
+ 		MemSet(&timerCommArea.value, 0, sizeof(struct itimerval));
+ 		InitializeCriticalSection(&timerCommArea.crit_sec);
+
+ 		timerThreadHandle = CreateThread(NULL, 0, pg_timer_thread, NULL, 0, NULL);
+ 		if (timerThreadHandle == INVALID_HANDLE_VALUE)
+ 			ereport(FATAL,
+ 				(errmsg_internal("failed to create timer thread: %d", (int) GetLastError())));
+ 	}
+
+ 	/* Request the timer thread to change settings */
+ 	EnterCriticalSection(&timerCommArea.crit_sec);
+ 	if (ovalue)
+ 		*ovalue = timerCommArea.value;
+ 	timerCommArea.which = which;
+ 	timerCommArea.value = *value;
+ 	LeaveCriticalSection(&timerCommArea.crit_sec);
+ 	SetEvent(timerCommArea.event);
+
+ 	/* Timer thread will handle possible errors */
+ 	return 0;
+ }

Qingqing Zhou wrote:

I guess there are several ways to skin this cat - the way I had sort of
worked out reading the MSDN docs was to call QueueUserAPC on the timer
thread. I'd like to know what Magnus and Merlin especially think out it.

I am not sure - does this not require another thread in an alterable
state?

Maybe, I don't know. My impression from the docs was that the thread
could call WaitForSingleObjectEx on the timer handle and it would also
respond to the APC call. But my Windows API knowledge is not exactly
large. As long as what you have works it should be OK.

cheers

andrew

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.

Tom Lane wrote:

Well, you tried to "scale" into a domain where the performance is
going to be disk-I/O-limited, so I'm not sure it proves anything.

Good point. I took a 5% random extract from the lineitems table and
saw the expected improvement.

Sounds better. Certainly there are cases where
CHECK_FOR_INTERRUPTS isn't going to be a meaningful drag on
performance, but there are others where it will be.

BTW, looking at the code some more, I am thinking that
checking pgwin32_signal_event should be completely
unnecessary in pgwin32_check_queued_signals; that is, if
UNBLOCKED_SIGNAL_QUEUE() is nonzero we might as well just
enter pgwin32_dispatch_queued_signals unconditionally. The
only usefulness of calling WaitForSingleObjectEx is to allow
any pending APCs to be dispatched. Are there any other APCs
queued against the main thread besides the timer.c one?

Right, the check should be unnecessary. We need the event, because we
wait on it in the socket layer and in pgwin32_backend_usleep(). But as
long as the event is still reset in pgwin32_dispatch_queued_signals,
that should continue to work just as before.

There should be no other APCs. After we learned that we can't do socket
operations in APCs, we got rid of all other instances (signals code
initially used it a lot). (Plus, if there were other APCs, they won't
get called anyway for the same reason the timer APC wasn't called any
more..)

(Tried removing it in my code, and it still passes regression tests and
deadlock test, so it looks good. I kept the
pgwin32_check_queued_signals, but we could just as well get rid of that
one then, since it'll only contain a call to dispatch. I just didn't
want to have to do a complete rebuild after changing the macro..)

//Magnus

"Magnus Hagander" <mha@sollentuna.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

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.

Definitly agreed. I'd like to see both Merlin and Andrew run through the
same test suites they did before. And it wouldn't hurt to get a test run
on a differnet OS - so far I've only tested on XP SP2. I doubt there
would be OS specific differences, but jus tin case...

//Magnus

On Sun, 23 Oct 2005, Magnus Hagander wrote:

Here's another version of this patch ;-) I've based it on

your patch,

so the changes to ovalue etc should sitill be there.

I didn't find a thread saying above? Something wrong with my newsreader?

Regards,
Qingqing

On Sun, 23 Oct 2005, Magnus Hagander wrote:

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.

[ Finally I copied it from the website. Don't know why I can't find it in
my email reader or news reader. ]

Yeah, this is better.

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.

Yeah, several times I want to submit with an attachment, but I got an
error message saying that "I can't post a binary to a non-binary group".
If any one happens to use pine newsreader and know the solution, please
let me know, thanks.

Regards,
Qingqing

"Magnus Hagander" <mha@sollentuna.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

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...

That does seem right. I think the only reason it was there i nthe first
place was to deliver the APCs.

But. in theory, we can get a false positive from
UNBLOCKED_SIGNAL_QUEUE(), right? Since we do it unlocked between two
threads. If we do that, we'll "recover" in dispatch_signals, because
we'l lcheck again locked and not dispatch any signals. *but*. If this
happens, we will return EINTR even when there is no signal. That doesn't
seem correct to me. It's a very small window, but it should be possible,
no?

We probably need an actual check, so for example have
dispatch_queued_signals return a value indicating if any signals were
actually dispatched, and use that to control EINTR?

Comments? Or am I completely off being too tired right now? ;-)

//Magnus

On Sun, 23 Oct 2005, Tom Lane wrote:

"Magnus Hagander" <mha@sollentuna.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

On Sun, 23 Oct 2005, Magnus Hagander wrote:

But. in theory, we can get a false positive from
UNBLOCKED_SIGNAL_QUEUE(), right? Since we do it unlocked between two
threads. If we do that, we'll "recover" in dispatch_signals, because
we'l lcheck again locked and not dispatch any signals. *but*. If this
happens, we will return EINTR even when there is no signal. That doesn't
seem correct to me. It's a very small window, but it should be possible,
no?

We probably need an actual check, so for example have
dispatch_queued_signals return a value indicating if any signals were
actually dispatched, and use that to control EINTR?

Comments? Or am I completely off being too tired right now? ;-)

You are not. Basically that's what I just sent an email about :-) Since
signals are not quite often happened, so I am thinking just adding a
UNBLOCKED_SIGNAL_QUEUE() is more safe maybe for now.

Regards,
Qingqing

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.

Actually, wasn't the latest that we *only* use the event object in order
to "break out" of blocking operations in the socket code? And only use
checking + checking in critical seciton to deliver? In this case, it
shouldn't matter when the event is set as long as it's always set
*after* we update the values.

Meaning that we'd have for the check just:
UNBLOCKED_SIGNAL_QUEUE()
enter_critical_section()
UNBLOCKE_DSIGNAL_QUEUE() again
leave_critical_section()

and when we use the event to break out, tha'ts just a WFSO before that
whole cycle runs.

(Don't have the code around right now, and it's getting a bit late after
a long day, so forgive me if I'm missing something obvious in that
reasoning :P)

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

But. in theory, we can get a false positive from
UNBLOCKED_SIGNAL_QUEUE(), right?

We could have gotten a false positive from the old coding, too.
The event was certainly not any more tightly tied to the presence
of an unserviced signal flag than UNBLOCKED_SIGNAL_QUEUE, and arguably
less so.

I think this concern is irrelevant anyway. Returning EINTR from
select() is OK even if no signal was actually serviced, so long as
it doesn't recur indefinitely. EINTR just means "I didn't do the
select(), try again".

regards, tom lane

Qingqing Zhou <zhouqq@cs.toronto.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

But. in theory, we can get a false positive from
UNBLOCKED_SIGNAL_QUEUE(), right?

We could have gotten a false positive from the old coding, too.
The event was certainly not any more tightly tied to the
presence of an unserviced signal flag than
UNBLOCKED_SIGNAL_QUEUE, and arguably less so.

I think this concern is irrelevant anyway. Returning EINTR from
select() is OK even if no signal was actually serviced, so
long as it doesn't recur indefinitely. EINTR just means "I
didn't do the select(), try again".

Ok. I don't see any way why it would recur indefinitly, since we'll
clean it up in the dispatch routine. And any half-updated-value will be
fully updated a *very* short time later. So we sohuld be fine.

//Magnus

Tom Lane wrote:

"Magnus Hagander" <mha@sollentuna.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@dunslane.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

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?

Send the backend a signal when it's blocking for socket input (waiting
for a user command in a psql session for example), and see that it's
delivered. Hitting hte postmaster will test the select() path, and
hitting a backend will test the recv() path, IIRC.

//Magnus

OK, running the latest patch. Observations:
1. Confirmed that time for count(*) on narrow sets is greatly improved:
A real easy way to show this off is:

select count(*) from generate_series(1,(10^6)::integer);

with about a 60% drop in time on my XP box.

2. Did ISAM style record iteration over relatively large (50k records)
bill of materials. This was previous test where I reported no
performance gain...for a single user (n=1). For n=6, I am seeing about
a 20% reduction in the overall running time of the test, and a greatly
improved overall responsiveness of the server while running the test.

I tested this by doing select * from medium_sized_table while 6 backends
were busy feeding records to the ISAM app. While pg under 0 load might
serve the table in one second, with the n=6 test running the time might
(on win32) take 5 minutes. On linux, the worst case time was around
1/(n*2). Qingqing's patch brings win32 much closer to linux!

3. A pl/pgsql function stuck in a empty loop is unkillable except by
killing the process on the server, which cycles the entire server. This
was the behavior before the patch, btw.

I ran tests for about an hour, randomly killing/canceling backends
without any problems.

Merlin

"Merlin Moncure" <merlin.moncure@rcsonline.com> writes:

3. A pl/pgsql function stuck in a empty loop is unkillable except by
killing the process on the server, which cycles the entire server. This
was the behavior before the patch, btw.

Hmm, that suggests we need another CHECK_FOR_INTERRUPTS somewhere in
plpgsql. Please show the exact test case you were using.

regards, tom lane

Tom wrote:

"Merlin Moncure" <merlin.moncure@rcsonline.com> writes:

3. A pl/pgsql function stuck in a empty loop is unkillable except

by

killing the process on the server, which cycles the entire server.

This

was the behavior before the patch, btw.

Hmm, that suggests we need another CHECK_FOR_INTERRUPTS somewhere in
plpgsql. Please show the exact test case you were using.

create function test_func() returns int4 as $$ BEGIN LOOP END LOOP;
select 0; END; $$ language plpgsql;

select test_func(); // :-).

Tom Lane wrote:

"Merlin Moncure" <merlin.moncure@rcsonline.com> writes:

3. A pl/pgsql function stuck in a empty loop is unkillable except by
killing the process on the server, which cycles the entire server. This
was the behavior before the patch, btw.

Hmm, that suggests we need another CHECK_FOR_INTERRUPTS somewhere in
plpgsql. Please show the exact test case you were using.

We might be able to solve that for plpgsql, but in general we can't,
ISTM. What if I write a plperl function that loops forever? We have no
chance there to call CHECK_FOR_INTERRUPTS.

cheers

andrew

"Merlin Moncure" <merlin.moncure@rcsonline.com> writes:

Tom wrote:

Hmm, that suggests we need another CHECK_FOR_INTERRUPTS somewhere in
plpgsql. Please show the exact test case you were using.

create function test_func() returns int4 as $$ BEGIN LOOP END LOOP;
select 0; END; $$ language plpgsql;

Thanks, fixed.

regards, tom lane

Andrew Dunstan <andrew@dunslane.net> writes:

We might be able to solve that for plpgsql, but in general we can't,
ISTM. What if I write a plperl function that loops forever? We have no
chance there to call CHECK_FOR_INTERRUPTS.

Yeah, I was thinking about that too. Still, we can/should/already did
fix it in plpgsql.

regards, tom lane

On Mon, 24 Oct 2005, Magnus Hagander wrote:

Is there any way to test socket?

Send the backend a signal when it's blocking for socket input (waiting
for a user command in a psql session for example), and see that it's
delivered. Hitting hte postmaster will test the select() path, and
hitting a backend will test the recv() path, IIRC.

Ok.

- Test recv() path by runing a psql with "-f time_out.sql" and a batch
file executing "pg_ctl kill HUP/USR1" repeatedly.
- Test select() path by running two batch files, one is doing "psql -c
"set statement_timeout=10" test", the other is doing "pg_ctl kill HUP
<postmaster>".

Regards,
Qingqing

"Merlin Moncure" <merlin.moncure@rcsonline.com> writes:

OK, running the latest patch. Observations:
...
I ran tests for about an hour, randomly killing/canceling backends
without any problems.

Are we all comfortable that
http://archives.postgresql.org/pgsql-hackers/2005-10/msg01009.php
is OK to apply?

regards, tom lane

On Mon, 24 Oct 2005, Tom Lane wrote:

Are we all comfortable that
http://archives.postgresql.org/pgsql-hackers/2005-10/msg01009.php
is OK to apply?

regards, tom lane

I tried to persuade myself that removing all WaitForSingleObjectEx() is
safe ... the thing is we will false alarm EINTR as Magnus said (details to
repeat it are list below in case). There are several EINTR in the code,
semop() calls, socket() calls, ..., seems they are all ok except
pgwin32_backend_usleep() changes a little bit performance: it can't sleep
enough because of the false alarm, but it is ok though.

Conclusion: Agree to apply.

Regards,
Qingqing

---
Consider a sequence like this:

1. I am killing you signal A:

enter_crit;
set signal bit;
leave_crit;

2. You CHECK_FOR_INTERRUPTS():
enter_crit;
sig(A);
ResetEvent();
leave_crit;

3. I finish my killing:
SetEvent();

Now the event is signaled but the signal is handled already.

Qingqing Zhou <zhouqq@cs.toronto.edu> writes:

I tried to persuade myself that removing all WaitForSingleObjectEx() is
safe ... the thing is we will false alarm EINTR as Magnus said (details to
repeat it are list below in case).

Just to repeat myself: there were false alarms before. The interleaving
you describe could equally well happen if a new signal is sent just
after the old code executes WaitForSingleObjectEx and sees that a
previous signal is waiting for it. Both old and new signals can be
cleared by the recipient before the second signal sender gets as far as
setting the event.

regards, tom lane

On Mon, 24 Oct 2005, Tom Lane wrote:

Qingqing Zhou <zhouqq@cs.toronto.edu> writes:

I tried to persuade myself that removing all WaitForSingleObjectEx() is
safe ... the thing is we will false alarm EINTR as Magnus said (details to
repeat it are list below in case).

Just to repeat myself: there were false alarms before. The interleaving
you describe could equally well happen if a new signal is sent just
after the old code executes WaitForSingleObjectEx and sees that a
previous signal is waiting for it. Both old and new signals can be
cleared by the recipient before the second signal sender gets as far as
setting the event.

Oh, yeah. Just write the detailed case down for the sake of memory:

-- For previous code -- false alarm case --
1. I am killing you signal A:
enter_crit;
set signal bit;
leave_crit;

2. He *has killed* you signal B:

3. You CHECK_FOR_INTERRUPTS():
enter_crit;
sig(A);
sig(B);
ResetEvent();
leave_crit;

4. I finish my killing:
SetEvent();

Now the event is signaled but the signal is handled already.

Regards,
Qingqing

"Merlin Moncure" <merlin.moncure@rcsonline.com> writes:

OK, running the latest patch. Observations:
...
I ran tests for about an hour, randomly killing/canceling backends
without any problems.

Are we all comfortable that
http://archives.postgresql.org/pgsql-hackers/2005-10/msg01009.php
is OK to apply?

Yea. I can vouch for Magnus as well (he said so off list). I'd vote
with my own servers anyways.

Merlin

OK, running the latest patch. Observations:
...
I ran tests for about an hour, randomly killing/canceling

backends

without any problems.

Are we all comfortable that
http://archives.postgresql.org/pgsql-hackers/2005-10/msg01009.php
is OK to apply?

Yea. I can vouch for Magnus as well (he said so off list).
I'd vote with my own servers anyways.

Yup, and I can also vouch fo rmyself ;-) Just didn't get around to it
until now.

Yes, I believe we should be fine with that patch, even though we're late
in beta. It's a huge win, so I htink it's worth the small extra risk it
is.

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

Are we all comfortable that
http://archives.postgresql.org/pgsql-hackers/2005-10/msg01009.php
is OK to apply?

Yea. I can vouch for Magnus as well (he said so off list).
I'd vote with my own servers anyways.

Yup, and I can also vouch fo rmyself ;-) Just didn't get around to it
until now.

Yes, I believe we should be fine with that patch, even though we're late
in beta. It's a huge win, so I htink it's worth the small extra risk it
is.

OK, patch committed. Please recheck that CVS tip is OK.

regards, tom lane