Re: insert performance for win32

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

Nailed it.

problem is in mainloop.c -> setup_cancel_handler. Apparently you can
have multiple handlers and windows keeps track of them all, even if they
do the same thing. Keeping track of so many system handles would
naturally slow the whole process down.

Yipes. So we really want to do that only once.

AFAICS it is appropriate to move the sigsetjmp and setup_cancel_handler
calls in front of the per-line loop inside MainLoop --- can anyone see
a reason not to?

I'm inclined to treat this as an outright bug, not just a minor
performance issue, because it implies that a sufficiently long psql
script would probably crash a Windows machine.

regards, tom lane

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

Nailed it.

problem is in mainloop.c -> setup_cancel_handler. Apparently you

can

have multiple handlers and windows keeps track of them all, even if

they

do the same thing. Keeping track of so many system handles would
naturally slow the whole process down.

Yipes. So we really want to do that only once.

AFAICS it is appropriate to move the sigsetjmp and

setup_cancel_handler

calls in front of the per-line loop inside MainLoop --- can anyone see
a reason not to?

hm. mainloop is re-entrant, right? That means each \i would reset the
handler...what is downside to keeping global flag?

I'm inclined to treat this as an outright bug, not just a minor

certainly...

performance issue, because it implies that a sufficiently long psql
script would probably crash a Windows machine.

actually, it's worse than that, it's more of a dos on the whole system,
as windows will eventually stop granting handles, but there is a good
chance of side effects on other applications.

Merlin

On Fri, Nov 04, 2005 at 01:01:20PM -0500, Tom Lane wrote:

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

Nailed it.

problem is in mainloop.c -> setup_cancel_handler. Apparently you
can have multiple handlers and windows keeps track of them all,
even if they do the same thing. Keeping track of so many system
handles would naturally slow the whole process down.

Yipes. So we really want to do that only once.

AFAICS it is appropriate to move the sigsetjmp and
setup_cancel_handler calls in front of the per-line loop inside
MainLoop --- can anyone see a reason not to?

I'm inclined to treat this as an outright bug, not just a minor
performance issue, because it implies that a sufficiently long psql
script would probably crash a Windows machine.

Ouch. In light of this, are we *sure* what we've got a is a candidate
for release?

Cheers,
D
--
David Fetter david@fetter.org http://fetter.org/
phone: +1 510 893 6100 mobile: +1 415 235 3778

Remember to vote!

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

AFAICS it is appropriate to move the sigsetjmp and
setup_cancel_handler
calls in front of the per-line loop inside MainLoop --- can anyone see
a reason not to?

hm. mainloop is re-entrant, right? That means each \i would reset the
handler...what is downside to keeping global flag?

Ah, right, and in fact I'd missed the comment at line 325 pointing out
that we're relying on the sigsetjmp to be re-executed every time
through. That could be improved on, likely, but not right before a
release.

Does the flag need to be global? I'm thinking

void
setup_cancel_handler(void)
{
+ static bool done = false;
+
+ if (!done)
SetConsoleCtrlHandler(consoleHandler, TRUE);
+ done = true;
}

regards, tom lane

Tom Lane wrote:

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

Nailed it.

problem is in mainloop.c -> setup_cancel_handler. Apparently you can
have multiple handlers and windows keeps track of them all, even if they
do the same thing. Keeping track of so many system handles would
naturally slow the whole process down.

Yipes. So we really want to do that only once.

AFAICS it is appropriate to move the sigsetjmp and setup_cancel_handler
calls in front of the per-line loop inside MainLoop --- can anyone see
a reason not to?

Nope.

I'm inclined to treat this as an outright bug, not just a minor
performance issue, because it implies that a sufficiently long psql
script would probably crash a Windows machine.

Agreed.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

David Fetter <david@fetter.org> writes:

On Fri, Nov 04, 2005 at 01:01:20PM -0500, Tom Lane wrote:

I'm inclined to treat this as an outright bug, not just a minor
performance issue, because it implies that a sufficiently long psql
script would probably crash a Windows machine.

Ouch. In light of this, are we *sure* what we've got a is a candidate
for release?

Sure. This problem exists in 8.0.* too. Pre-existing bugs don't
disqualify an RC in my mind --- we fix them and move on, same as we
would do at any other time.

regards, tom lane

I'm inclined to treat this as an outright bug, not just a minor

certainly...

performance issue, because it implies that a sufficiently long psql
script would probably crash a Windows machine.

actually, it's worse than that, it's more of a dos on the
whole system, as windows will eventually stop granting
handles, but there is a good chance of side effects on other
applications.

Does it actually use up *handles* there? I don't see anything in the
docs that says it should do that - and they usually do document when
handles are used. You should be seeing a *huge* increase in system
handles very fast if it does, right?

That said, I definitly agree with calling it a bug :-)

//Magnus

David Fetter wrote:

On Fri, Nov 04, 2005 at 01:01:20PM -0500, Tom Lane wrote:

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

Nailed it.

problem is in mainloop.c -> setup_cancel_handler. Apparently you
can have multiple handlers and windows keeps track of them all,
even if they do the same thing. Keeping track of so many system
handles would naturally slow the whole process down.

