headers
Tom Lane | 4 Nov 2005 19:01
Picon

Re: [PERFORM] insert performance for win32

"Merlin Moncure" <merlin.moncure <at> 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

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend
Permalink | Reply | 0 comments |
headers
Merlin Moncure | 4 Nov 2005 19:07

Re: [PERFORM] insert performance for win32

> "Merlin Moncure" <merlin.moncure <at> 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.
(Continue reading)

Permalink | Reply | 0 comments |
headers
David Fetter | 4 Nov 2005 19:14
Gravatar

Re: [PERFORM] insert performance for win32

On Fri, Nov 04, 2005 at 01:01:20PM -0500, Tom Lane wrote:
> "Merlin Moncure" <merlin.moncure <at> 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 <at> fetter.org http://fetter.org/
phone: +1 510 893 6100   mobile: +1 415 235 3778

Remember to vote!
(Continue reading)

Permalink | Reply | 0 comments |
headers
Bruce Momjian | 4 Nov 2005 19:15
Picon

Re: [PERFORM] insert performance for win32

Tom Lane wrote:
> "Merlin Moncure" <merlin.moncure <at> 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 <at> 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

---------------------------(end of broadcast)---------------------------
(Continue reading)

Permalink | Reply | 0 comments |
headers
Tom Lane | 4 Nov 2005 19:14
Picon

Re: [PERFORM] insert performance for win32

"Merlin Moncure" <merlin.moncure <at> 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

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?
(Continue reading)

Permalink | Reply | 0 comments |
headers
Tom Lane | 4 Nov 2005 19:19
Picon

Re: [PERFORM] insert performance for win32

David Fetter <david <at> 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

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

               http://www.postgresql.org/docs/faq
Permalink | Reply | 0 comments |
headers
Bruce Momjian | 4 Nov 2005 19:21
Picon

Re: [PERFORM] insert performance for win32

David Fetter wrote:
> On Fri, Nov 04, 2005 at 01:01:20PM -0500, Tom Lane wrote:
> > "Merlin Moncure" <merlin.moncure <at> 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 <at> candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
(Continue reading)

Permalink | Reply | 0 comments |
headers
Magnus Hagander | 4 Nov 2005 19:21

Re: [PERFORM] insert performance for win32

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

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

               http://archives.postgresql.org
Permalink | Reply | 0 comments |
headers
Magnus Hagander | 4 Nov 2005 19:30

Re: [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
> 
>   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)
(Continue reading)

Permalink | Reply | 0 comments |
headers
Dann Corbit | 4 Nov 2005 19:41

Re: [PERFORM] insert performance for win32

> -----Original Message-----
> From: pgsql-hackers-owner <at> postgresql.org [mailto:pgsql-hackers-
> owner <at> postgresql.org] On Behalf Of Magnus Hagander
> Sent: Friday, November 04, 2005 10:31 AM
> To: Tom Lane; Merlin Moncure
> Cc: pgsql-hackers <at> postgresql.org; pgsql-performance <at> 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;
 +
(Continue reading)

Permalink | Reply | 0 comments |

Gmane