Re: [HACKERS] 8.04 and RedHat/CentOS init script issue and sleep
I'm not actually particularly worried about the startup time. What's bothering me right at the moment, given the new-found knowledge that strftime() is slow on Linux, is that we're using it in elog(). At the time that code was written, we did it deliberately to ensure that all the backends would write log timestamps in the same timezone regardless of local SET TimeZone commands. That's still an important consideration, but I wonder whether we don't now have enough timezone infrastructure that we could get the same results using pg_strftime. If glibc fixes the problem upstream then we can leave well enough alone, but if they indicate they won't then we should That'll take quite a while to trickle down into the distributions even if it's fixed, won't it? If the fix is simple, we should perhaps consider it anyway. think about doing this someday. The major problem with it probably is what do you do when messages need to be emitted before pgtz has been initialized? Shouldn't be too hard, I think. If we declare a pg_tz* system_timezone or so, and initialize it to NULL. Once pgtz is initialized we assign a valid timezone to it, being the startup timezone. Then in elog, we simply check if system_timezone is null and then fallback on the glibc version of strftime. It shouldn't be a performance issue if it fails that often, because we won't call elog a whole lot of times there, right? //Magnus ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [GENERAL] [HACKERS] 'a' == 'a '
Dann Corbit wrote:
-Original Message-
From: Tom Lane [mailto:[EMAIL PROTECTED]
Sent: Thursday, October 20, 2005 2:54 PM
To: Dann Corbit
Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; pgsql-
[EMAIL PROTECTED] Subject: Re: [GENERAL] [HACKERS] 'a' == 'a '
Dann Corbit [EMAIL PROTECTED] writes:
I guess that additional ambiguity arises if you add additional
spaces to the end. Many database systems solve this by trimming
the characters from the end of the string upon storage and the
returned
string will
not
have any trailing blanks.
Can you document that? ISTM that that would effectively make char(n)
and varchar(n) exactly equivalent, which is ... um ... a bit stupid.
This is SQL*Server:
drop table test_char
go
create table test_char(
fixed_30 char(30),
varch_30 varchar(30),
nchar_30 nchar(30),
nvarc_30 nvarchar(30)
)
go
insert into test_char values('Dann ', 'Dann ', 'Dann ', 'Dann ') go
select len(fixed_30), len(varch_30), len(nchar_30), len(nvarc_30)
from test_char go
Result set:
4 4 4 4
For SQL Server, you are seeing the behaviour of the len function,
none of the data has been trimmed.
Using the same tables:
select len('x' + (fixed_30) + 'x'),
len('x' + varch_30 + 'x'),
len('x' + nchar_30 + 'x'),
len('x' + nvarc_30 + 'x')
from test_char
Result set :
32 7 32 7
Cheers, Kurt.
--
This electronic message together with any attachments is confidential and
intended for the named recipient's use only. If you are not the intended
recipient (i) do not copy, disclose or use the contents in any way, (ii)
please let us know by return email immediately then destroy the message, and
any hard copies of the message, and any attachments. The sender of this
message is not responsible for any changes made to this message and/or any
attachments and/or connection linkages to the Internet referred to in this
message after it has been sent. Unless otherwise stated, any pricing
information given in this message and/or attachments is indicative only, is
subject to change and does not constitute an offer to buy or sell securities
or derivatives at any price quoted. Any reference to the terms of executed
transactions should be treated as preliminary only and subject to separate
formal written notification. Where reference is made to research material
and/or research recommendations, the basis of the provision of such research
material and/or recommendations is set out in the relevant disclaimer.
---(end of broadcast)---
TIP 4: Have you searched our list archives?
http://archives.postgresql.org
Re: [HACKERS] Question about Ctrl-C and less
On Thu, Oct 20, 2005 at 08:11:14PM -0400, [EMAIL PROTECTED] wrote: I disagree that psql should make *any* assumptions about what SIGINT means to the child process. Consider less again, and Control-C used to abort a search. You are suggesting that Control-C should not only abort the search, but should also cut off the input from less. Less won't die. Less will just see a terminated input stream. What has been gained from this? Is this intuitive behaviour? I must say I agree with the idea that Ctrl-C shouldn't stop the stream from psql, but I'm willing to let it slide because a lot of other programs work this way. I imagine asking it to be configurable will meet even more resistance. I think the only reasonable behaviour is to ignore SIGINT within the parent, until the child exits. I don't see why other behaviours are even being considered. To me, it points at a misunderstanding of the problem. I've been playing with a version of psql which on Ctrl-C doesn't longjmp() but politely frees everything, waits for the pager and then back to the main loop with the message Interrupted. But now we have another behaviour change: How to abort the gets() when you don't have readline? Doing it with a flag is a lot more susceptable to subtle behaviour changes, but I'll see if I can make it work. Have a nice day, -- Martijn van Oosterhout kleptog@svana.org http://svana.org/kleptog/ Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a tool for doing 5% of the work and then sitting around waiting for someone else to do the other 95% so you can sue them. pgpaRH7VBVICN.pgp Description: PGP signature
Re: [HACKERS] Seeing context switch storm with 10/13 snapshot of
On Thu, 2005-10-20 at 16:54 -0600, Robert Creager wrote: On Thu, 20 Oct 2005 23:28:21 +0100 Simon Riggs [EMAIL PROTECTED] wrote: If the CS is the same, then it will tell us that the issue is not data dependent. If the CS drops, it tells us that it is an activity performed on the precise data blocks rather than the shared data structures which is the issue. That would then account for why the effect appears to come and go in your own application, because the effect is actually dependant on the data distribution (which presumably varies in your tables). The CS is the same on both 7.4.1 and 8.1b3 (with 8.1b3 showing 100k and 7.4.1 showing 170k using three clients). I double checked the scripts to make sure... So that means it is either a generic issue or likely to be a lwlock issue on the main named locks. The drop in CS between releases is consistent with the overall drop in contention as a result of the redesign of the buffer manager. Does definitely look like spinlock issues on that platform. It would be good to test this on a much newer system. Tuning for older hardware doesn't seem that worthwhile (no shouting, please!). It would be good right now to have a multi-process test harness that would allow us to test out different spin lock code without the rest of PostgreSQL getting in the way of testing. If we can isolate the issue outside of PostgreSQL it will be much easier to fix. Any hackers out there looking to set up a spinlock test harness? Best Regards, Simon Riggs ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [GENERAL] [HACKERS] 'a' == 'a '
Tom Lane [EMAIL PROTECTED] wrote on 10/20/2005 03:11:23 PM: snip The hard part would be in figuring out how the output routine could know how many spaces to add back. The length is in the metadata for the column, or am I being dense? regards, tom lane ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [GENERAL] [HACKERS] 'a' == 'a '
At 05:33 PM 10/19/2005 -0700, Dann Corbit wrote: If there is a significant performance benefit to not expanding text columns in comparison operations, then it seems it should be OK. I probably read the standard wrong, but it seems to me that varchar, char, and bpchar columns should all behave the same (e.g. if you do not expand with blank or the PAD character (whatever that is) then all char type columns should behave the same. I guess that there could be different default collations for different column I am not a DB guru. BUT IMO they should NOT behave the same. Varchars should NOT be padded. For the very reason when you select text out of varchar fields the result is not padded. If I insert a string with a single trailing space into a varchar, I _want_ that single trailing space to still be there when I retrieve it, and not followed by more spaces. Otherwise I will have to pick a different database ;). So similarly, I would expect that varchars 'a ' and 'a' when compared should be different. However, in the case of _chars_ which are padded, then 'a ' should be padded so that it can be compared with 'a '. Otherwise there will be no reason to do equality comparisons of char(5) fields with char(8) fields - they can NEVER be the same :). But would that mean that when one does equality comparisons of varchars with chars, one would probably want padding? Or only varchars of the same length as the char would have a chance of matching? Hmm.. I think I better leave this one to the DB gurus :). But I really don't ever want 'a ' to be the same as 'a ' for varchars. Link. ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] 'a' == 'a ' (Was: RE: [pgsql-advocacy] [GENERAL] Oracle buysInnobase)
Richard Huxton wrote: Dann Corbit wrote: I can see plenty of harm and absolutely no return. We are talking about blank padding before comparison. Do you really want 'Danniel ' considered distinct from 'Danniel ' in a comparison? In real life, what does that buy you? 100% YES! If two values are the same, then any function on those two values should return the same result. Otherwise what does equals mean? At the very least length() is broken by your argument. I agree completely. I would much rather be precise than intuitive. And, I have done applications where the padding length was important, especially when working with remote batch processing in the credit industries and the like. Writing queries to create and process fixed-width batch files is much easier if you can rely on these kinds of behaviors. Here it's CHAR that's broken IMHO - spawn of some punch-card spawned data processing rules of the 70s. -- Richard Huxton Archonet Ltd ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
RES: [HACKERS] Key violation. ERROR: type lo does not exist.
Title: RES: [HACKERS] Key violation. ERROR: type lo does not exist. I do not know. How I make this? How I verify if this installed? At. Fernando C. Figueiredo CRC Vivo - Goiânia Fone: (62) 3238-3441 / 9978-9857 -Mensagem original- De: Douglas McNaught [mailto:[EMAIL PROTECTED]] Enviada em: quinta-feira, 20 de outubro de 2005 09:04 Para: [EMAIL PROTECTED] Cc: pgsql-hackers@postgresql.org Assunto: Re: [HACKERS] Key violation. ERROR: type lo does not exist. [EMAIL PROTECTED] writes: I receive the mesagem from error: Key violation. ERROR: type lo does not exist. Have you installed the 'contrib/lo' module that creates that type? -Doug
Re: [HACKERS] Question about Ctrl-C and less
On Fri, Oct 21, 2005 at 01:53:32PM +0200, Martijn van Oosterhout wrote: On Thu, Oct 20, 2005 at 08:11:14PM -0400, [EMAIL PROTECTED] wrote: I disagree that psql should make *any* assumptions about what SIGINT means to the child process. Consider less again, and Control-C used to abort a search. You are suggesting that Control-C should not only abort the search, but should also cut off the input from less. Less won't die. Less will just see a terminated input stream. What has been gained from this? Is this intuitive behaviour? I must say I agree with the idea that Ctrl-C shouldn't stop the stream from psql, but I'm willing to let it slide because a lot of other programs work this way. I imagine asking it to be configurable will meet even more resistance. Which other ones? I can't think of one. The ones that don't handle SIGINT, or that are not designed for this scenario certainly don't count - as that is how psql works right now without the patch. Of the remaining programs that are designed to work with an application such as less, which ones purposefully close the input stream to less, and continue running? I think 0. I think the only reasonable behaviour is to ignore SIGINT within the parent, until the child exits. I don't see why other behaviours are even being considered. To me, it points at a misunderstanding of the problem. I've been playing with a version of psql which on Ctrl-C doesn't longjmp() but politely frees everything, waits for the pager and then back to the main loop with the message Interrupted. But now we have another behaviour change: How to abort the gets() when you don't have readline? Doing it with a flag is a lot more susceptable to subtle behaviour changes, but I'll see if I can make it work. SIG_IGN is the easiest, and the most effective. Certainly the easiest to maintain, and the least intrusive to the core. I don't understand. Cheers, mark -- [EMAIL PROTECTED] / [EMAIL PROTECTED] / [EMAIL PROTECTED] __ . . _ ._ . . .__. . ._. .__ . . . .__ | Neighbourhood Coder |\/| |_| |_| |/|_ |\/| | |_ | |/ |_ | | | | | | \ | \ |__ . | | .|. |__ |__ | \ |__ | Ottawa, Ontario, Canada One ring to rule them all, one ring to find them, one ring to bring them all and in the darkness bind them... http://mark.mielke.cc/ ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [GENERAL] [HACKERS] 'a' == 'a '
I will happily reiterate that I am the troll who started this mess by whining about how *Oracle* handles this. Tom's explanation that CHAR is has a PAD collation and VARCHAR has a NO PAD collation have restored my faith that there is goodness in the world. My whining was out of ignorance. I wouldn't change the proper way PostgreSQL works. Documenting it is good. I will use this new found knowledge from now on in my database designs. Cheers, Rick Chris Travers [EMAIL PROTECTED] wrote on 10/20/2005 01:52:36 PM: Dann Corbit wrote: Let me make something clear: When we are talking about padding here it is only in the context of a comparison operator and NOT having anything to do with storage. IIrc, varchar and bpchar are stored in a similar way, but are presented differently when retrieved. I.e. storage is separate from presentation in this case. I.e. the padding in bpchar occurs when it is presented and stripped when it is stored. Again, I am happy solving this simply by documenting it since any questions of interpretation and implimentation of the standard would be answered. So far what I (and I am sure others) have not heard is a strong case for changing the behavior, given that it is in line with a reasonable interpretation of the standards. Given two strings of different in a comparison, most database systems (by default) will blank pad the shorter string so that they are the same length before performing the comparison. Understood, but what gain do you have in a case like this that might justify the effort that would go into making it, say, an initdb option? How often does this behavior cause problems? Best Wishes, Chris Travers Metatron Technology Consulting ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [GENERAL] [HACKERS] 'a' == 'a '
[Removed all the non-list addresses] Dann Corbit wrote: Let me make something clear: When we are talking about padding here it is only in the context of a comparison operator and NOT having anything to do with storage. Given two strings of different in a comparison, most database systems (by default) will blank pad the shorter string so that they are the same length before performing the comparison. Hence, you will see that 'Danniel' = 'Danniel ' is true in most cases. Now, this really does not have any connection with storage or varchar or bpchar or char or text or anything like that. Is this really true??? My understanding of the spec was that this was =exactly= the difference between char(N) and varchar(N) - the former is padded to length N when you store it, or at least the DB has to act as if this is the case. Can someone quote the appropriate chapter and verse? Thanks. - John D. Burger MITRE ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [GENERAL] [HACKERS] 'a' == 'a '
Dann Corbit [EMAIL PROTECTED] wrote on 10/20/2005 04:24:26 PM: -Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] Sent: Thursday, October 20, 2005 2:12 PM To: Tom Lane Cc: Chris Travers; Dann Corbit; Greg Stark; josh@agliodbs.com; pgsql- [EMAIL PROTECTED]; pgsql-hackers@postgresql.org; Marc G. Fournier; Stephan Szabo; Terry Fielder; Tino Wildenhain Subject: Re: [GENERAL] [HACKERS] 'a' == 'a ' Tom Lane [EMAIL PROTECTED] wrote on 10/20/2005 03:11:23 PM: snip The hard part would be in figuring out how the output routine could know how many spaces to add back. The length is in the metadata for the column, or am I being dense? I guess that what Tom is saying is that it would be nice to store everything as VARCHAR. snip I get that part, but he asked how to determine the proper output length based on the PAD semantics, and I was saying to just pad whatever is stored to the length available in the column metadata for those collations that require padding. I guess that additional ambiguity arises if you add additional spaces to the end. Many database systems solve this by trimming the characters from the end of the string upon storage and the returned string will not have any trailing blanks. I am not arguing pro nor con this way of doing things. For PAD correlations, Oracle stores the blanks. Interbase does not. Going from Interbase to Oracle I switched from CHAR to VARCHAR2 because of this, shot my self in the foot by not understanding PAD, whined about it, and you know the rest. As to how it is output, I know the language interface has an effect. Embedded SQL in C will put the null terminator in different places for different databases and different switch settings particular to each database. ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [GENERAL] [HACKERS] 'a' == 'a '
I wonder how widespread the MicroSoft behavior is Sybase ASE,
for example, gives this result set:
30 5 30 5
That seems more appropriate to me.
-Kevin
Dann Corbit [EMAIL PROTECTED] writes:
I guess that additional ambiguity arises if you add additional
spaces to
the end. Many database systems solve this by trimming the
characters
from the end of the string upon storage and the returned string will
not
have any trailing blanks.
Can you document that? ISTM that that would effectively make char(n)
and varchar(n) exactly equivalent, which is ... um ... a bit stupid.
This is SQL*Server:
drop table test_char
go
create table test_char(
fixed_30 char(30),
varch_30 varchar(30),
nchar_30 nchar(30),
nvarc_30 nvarchar(30)
)
go
insert into test_char values('Dann ', 'Dann ', 'Dann ', 'Dann ')
go
select len(fixed_30), len(varch_30), len(nchar_30), len(nvarc_30)
from test_char
go
Result set:
4 4 4 4
---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster
Re: [HACKERS] Question about Ctrl-C and less
On Fri, Oct 21, 2005 at 08:48:31AM -0400, [EMAIL PROTECTED] wrote: Which other ones? I can't think of one. The ones that don't handle SIGINT, or that are not designed for this scenario certainly don't count - as that is how psql works right now without the patch. Of the remaining programs that are designed to work with an application such as less, which ones purposefully close the input stream to less, and continue running? The one run into all the time is: rgrep something . | less If you do a search and it doesn't find anything, it'll keep reading in data until it finds it or reaches the end. Hit Ctrl-C to abort the search and you kill the rgrep too. Now, arguably rgrep is not designed to work with an application such as less so doesn't meet your criteria. So the argument is about whether psql is. Is the pager an integral part of the output mechanism of psql, or just a bolt-on like a shell pipe? I suspect you can argue either way, which is why I'm trying to find a way to have it both ways. SIG_IGN is the easiest, and the most effective. Certainly the easiest to maintain, and the least intrusive to the core. Yeah, but doesn't fix the memory leaks and other random leakage and breakage from the longjmp(). But I'm getting there... Have a nice day, -- Martijn van Oosterhout kleptog@svana.org http://svana.org/kleptog/ Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a tool for doing 5% of the work and then sitting around waiting for someone else to do the other 95% so you can sue them. pgpe84NmsO9L6.pgp Description: PGP signature
Re: [HACKERS] collector/autovacuum after crash (8.1beta3)
Szima Gábor wrote: At system crash or poweroff the autovacuum statistics will be lost, because this statistics only stored in RAM and saved/restored at service shutdown/startup. I think it should be saved periodically and not to be deleted after crash. I don't remember why it was that we made the stat file be dropped on crash recovery, but there was a reason. I guess you could research the reason; it was a thread which ended with an autovacuum patch from me. You could look the CVS logs for the pgstat.c file. -- Alvaro Herrerahttp://www.advogato.org/person/alvherre Someone said that it is at least an order of magnitude more work to do production software than a prototype. I think he is wrong by at least an order of magnitude. (Brian Kernighan) ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [HACKERS] Seeing context switch storm with 10/13 snapshot of
Simon Riggs [EMAIL PROTECTED] writes: It would be good right now to have a multi-process test harness that would allow us to test out different spin lock code without the rest of PostgreSQL getting in the way of testing. If we can isolate the issue outside of PostgreSQL it will be much easier to fix. Actually I disagree with that, on three grounds: 1. Optimizing for an artificial environment may result in the wrong optimization. 2. I don't think you'll be able to prove anything except that all SMP designs ultimately suck. There is no hardware on the planet that can trade cache lines back and forth at instruction-dispatch rates. 3. The problem right now is not lack of ability to reproduce the problem, it is lack of ideas how to fix it. Building an artificial testbed is just directing effort into make-work rather than towards solving the problem. regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] [COMMITTERS] pgsql: Clean up some obsolete statements about GiST
Christopher Kings-Lynne [EMAIL PROTECTED] writes: Tom, I also notice that the link I put in ages ago to Kornacker's thesis is now defunct :( A quick search of Google Scholar finds it hosted on Oleg Teodor's site. Fixed, thanks. regards, tom lane ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] collector/autovacuum after crash (8.1beta3)
Alvaro Herrera [EMAIL PROTECTED] writes: Szima Gábor wrote: At system crash or poweroff the autovacuum statistics will be lost, I don't remember why it was that we made the stat file be dropped on crash recovery, but there was a reason. The fact that it's not WAL-backed and hence couldn't be trusted. But I think he's complaining about the contrib version of autovacuum anyway. regards, tom lane ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [HACKERS] Seeing context switch storm with 10/13 snapshot
Remember the suggestion I made that PostgreSQL add the capability to define named caches and bind specific objects to those caches? One of the reasons Sybase recommends using such named caches (per their performance tuning documentation) is to reduce spinlock contention. I don't know whether PostgreSQL would get a similar benefit, but this discussion rang a bell, so I thought I should throw it out there on the chance that it might be useful. -Kevin Tom Lane [EMAIL PROTECTED] The problem right now is not lack of ability to reproduce the problem, it is lack of ideas how to fix it. ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
[HACKERS] Which platforms don't HAVE_POSIX_SIGNALS?
Other than Windows I guess. Sometimes the system call restart behaviour is annoying, but you can work around that using sigaction. But can that be relied apon? Thanks in advance, -- Martijn van Oosterhout kleptog@svana.org http://svana.org/kleptog/ Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a tool for doing 5% of the work and then sitting around waiting for someone else to do the other 95% so you can sue them. pgpJiwfPG85fB.pgp Description: PGP signature
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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.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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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 ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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-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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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 `libpostgres_a_iname' fu03.o(.idata$3+0xc): undefined reference to `libpostgres_a_iname' nmth00.o(.idata$4+0x0): undefined reference to `_nm__pg_signal_mask' nmth02.o(.idata$4+0x0): undefined reference to `_nm__pg_signal_queue' ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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 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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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 your friend
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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 ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance
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 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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 ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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 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 ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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, 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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 ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance
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 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 ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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 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 ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] Seeing context switch storm with 10/13 snapshot of
On Fri, 2005-10-21 at 09:52 -0400, Tom Lane wrote: Simon Riggs [EMAIL PROTECTED] writes: It would be good right now to have a multi-process test harness that would allow us to test out different spin lock code without the rest of PostgreSQL getting in the way of testing. If we can isolate the issue outside of PostgreSQL it will be much easier to fix. Actually I disagree with that, on three grounds: 1. Optimizing for an artificial environment may result in the wrong optimization. 2. I don't think you'll be able to prove anything except that all SMP designs ultimately suck. There is no hardware on the planet that can trade cache lines back and forth at instruction-dispatch rates. 3. The problem right now is not lack of ability to reproduce the problem, it is lack of ideas how to fix it. Building an artificial testbed is just directing effort into make-work rather than towards solving the problem. If we think spinlocks are the problem, building a spinlock test harness will prove that and also simplify the testing of a solution. Isolating the spinlocks in that way is not artificial, but actually a very pure test, so although I agree with (1) as a general statement, this does not apply for the test harness proposal. I was seeing the problem as likely to rear its head again over time. We are entering a stage of increased importance of SMP code, since within a few short years all CPUs will be dual core/HT or something similar. We may fix it for one platform, but other similar problems may re/emerge. The easiest way to test spinlock code on any platform is to get an isolated test case that is runnable outside of the context of PostgreSQL, yet using the pg spinlock code. That would allow us to bring in other people and their many eyeballs to look at the issue; we on this list are not experts at everything. Certainly there is a lack of ideas as to how to fix it, as you mention in (3). This shows to me that the solution lies in one of two areas: a) the solution has not yet been considered or b) the solution has already been thought of and for whatever reason disregarded. You may be certain that the solution lies in a), though I am not. Rejecting ideas quickly may simply increase the chances of finding the solution in a b) case. Forgive me but (2) seems spurious. Nobody said anything about trading cache lines at instruction-dispatch rates. The objective of the test harness would be to check whether negative effects such as CS storms exist on that platform. Actual optimization of the spinlock mechanisms in the context of the PostgreSQL server should certainly be done within the code. In a more general sense: what is the best next action to make progress on the CS issue that exists for certain CPUs? Best Regards, Simon Riggs ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance
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 - 1.12
+++ backend/port/win32/signal.c 21 Oct 2005 05:32:52 -
@@ -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.h15 Oct 2005 02:49:45 - 1.47
+++ include/port/win32.h21 Oct 2005 05:33:26 -
@@ -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 - 1.180
+++ include/miscadmin.h 21 Oct 2005 05:33:56 -
@@ -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)
---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance
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 ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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
---(end of broadcast)---
TIP 4: Have you searched our list archives?
http://archives.postgresql.org
Re: [HACKERS] Seeing context switch storm with 10/13 snapshot of
Simon Riggs [EMAIL PROTECTED] writes: Certainly there is a lack of ideas as to how to fix it, as you mention in (3). This shows to me that the solution lies in one of two areas: a) the solution has not yet been considered or b) the solution has already been thought of and for whatever reason disregarded. You may be certain that the solution lies in a), though I am not. Rejecting ideas quickly may simply increase the chances of finding the solution in a b) case. However, building a spinlock test harness presupposes that the solution lies in the spinlock code itself, and not in (say) changing our usage patterns of it. So I'd throw the rejecting ideas too quickly challenge right back at you. What we need to optimize is the behavior in the real context of Postgres, not in a test harness. regards, tom lane ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [HACKERS] [COMMITTERS] pgsql: Do all accesses to shared buffer
On Mon, 2005-17-10 at 16:48 -0500, Jim C. Nasby wrote: Sorry if I'm just confused here, but don't LWLocks protect data structures susceptible to corruption? And if that's the case don't we need to be sure that the compiler can't optimize around them? LWLocks certainly do protect shared data, and if the compiler rearranged loads and stores around LWLocks acquire/release, it would result in corruption. Tom was arguing it is unlikely the compiler will actually do this (because LWLockAcquire is an out-of-line function call that might invoke a system call, unlike SpinLockAcquire). -Neil ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance tweak
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.origFri 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 intpg_signal_queue;
static pqsigfunc pg_signal_array[PG_SIGNAL_COUNT];
static pqsigfunc pg_signal_defaults[PG_SIGNAL_COUNT];
- static intpg_signal_mask;
-
- DLLIMPORT HANDLE pgwin32_signal_event;
- HANDLEpgwin32_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;
!
! HANDLEpgwin32_signal_event;
! HANDLEpgwin32_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.origFri 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.hFri Oct 21 17:36:12 2005
***
*** 214,224
Re: [HACKERS] [COMMITTERS] pgsql: Do all accesses to shared buffer
Neil Conway [EMAIL PROTECTED] writes: LWLocks certainly do protect shared data, and if the compiler rearranged loads and stores around LWLocks acquire/release, it would result in corruption. Tom was arguing it is unlikely the compiler will actually do this (because LWLockAcquire is an out-of-line function call that might invoke a system call, unlike SpinLockAcquire). Actually it seems the sore spot is more likely to be SpinLockRelease, which on many architectures expands to nothing more than an inlined *(lockptr) = 0; The lock pointer is declared as pointer to volatile, which should discourage the compiler from removing the store altogether, but as we've seen the compiler may be willing to rearrange the store with respect to adjacent loads/stores that aren't marked volatile. SpinLockAcquire contains an out-of-line function call, so although the compiler could theoretically misoptimize things in the fast path, it's probably much less risky than SpinLockRelease. BTW, we may be perfectly safe on architectures like PPC, where S_UNLOCK includes an __asm__ __volatile__ section for a hardware-level optimization fence instruction. I wonder though if it'd be a good idea to be marking those fence instructions with the clobbers memory qualifier to ensure this? regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance
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 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.origFri 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 intpg_signal_queue;
static pqsigfunc pg_signal_array[PG_SIGNAL_COUNT];
static pqsigfunc pg_signal_defaults[PG_SIGNAL_COUNT];
- static intpg_signal_mask;
-
- DLLIMPORT HANDLE pgwin32_signal_event;
- HANDLEpgwin32_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;
! HANDLEpgwin32_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.origFri 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.hFri Oct 21 17:36:12 2005
Re: [HACKERS] Question about Ctrl-C and less
[EMAIL PROTECTED] wrote: On Thu, Oct 20, 2005 at 03:42:10PM -0700, Kevin Brown wrote: Martijn van Oosterhout wrote: You can't do a pclose in a signal handler, it's not one of the reeentrant safe functions and could lead to deadlocks. The signal manpage documents the ones you can use. Just set a flag. Setting the descriptor to NULL is worse because then we have check before every output function. fprintf(NULL, ...) will segfault on most architechtures I wager. Yeah, I was thinking that you'd do the check for the flag and invoke a cleanup handler after the write() to the output file descriptor. It's not clear that you'd need to do the check anyplace else. It's been a while since I've messed with this stuff, but if I recall correctly, the write() will return immediately after receipt of a signal, and will indicate how much was actually written. So receipt of a SIGINT should wind up being handled in a reasonably timely fashion. Additionally the normal SIGINT signal handler (the one that gets invoked when the pager is turned off) can be called from the cleanup handler in order to maintain the proper semantics. I disagree that psql should make *any* assumptions about what SIGINT means to the child process. Consider less again, and Control-C used to abort a search. You are suggesting that Control-C should not only abort the search, but should also cut off the input from less. Less won't die. Less will just see a terminated input stream. What has been gained from this? Is this intuitive behaviour? It's behaviour that's consistent with every other pipeline application set I've ever seen. The only difference between those and this situation is that for a standard pipeline set, SIGINT will kill all the processes in the pipeline. Less explicitly ignores (or traps) SIGINT, so the effect of generating a SIGINT where less is at the end of the pipeline is to kill everything that precedes it, and less will then show what results it received. And obviously, that implies that the pipeline gets closed. If psql does not close the pipeline right then and there, then its behaviour will obviously be different from what the user likely expects, based on other pipelined uses of less. After all, if they wanted to see the entire result set then they wouldn't have sent SIGINT, would they? If the pager does die in response to SIGINT, the write() will fail with SIGPIPE. Completely clean, without any need for psql to pay attention to SIGINT. We're not talking about the semantics of the pager, we're talking about the semantics of psql. You said it yourself: psql can't make any assumptions about what SIGINT means to the child process. So it has to consider what to do if the child does *not* die in response to SIGINT. What are the proper semantics for psql + the child in that situation? Well, I think it's clear that having psql ignore SIGINT in that situation is *not* correct, because it implies that whether or not SIGINT causes processing to stop (as the user would expect) depends entirely on the child. More specifically, it would depend on the child either dying or explicitly closing its stdin upon receipt of SIGINT. The bottom line is that, at least in my opinion, the semantics of SIGINT as regards psql should be the same whether or not there's a pager involved, with one crucial difference: if there's a pager involved, psql should wait for it to terminate before showing the prompt. But otherwise, the semantics should be identical, because they are what the user expects. I think the only reasonable behaviour is to ignore SIGINT within the parent, until the child exits. I don't see why other behaviours are even being considered. To me, it points at a misunderstanding of the problem. Not at all. When you send SIGINT to a process, you want that process to stop doing whatever it's doing and return control to you. That's what it means, and that's what it's for. If we ignore SIGINT then obviously we will *not* be heeding the wishes of the user who sends SIGINT, and that is not likely what the user expects. -- Kevin Brown [EMAIL PROTECTED] ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [HACKERS] Question about Ctrl-C and less
If you send a recent version of vim a CONTROL-C, and you're just sitting there at a prompt, it gives you a hint: Type :quitEnter to exit Vim Any reason not to just trap the CONTROL-C in psql when paging and offer a hint? Especially since we don't really know that the user really wanted to type CONTROL-C instead of q for quit. I know that I have always meant to type q and was just distracted whenever I've typed CONTROL-C in the pager, and so passing the CONTROL-C on to less is not actually heeding my wishes, it is instead giving me enough rope to shoot myself in the foot. Sean (and mixed metaphors really make by hair boil) [big snippage] Not at all. When you send SIGINT to a process, you want that process to stop doing whatever it's doing and return control to you. That's what it means, and that's what it's for. If we ignore SIGINT then obviously we will *not* be heeding the wishes of the user who sends SIGINT, and that is not likely what the user expects. -- Kevin Brown [EMAIL PROTECTED] ---(end of broadcast)--- TIP 6: explain analyze is your friend ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] Question about Ctrl-C and less
Andrew - Supernews wrote: On 2005-10-19, Kevin Brown [EMAIL PROTECTED] wrote: Making assumptions about what the pager will do upon receipt of SIGINT is folly as well. Setting up SIGINT to be ignored may be the right answer (I don't believe it is -- see below), but if so then it needs to be done properly. If it gets ignored prior to the popen(), then the child will also end up ignoring it by default, because signal disposition is inherited by child processes. If we ignore SIGINT, it should be after the popen(), not before. I do not believe it is possible to do the signal disposition correctly and still use popen() to run the pager. (You would need to reimplement popen using raw syscalls.) I'm not sure I see why this is so. popen() just creates the pipeline, fork()s, closes the proper file descriptor (depending on whether it's in the parent or the child and whether the pipe was open for read or write) and then exec()s in the child. In the parent, it returns control after the fork() and file descriptor cleanup. So the parent can set up its own internal signal disposition immediately after popen() returns. This sequence of events is exactly what we'd end up doing if we did everything ourselves using raw syscalls, save for the use of stdio instead of direct syscalls for the file operations. So I think the right answer here is for psql to handle SIGINT internally by doing a pclose() first The chances that psql can do this safely approach zero. pclose() is not a signal-safe function, so it can only be called from a signal handler if you _know_ that the signal did not interrupt any non-signal-safe function. (Nor can the signal handler longjmp out in such a case, unless the code is never again going to call any unsafe function.) I agree. I guess I need to be a little more explicit about what I envision here. There would be two possible signal handlers. The first is the one we have now, which cleans up various things upon receipt of SIGINT. The second simply sets a flag that says that SIGINT has been received. The signal handler that gets assigned to SIGINT depends on whether or not a pager is going to be used. If it's not, then we point it to the first signal handler. If it is, then we point it to the second, and clear the flag. When a pager is being used, we check for the flag immediately after doing a write()/fwrite() to the pipe. If it's set, we pclose(), clear the flag, and then manually invoke the non-pager signal handler. SIGINT should cause the write() to return immediately, possibly with EINTR. Make sense? -- Kevin Brown [EMAIL PROTECTED] ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [HACKERS] Question about Ctrl-C and less
I failed to mention that I also tend to type CONTROL-C when I forget that putty acts like an xterm, and doesn't need CONTROL-C to copy text into the clipboard. In that case, aborting the pager, and leaving the terminal trashed requiring me to exit psql, stty sane, and start up psql again is really not heeding my wishes, since what I was trying to do was copy some text from the screen in a moment of brain-deadness. Sean ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [HACKERS] [PATCHES] Win32 CHECK_FOR_INTERRUPTS() performance
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:
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 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.origFri Oct 14 22:59:56 2005
--- src/backend/port/win32/signal.cFri 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 intpg_signal_queue;
static pqsigfunc pg_signal_array[PG_SIGNAL_COUNT];
static pqsigfunc pg_signal_defaults[PG_SIGNAL_COUNT];
- static intpg_signal_mask;
- - DLLIMPORT HANDLE pgwin32_signal_event;
- HANDLEpgwin32_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;
! intpg_signal_mask;
! ! HANDLEpgwin32_signal_event;
! HANDLEpgwin32_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)
{
inti;
EnterCriticalSection(pg_signal_crit_sec);
! while (pg_signal_queue ~pg_signal_mask)
{
/* One or more unblocked signals queued for execution */
! ! intexec_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)
{
inti;
EnterCriticalSection(pg_signal_crit_sec);
! while (UNBLOCKED_SIGNAL_QUEUE())
{
/* One or more unblocked signals queued for execution */
! intexec_mask = UNBLOCKED_SIGNAL_QUEUE();
for (i = 0; i PG_SIGNAL_COUNT; i++)
{
*** src/include/miscadmin.h.origFri Oct 14 23:00:22 2005
--- src/include/miscadmin.hFri 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(); \
Re: [HACKERS] RSS feeds of CVS revision logs
Hi Joshua, I think the RSS feed is a great Idea! (I personally use InfoRss with Mozilla) Though it would be nice to reduce the amount of text before the description (I barely get by the revision number to see the description) maybe formatted CMP:R##: Description... Though I have no idea how much work that would be. Oh, and by the way, thanks to all for one really nice database! Nathan
