Re: [HACKERS] insert performance for win32

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. Commenting that line times are
flat as a pancake. I am thinking keeping track of a global flag would
be appropriate.

Merlin

"Merlin Moncure" <merlin(dot)moncure(at)rcsonline(dot)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

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

Remember to vote!

Tom Lane wrote:
> "Merlin Moncure" <merlin(dot)moncure(at)rcsonline(dot)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(dot)pha(dot)pa(dot)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(at)fetter(dot)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

David Fetter wrote:
> On Fri, Nov 04, 2005 at 01:01:20PM -0500, Tom Lane wrote:
> > "Merlin Moncure" <merlin(dot)moncure(at)rcsonline(dot)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(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

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