Yipes. So we really want to do that only once.

AFAICS it is appropriate to move the sigsetjmp and
setup_cancel_handler calls in front of the per-line loop inside
MainLoop --- can anyone see a reason not to?

I'm inclined to treat this as an outright bug, not just a minor
performance issue, because it implies that a sufficiently long psql
script would probably crash a Windows machine.

Ouch. In light of this, are we *sure* what we've got a is a candidate
for release?

Good point. It is something we would fix in a minor release, so it
doesn't seem worth doing another RC just for that.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

void
setup_cancel_handler(void)
{
+ static bool done = false;
+
+ if (!done)
SetConsoleCtrlHandler(consoleHandler, TRUE);
+ done = true;
}

That works, I tried ctrl-c various ways including from within \i copy.
Problem solved!

Merlin

AFAICS it is appropriate to move the sigsetjmp and
setup_cancel_handler calls in front of the per-line loop inside
MainLoop --- can anyone see a reason not to?

hm. mainloop is re-entrant, right? That means each \i

would reset the

handler...what is downside to keeping global flag?

Ah, right, and in fact I'd missed the comment at line 325
pointing out that we're relying on the sigsetjmp to be
re-executed every time through. That could be improved on,
likely, but not right before a release.

Does the flag need to be global? I'm thinking

void
setup_cancel_handler(void)
{
+ static bool done = false;
+
+ if (!done)
SetConsoleCtrlHandler(consoleHandler, TRUE);
+ done = true;
}

Seems like a simple enough solution, don't see why it shouldn't work. As
long as psql is single-threaded, which it is...
(Actually, that code seems to re-set done=true on every call which seems
unnecessary - but that might be optimised away, I guess)

//Magnus

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

That works, I tried ctrl-c various ways including from within \i copy.
Problem solved!

Good. I've applied the patch in both HEAD and 8.0 branches.

Since we're very nearly ready to wrap 8.1, would someone with access to
a Windows machine please double-check that CVS tip still works?

regards, tom lane

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-
owner@postgresql.org] On Behalf Of Magnus Hagander
Sent: Friday, November 04, 2005 10:31 AM
To: Tom Lane; Merlin Moncure
Cc: pgsql-hackers@postgresql.org; pgsql-performance@postgresql.org
Subject: Re: [HACKERS] [PERFORM] insert performance for win32

AFAICS it is appropriate to move the sigsetjmp and
setup_cancel_handler calls in front of the per-line loop inside
MainLoop --- can anyone see a reason not to?

hm. mainloop is re-entrant, right? That means each \i

would reset the

handler...what is downside to keeping global flag?

Ah, right, and in fact I'd missed the comment at line 325
pointing out that we're relying on the sigsetjmp to be
re-executed every time through. That could be improved on,
likely, but not right before a release.

Does the flag need to be global? I'm thinking

How about:

   void
   setup_cancel_handler(void)
   {
 +	static bool done = false;
 +
 +	if (!done)
 +    {    
 	  	SetConsoleCtrlHandler(consoleHandler, TRUE);
 +	      done = true;
 +     }
   }

Seems like a simple enough solution, don't see why it shouldn't work.

As

long as psql is single-threaded, which it is...
(Actually, that code seems to re-set done=true on every call which

seems

unnecessary - but that might be optimised away, I guess)

//Magnus

---------------------------(end of

broadcast)---------------------------

Tom Lane wrote:

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

That works, I tried ctrl-c various ways including from within \i copy.
Problem solved!

Good. I've applied the patch in both HEAD and 8.0 branches.

Since we're very nearly ready to wrap 8.1, would someone with access to
a Windows machine please double-check that CVS tip still works?

Worked for me. See buildfarm. Or are there more tests you want run?

cheers

andrew

On Fri, 2005-11-04 at 13:21 -0500, Bruce Momjian wrote:

David Fetter wrote:

On Fri, Nov 04, 2005 at 01:01:20PM -0500, Tom Lane wrote:

I'm inclined to treat this as an outright bug, not just a minor
performance issue, because it implies that a sufficiently long psql
script would probably crash a Windows machine.

Ouch. In light of this, are we *sure* what we've got a is a candidate
for release?

Good point. It is something we would fix in a minor release, so it
doesn't seem worth doing another RC just for that.

Will this be documented in the release notes? If we put unimplemented
features in TODO, where do we list things we regard as bugs?

Best Regards, Simon Riggs

Simon Riggs wrote:

On Fri, 2005-11-04 at 13:21 -0500, Bruce Momjian wrote:

David Fetter wrote:

On Fri, Nov 04, 2005 at 01:01:20PM -0500, Tom Lane wrote:

I'm inclined to treat this as an outright bug, not just a minor
performance issue, because it implies that a sufficiently long psql
script would probably crash a Windows machine.

Ouch. In light of this, are we *sure* what we've got a is a candidate
for release?

Good point. It is something we would fix in a minor release, so it
doesn't seem worth doing another RC just for that.

Will this be documented in the release notes? If we put unimplemented
features in TODO, where do we list things we regard as bugs?

No need, I think. It was patched 2 days ago.

cheers

andrew