"Magnus Hagander" <[EMAIL PROTECTED]> 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
> > > 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?
>
> Y
> "Merlin Moncure" <[EMAIL PROTECTED]> 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.p
On Mon, 24 Oct 2005, Tom Lane wrote:
> Qingqing Zhou <[EMAIL PROTECTED]> 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 my
Qingqing Zhou <[EMAIL PROTECTED]> 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 interlea
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 w
"Merlin Moncure" <[EMAIL PROTECTED]> 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 ap
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
>
Andrew Dunstan <[EMAIL PROTECTED]> 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/alrea
"Merlin Moncure" <[EMAIL PROTECTED]> 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;
Tom Lane wrote:
"Merlin Moncure" <[EMAIL PROTECTED]> 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 CHEC
Tom wrote:
> "Merlin Moncure" <[EMAIL PROTECTED]> 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_F
"Merlin Moncure" <[EMAIL PROTECTED]> 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 somewher
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 r
> > 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?
"Andrew Dunstan" <[EMAIL PROTECTED]> 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 an
Tom Lane wrote:
"Magnus Hagander" <[EMAIL PROTECTED]> 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 cha
> > 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 arg
Qingqing Zhou <[EMAIL PROTECTED]> 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 del
"Magnus Hagander" <[EMAIL PROTECTED]> 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 UN
> > 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_sig
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.
On Sun, 23 Oct 2005, Tom Lane wrote:
> "Magnus Hagander" <[EMAIL PROTECTED]> 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_INTERRUPT
> > 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 WaitForSi
"Magnus Hagander" <[EMAIL PROTECTED]> 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
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 we
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
-
> > 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
"Magnus Hagander" <[EMAIL PROTECTED]> 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
> > 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. Certa
> 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, i
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 thre
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
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.
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 -
> Andrew Dunstan <[EMAIL PROTECTED]> 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 cle
Andrew Dunstan <[EMAIL PROTECTED]> 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
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
con
Andrew Dunstan <[EMAIL PROTECTED]> 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 improveme
Tom Lane wrote:
Andrew Dunstan <[EMAIL PROTECTED]> 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 g
Andrew Dunstan <[EMAIL PROTECTED]> 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.
Wel
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 s
"Magnus Hagander" <[EMAIL PROTECTED]> 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
Tom Lane wrote:
"Magnus Hagander" <[EMAIL PROTECTED]> 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
> > 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
"Magnus Hagander" <[EMAIL PROTECTED]> 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 bei
> >>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
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 -
> 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
> TP
> > 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
On 22/10/05 3:45 am, "Tom Lane" <[EMAIL PROTECTED]> wrote:
> Andrew Dunstan <[EMAIL PROTECTED]> 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
Andrew Dunstan <[EMAIL PROTECTED]> 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 ex
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
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:
I wrote:
BTW, expanding on Andrew's comment, ISTM we could move the kernel call
out of the macro, ie make it look lik
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 commi
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)
>
> 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
--
> 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/win3
"Magnus Hagander" <[EMAIL PROTECTED]> 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 an
>
> 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
---(end of broadcast)-
On Fri, 21 Oct 2005, Merlin Moncure wrote:
> > "Merlin Moncure" <[EMAIL PROTECTED]> 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
> > 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, e
"Merlin Moncure" <[EMAIL PROTECTED]> 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,
> "Merlin Moncure" <[EMAIL PROTECTED]> 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 rec
"Merlin Moncure" <[EMAIL PROTECTED]> 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
> improve
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
> > > 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
> > 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 s
Qingqing Zhou wrote:
"Tom Lane" <[EMAIL PROTECTED]> 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 danger
"Qingqing Zhou" <[EMAIL PROTECTED]> 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
---(end of broadcast)-
> > 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 u
> 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
---(end of broadcast)---
TIP 6: explain analyze is yo
"Tom Lane" <[EMAIL PROTECTED]> 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 PGSemaphoreLoc
"Merlin Moncure" <[EMAIL PROTECTED]> 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
---(end of broadcast)---
TIP 1:
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)
fu01.o(.idata$3+0xc): undefined reference to
"Magnus Hagander" <[EMAIL PROTECTED]> 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-fre
> > 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
Qingqing Zhou <[EMAIL PROTECTED]> 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.p
78 matches
Mail list logo
