Обсуждение: Re: TRAP: FailedAssertion("!((itemid)->lp_flags & 0x01)",
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us] > "Jim C. Nasby" <jnasby@pervasive.com> writes: > > On Fri, Oct 28, 2005 at 05:45:51PM -0400, Tom Lane wrote: > >> Jim, are you interested > >> in seeing if this patch makes the problem go away for you? > > > Well, this is a production system... what's the risk with > that patch? > > Well, it's utterly untested, which means it might crash your system, > which is where you are now, no? Yes, but the crashes are somewhat sporadic and most importantly they don't appear to involve any data loss/corruption. Ijust don't want to make matters any worse. In any case, my client's gone home for the weekend, so I doubt anything would happen until Monday. > > BTW, is it typical to see a 10 difference between asserts > on and off? My > > client has a process that was doing 10-20 records/sec with > asserts on > > and 90-110 with asserts off. > > Not typical, but I can believe there are some code paths like that. Yeah, they're doing some not-so-good things like row-by-row operations, so that's probably what the issue is. I seem to recall20% being the penalty that's normally thrown around, so I was just surprised by such a huge difference.
OK, I think I see it. The problem is that the code in slru.c is careful about not modifying state when it doesn't hold the proper lock, but not so careful about not *inspecting* state without the proper lock. In particular consider these lines in SimpleLruReadPage (line numbers are as in CVS tip): /* Mark the slot read-busy (no-op if it already was) */ 277 shared->page_number[slotno] = pageno; 278 shared->page_status[slotno] = SLRU_PAGE_READ_IN_PROGRESS; ... /* Release shared lock, grab per-buffer lock instead */ 287 LWLockRelease(shared->ControlLock); 288 LWLockAcquire(shared->buffer_locks[slotno], LW_EXCLUSIVE); /* * Check to see if someone else already did the read, or took * the buffer away from us. If so, restart fromthe top. */ 294 if (shared->page_number[slotno] != pageno || 295 shared->page_status[slotno] != SLRU_PAGE_READ_IN_PROGRESS) ... Suppose that we arrive at line 277 when the page is currently being faulted in by another process (ie, its state is already read-in-progress). The assignments at lines 277 & 278 are then no-ops, and we'll block at line 288 waiting for the other guy to finish the I/O and release the per-buffer lock. Suppose that after the I/O finishes and before we get to run again, this buffer sinks back to the bottom of the LRU list and gets chosen to be replaced with another page. Some other process will then start executing this code and will change the page number (line 277) and change the state from clean to read-in-progress (line 278). The race condition is that this could happen between the tests at lines 294 and 295 in our original process. In that case the original process would think that the page still needed to be read in, and would set about doing so. It would discover its error at the Assert at line 308, exactly where Jim reports a problem. The association we thought we'd noted to recursion through SlruSelectLRUPage is in part a red herring: the race can occur without that. However, it's perhaps a bit more probable to occur in that path, because when SlruSelectLRUPage recurses back to SimpleLruReadPage, we *know* that there is another process currently doing read-in, and so the first coincidence is already satisfied. Still, the race condition window is extremely narrow, only a couple of instructions. I looked at the assembly output for the compiler Jim is using, and it looks like this: cmpl %r13d, 104(%rbp,%r12,4)je .L155 .L116:... code for if() body here... .L155:cmpl $1, 72(%rbp,%r12,4)jne .L116... code for subsequent lines here However, if the processor predicts the forward branch not to be taken, it could waste a few cycles recovering from its mistake, so the window maybe is a little wider than it appears. I'd like Jim to test this theory by seeing if it helps to reverse the order of the if-test elements at lines 294/295, ie make it look like if (shared->page_status[slotno] != SLRU_PAGE_READ_IN_PROGRESS || shared->page_number[slotno] != pageno) This won't do as a permanent patch, because it isn't guaranteed to fix the problem on machines that don't strongly order writes, but it should work on Opterons, at least well enough to confirm the diagnosis. I'm still thinking about how to make a real fix without introducing another lock/unlock cycle --- we can do that if we have to, but I think maybe it's possible to fix it without. SimpleLruWritePage has an identical problem BTW :-( regards, tom lane
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags & 0x01)",)
От
"Jim C. Nasby"
Дата:
On Sun, Oct 30, 2005 at 06:17:53PM -0500, Tom Lane wrote: > I'd like Jim to test this theory by seeing if it helps to reverse the > order of the if-test elements at lines 294/295, ie make it look like > > if (shared->page_status[slotno] != SLRU_PAGE_READ_IN_PROGRESS || > shared->page_number[slotno] != pageno) > > This won't do as a permanent patch, because it isn't guaranteed to fix > the problem on machines that don't strongly order writes, but it should > work on Opterons, at least well enough to confirm the diagnosis. Given your proposed fix on -patches, do you still need me to test this? Also, is there any heap corruption risk associated with this patch? I'm also wondering what the effect of this is when assertions are turned off. My client had to go back to running with assertions turned off because of the performance impact. Are they now risking data corruption? Is there a way to turn on the assertion just in this code segment? This incident has made me wonder if it's worth creating two classes of assertions. The (hopefully more common) set of assertions would be for things that shouldn't happen, but if go un-caught won't result in heap corruption. A new set (well, existing asserts, but just re-classified) would be for things that if uncaught could result in heap corruption. My hope is that the set of critical assertions could be turned on by default, helping to identify race conditions and other bugs that conventional testing is unlikely to find. -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags & 0x01)",)
От
"Jim C. Nasby"
Дата:
Sorry, two more things... Will increasing shared_buffers make this less likely to occur? Or is this just something that's likely to happen when there are things like seqscans that are putting buffers near the front of the LRU? (The 8.0.3 buffer manager does something like that, right?) Is this something that a test case can be created for? I know someone submitted a framework for doing concurrent testing... -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags
От
Bruce Momjian
Дата:
Jim C. Nasby wrote: > On Sun, Oct 30, 2005 at 06:17:53PM -0500, Tom Lane wrote: > > I'd like Jim to test this theory by seeing if it helps to reverse the > > order of the if-test elements at lines 294/295, ie make it look like > > > > if (shared->page_status[slotno] != SLRU_PAGE_READ_IN_PROGRESS || > > shared->page_number[slotno] != pageno) > > > > This won't do as a permanent patch, because it isn't guaranteed to fix > > the problem on machines that don't strongly order writes, but it should > > work on Opterons, at least well enough to confirm the diagnosis. > > Given your proposed fix on -patches, do you still need me to test this? > Also, is there any heap corruption risk associated with this patch? Because it is a test, I am not sure there is any way to know what the possible impact of a bug is. If we knew there were bug in the patch, it would have been fixed already. > I'm also wondering what the effect of this is when assertions are turned > off. My client had to go back to running with assertions turned off > because of the performance impact. Are they now risking data corruption? > Is there a way to turn on the assertion just in this code segment? > > This incident has made me wonder if it's worth creating two classes of > assertions. The (hopefully more common) set of assertions would be for > things that shouldn't happen, but if go un-caught won't result in heap > corruption. A new set (well, existing asserts, but just re-classified) > would be for things that if uncaught could result in heap corruption. My > hope is that the set of critical assertions could be turned on by > default, helping to identify race conditions and other bugs that > conventional testing is unlikely to find. That is probably overkill. Running with test patches isn't something we expect folks to do often. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
"Jim C. Nasby" <jnasby@pervasive.com> writes: > On Sun, Oct 30, 2005 at 06:17:53PM -0500, Tom Lane wrote: >> This won't do as a permanent patch, because it isn't guaranteed to fix >> the problem on machines that don't strongly order writes, but it should >> work on Opterons, at least well enough to confirm the diagnosis. > Given your proposed fix on -patches, do you still need me to test this? Yes; we still need to verify that my theory actually explains your problem. Given that I'm positing that you can repeatedly hit a two-instruction window, this is by no means a sure thing. We need it tested (and with asserts on, so that we can tell if it's fixed the problem or not). > Also, is there any heap corruption risk associated with this patch? Look, Jim, I'm trying to help you fix this. Are you going to help or not? If you want some kind of written guarantee, you're not going to get one. regards, tom lane
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags & 0x01)",)
От
"Jim C. Nasby"
Дата:
On Mon, Oct 31, 2005 at 01:05:06PM -0500, Tom Lane wrote: > "Jim C. Nasby" <jnasby@pervasive.com> writes: > > On Sun, Oct 30, 2005 at 06:17:53PM -0500, Tom Lane wrote: > >> This won't do as a permanent patch, because it isn't guaranteed to fix > >> the problem on machines that don't strongly order writes, but it should > >> work on Opterons, at least well enough to confirm the diagnosis. > > > Given your proposed fix on -patches, do you still need me to test this? > > Yes; we still need to verify that my theory actually explains your > problem. Given that I'm positing that you can repeatedly hit a > two-instruction window, this is by no means a sure thing. We need > it tested (and with asserts on, so that we can tell if it's fixed > the problem or not). Ok, I'll work on getting this tested. Just to clarify, if this fixes it then the problem wouldn't occur, or would we just see a different assert? > > Also, is there any heap corruption risk associated with this patch? > > Look, Jim, I'm trying to help you fix this. Are you going to help or not? > If you want some kind of written guarantee, you're not going to get one. Of course not, and I'm not looking for one. On the otherhand, I don't want to recommend something on a production system without understanding what kind of risks are involved, and unfortunately much of this is still over my head. I would really like to have a better idea of what the impact of this bug is. -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags
От
Bruce Momjian
Дата:
Tom Lane wrote: > "Jim C. Nasby" <jnasby@pervasive.com> writes: > > On Sun, Oct 30, 2005 at 06:17:53PM -0500, Tom Lane wrote: > >> This won't do as a permanent patch, because it isn't guaranteed to fix > >> the problem on machines that don't strongly order writes, but it should > >> work on Opterons, at least well enough to confirm the diagnosis. > > > Given your proposed fix on -patches, do you still need me to test this? > > Yes; we still need to verify that my theory actually explains your > problem. Given that I'm positing that you can repeatedly hit a > two-instruction window, this is by no means a sure thing. We need > it tested (and with asserts on, so that we can tell if it's fixed > the problem or not). > > > Also, is there any heap corruption risk associated with this patch? > > Look, Jim, I'm trying to help you fix this. Are you going to help or not? > If you want some kind of written guarantee, you're not going to get one. I think we can say Jim gets his money back if he finds a bug. :-) -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags
От
"Jim C. Nasby"
Дата:
On Mon, Oct 31, 2005 at 01:01:14PM -0500, Bruce Momjian wrote: > > This incident has made me wonder if it's worth creating two classes of > > assertions. The (hopefully more common) set of assertions would be for > > things that shouldn't happen, but if go un-caught won't result in heap > > corruption. A new set (well, existing asserts, but just re-classified) > > would be for things that if uncaught could result in heap corruption. My > > hope is that the set of critical assertions could be turned on by > > default, helping to identify race conditions and other bugs that > > conventional testing is unlikely to find. > > That is probably overkill. Running with test patches isn't something we > expect folks to do often. I wasn't thinking about test patches. My assumption is that the asserts that are currently in place fall into one of two categories: either they check for something that if false could result in data corruption in the heap, or they check for something that shouldn't happen, but if it does it can't corrupt the heap. If that assumption is correct then seperating them might make it easier to run with the set of critical asserts turned on. Currently, there can be a substantial performance penalty with all asserts turned on, but I suspect a lot of that penalty is from asserts in things like parsing and planning code; code that pretty much couldn't corrupt data. -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags
От
Bruce Momjian
Дата:
Jim C. Nasby wrote: > On Mon, Oct 31, 2005 at 01:01:14PM -0500, Bruce Momjian wrote: > > > This incident has made me wonder if it's worth creating two classes of > > > assertions. The (hopefully more common) set of assertions would be for > > > things that shouldn't happen, but if go un-caught won't result in heap > > > corruption. A new set (well, existing asserts, but just re-classified) > > > would be for things that if uncaught could result in heap corruption. My > > > hope is that the set of critical assertions could be turned on by > > > default, helping to identify race conditions and other bugs that > > > conventional testing is unlikely to find. > > > > That is probably overkill. Running with test patches isn't something we > > expect folks to do often. > > I wasn't thinking about test patches. > > My assumption is that the asserts that are currently in place fall into > one of two categories: either they check for something that if false > could result in data corruption in the heap, or they check for something > that shouldn't happen, but if it does it can't corrupt the heap. If that > assumption is correct then seperating them might make it easier to run > with the set of critical asserts turned on. Currently, there can be a > substantial performance penalty with all asserts turned on, but I > suspect a lot of that penalty is from asserts in things like parsing and > planning code; code that pretty much couldn't corrupt data. There is no way if the system has some incorrect value whether that would later corrupt the data or not. Anything the system does that it shouldn't do is a potential corruption problem. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags
От
"Jim C. Nasby"
Дата:
On Mon, Oct 31, 2005 at 01:34:17PM -0500, Bruce Momjian wrote: > There is no way if the system has some incorrect value whether that > would later corrupt the data or not. Anything the system does that it > shouldn't do is a potential corruption problem. But is it safe to say that there are areas where a failed assert is far more likely to result in data corruption? And that there's also areas where there's likely to be difficult/impossible to find bugs, such as race conditions? ISTM that it would be valuable to do some additional checking in these critical areas. -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags
От
Gregory Maxwell
Дата:
On 10/31/05, Jim C. Nasby <jnasby@pervasive.com> wrote: > On Mon, Oct 31, 2005 at 01:34:17PM -0500, Bruce Momjian wrote: > > There is no way if the system has some incorrect value whether that > > would later corrupt the data or not. Anything the system does that it > > shouldn't do is a potential corruption problem. > But is it safe to say that there are areas where a failed assert is far > more likely to result in data corruption? And that there's also areas > where there's likely to be difficult/impossible to find bugs, such as > race conditions? ISTM that it would be valuable to do some additional > checking in these critical areas. There are, no doubt, also places where an assert has minimal to no performance impact. I'd wager a guess that the intersection of low impact asserts, and asserts which measure high risk activities, is small enough to be uninteresting.
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags & 0x01)",)
От
"Jim C. Nasby"
Дата:
Now that I've got a little better idea of what this code does, I've noticed something interesting... this issue is happening on an 8-way machine, and NUM_SLRU_BUFFERS is currently defined at 8. Doesn't this greatly increase the odds of buffer conflicts? Bug aside, would it be better to set NUM_SLRU_BUFFERS higher for a larger number of CPUs? Also, something else to note is that this database can see a pretty high transaction rate... I just checked and it was doing 200TPS, but iirc it can hit 1000+ TPS during the day. -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags & 0x01)",)
От
Alvaro Herrera
Дата:
Jim C. Nasby wrote: > Now that I've got a little better idea of what this code does, I've > noticed something interesting... this issue is happening on an 8-way > machine, and NUM_SLRU_BUFFERS is currently defined at 8. Doesn't this > greatly increase the odds of buffer conflicts? Bug aside, would it be > better to set NUM_SLRU_BUFFERS higher for a larger number of CPUs? We had talked about increasing NUM_SLRU_BUFFERS depending on shared_buffers, but it didn't get done. Something to consider for 8.2 though. I think you could have better performance by increasing that setting, while at the same time dimishing the possibility that the race condition appears. I think you should also consider increasing PGPROC_MAX_CACHED_SUBXIDS (src/include/storage/proc.h), because that should decrease the chance that the subtrans area needs to be scanned. By how much, however, I wouldn't know -- it depends on the number of subtransactions you typically have; I guess you could activate the measuring code in procarray.c to get a figure. -- Alvaro Herrera http://www.amazon.com/gp/registry/CTMLCN8V17R4 www.google.com: interfaz de línea de comando para la web.
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags & 0x01)",)
От
"Jim C. Nasby"
Дата:
On Mon, Oct 31, 2005 at 09:02:59PM -0300, Alvaro Herrera wrote: > Jim C. Nasby wrote: > > Now that I've got a little better idea of what this code does, I've > > noticed something interesting... this issue is happening on an 8-way > > machine, and NUM_SLRU_BUFFERS is currently defined at 8. Doesn't this > > greatly increase the odds of buffer conflicts? Bug aside, would it be > > better to set NUM_SLRU_BUFFERS higher for a larger number of CPUs? > > We had talked about increasing NUM_SLRU_BUFFERS depending on > shared_buffers, but it didn't get done. Something to consider for 8.2 > though. I think you could have better performance by increasing that > setting, while at the same time dimishing the possibility that the race > condition appears. Ok, I'll look into that. This database is definately having issues due to the sheer transaction volume, so maybe that will help. If NUM_SLRU_BUFFERS were to be tied to something, wouldn't it make more sense to tie it to wal_buffers though? One example is a data warehouse might have a very high shared_buffers, but most likely won't have a high transaction rate. ISTM that most databases with a high transaction rate are likely to have increased wal_buffers. > I think you should also consider increasing PGPROC_MAX_CACHED_SUBXIDS > (src/include/storage/proc.h), because that should decrease the chance > that the subtrans area needs to be scanned. By how much, however, I > wouldn't know -- it depends on the number of subtransactions you > typically have; I guess you could activate the measuring code in > procarray.c to get a figure. AFAIK they're not using subtransactions at all, but I'll check. Is there anywhere this stuff is documented other than in code? It sounds like an advanced tuning guide would be very valuable for environments like this one... -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
"Jim C. Nasby" <jnasby@pervasive.com> writes: > AFAIK they're not using subtransactions at all, but I'll check. Well, yeah, they are ... else you'd never have seen this failure. regards, tom lane
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags & 0x01)",)
От
Alvaro Herrera
Дата:
Tom Lane wrote: > "Jim C. Nasby" <jnasby@pervasive.com> writes: > > AFAIK they're not using subtransactions at all, but I'll check. > > Well, yeah, they are ... else you'd never have seen this failure. Maybe it's in plpgsql EXCEPTION clauses. -- Alvaro Herrera Valdivia, Chile ICBM: S 39º 49' 17.7", W 73º 14' 26.8" "I can't go to a restaurant and order food because I keep looking at the fonts on the menu. Five minutes later I realize that it's also talking about food" (Donald Knuth)
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags & 0x01)",)
От
"Jim C. Nasby"
Дата:
On Tue, Nov 01, 2005 at 11:23:55AM -0300, Alvaro Herrera wrote: > Tom Lane wrote: > > "Jim C. Nasby" <jnasby@pervasive.com> writes: > > > AFAIK they're not using subtransactions at all, but I'll check. > > > > Well, yeah, they are ... else you'd never have seen this failure. > > Maybe it's in plpgsql EXCEPTION clauses. They say they're not using either. Doesn't clog use the same code? -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags & 0x01)",)
От
"Jim C. Nasby"
Дата:
On Tue, Nov 01, 2005 at 11:23:55AM -0300, Alvaro Herrera wrote: > Tom Lane wrote: > > "Jim C. Nasby" <jnasby@pervasive.com> writes: > > > AFAIK they're not using subtransactions at all, but I'll check. > > > > Well, yeah, they are ... else you'd never have seen this failure. > > Maybe it's in plpgsql EXCEPTION clauses. Err, I forgot they're using Slony, which is probably using savepoints and/or exceptions. -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
"Jim C. Nasby" <jnasby@pervasive.com> writes: > Doesn't clog use the same code? Yeah, but all three of your examples were referencing pg_subtrans, as proven by the stack traces and the contents of the shared control block. Even though the bug seems completely clog.c's fault, this is not a coincidence: if subtransactions are being used heavily, then pg_subtrans would have far greater I/O volume than any of the other clog-managed logs, and hence have more exposure to the race condition. We really ought to fix that code so that pg_subtrans can have more buffers than pg_clog... regards, tom lane
I wrote: > Even though the bug seems completely clog.c's fault, s/clog.c/slru.c/ of course :-( regards, tom lane
jnasby@pervasive.com ("Jim C. Nasby") writes: > AFAIK they're not using subtransactions at all, but I'll check. Are they perchance using pl/PerlNG? We discovered a problem with Slony-I's handling of subtransactions which was exposed by pl/PerlNG, which evidently wraps its SPI calls inside subtransactions. For more details... <http://gborg.postgresql.org/project/slony1/bugs/bugupdate.php?1293> That is the only subtransaction issue I am aware of... -- select 'cbbrowne' || '@' || 'acm.org'; http://www.ntlug.org/~cbbrowne/nonrdbms.html :FATAL ERROR -- ERROR IN ERROR HANDLER
jnasby@pervasive.com ("Jim C. Nasby") writes: > On Tue, Nov 01, 2005 at 11:23:55AM -0300, Alvaro Herrera wrote: >> Tom Lane wrote: >> > "Jim C. Nasby" <jnasby@pervasive.com> writes: >> > > AFAIK they're not using subtransactions at all, but I'll check. >> > >> > Well, yeah, they are ... else you'd never have seen this failure. >> >> Maybe it's in plpgsql EXCEPTION clauses. > > Err, I forgot they're using Slony, which is probably using savepoints > and/or exceptions. Slony-I does use exceptions in pretty conventional ways; it does *not* make any use of subtransactions, because it needs to run on PG 7.3 and 7.4 that do not support subtransactions. -- (format nil "~S@~S" "cbbrowne" "acm.org") http://www3.sympatico.ca/cbbrowne/linuxxian.html "I can't believe my room doesn't have Ethernet! Why wasn't it wired when the house was built?" "The house was built in 1576." -- Alex Kamilewicz on the Oxford breed of `conference American.'
Chris Browne wrote: > jnasby@pervasive.com ("Jim C. Nasby") writes: > > > On Tue, Nov 01, 2005 at 11:23:55AM -0300, Alvaro Herrera wrote: > >> Tom Lane wrote: > >> > "Jim C. Nasby" <jnasby@pervasive.com> writes: > >> > > AFAIK they're not using subtransactions at all, but I'll check. > >> > > >> > Well, yeah, they are ... else you'd never have seen this failure. > >> > >> Maybe it's in plpgsql EXCEPTION clauses. > > > > Err, I forgot they're using Slony, which is probably using savepoints > > and/or exceptions. > > Slony-I does use exceptions in pretty conventional ways; it does *not* > make any use of subtransactions, because it needs to run on PG 7.3 and > 7.4 that do not support subtransactions. Hmm, does it use the BEGIN/EXCEPTION/END construct at all? Because if it does, it won't work on 7.4; and if it doesn't, then it isn't using savepoints in 8.0 either. -- Alvaro Herrera http://www.amazon.com/gp/registry/5ZYLFMCVHXC "Siempre hay que alimentar a los dioses, aunque la tierra esté seca" (Orual)
alvherre@alvh.no-ip.org (Alvaro Herrera) writes: > Chris Browne wrote: >> jnasby@pervasive.com ("Jim C. Nasby") writes: >> >> > On Tue, Nov 01, 2005 at 11:23:55AM -0300, Alvaro Herrera wrote: >> >> Tom Lane wrote: >> >> > "Jim C. Nasby" <jnasby@pervasive.com> writes: >> >> > > AFAIK they're not using subtransactions at all, but I'll check. >> >> > >> >> > Well, yeah, they are ... else you'd never have seen this failure. >> >> >> >> Maybe it's in plpgsql EXCEPTION clauses. >> > >> > Err, I forgot they're using Slony, which is probably using savepoints >> > and/or exceptions. >> >> Slony-I does use exceptions in pretty conventional ways; it does *not* >> make any use of subtransactions, because it needs to run on PG 7.3 and >> 7.4 that do not support subtransactions. > > Hmm, does it use the BEGIN/EXCEPTION/END construct at all? Because if > it does, it won't work on 7.4; and if it doesn't, then it isn't using > savepoints in 8.0 either. Ah, then I was misreading that. There are instances of RAISE EXCEPTION, which was what I had in mind, but not of BEGIN/EXCEPTION/END. There is some logic in 8.x to *detect* the nesting of transactions, but that's quite another matter. -- output = ("cbbrowne" "@" "acm.org") http://cbbrowne.com/info/multiplexor.html "If I could find a way to get [Saddam Hussein] out of there, even putting a contract out on him, if the CIA still did that sort of a thing, assuming it ever did, I would be for it." -- Richard M. Nixon
Bruce Momjian <pgman@candle.pha.pa.us> writes: > Jim C. Nasby wrote: > > > My assumption is that the asserts that are currently in place fall into > > one of two categories: either they check for something that if false > > could result in data corruption in the heap, or they check for something > > that shouldn't happen, but if it does it can't corrupt the heap. If that > > assumption is correct then seperating them might make it easier to run > > with the set of critical asserts turned on. Currently, there can be a > > substantial performance penalty with all asserts turned on, but I > > suspect a lot of that penalty is from asserts in things like parsing and > > planning code; code that pretty much couldn't corrupt data. > > There is no way if the system has some incorrect value whether that > would later corrupt the data or not. Anything the system does that it > shouldn't do is a potential corruption problem. That's true but the reason why is subtler than that. If something has happened that "can't happen" then there's no way to know how you arrived in that situation. You might already have major problems that can lead to data corruption -- or for that matter have already caused data corruption.. I happen to think that except for the rare assertion that has major performance impact all the assertions should be on in production builds. The goal of assertions is to catch corruption quickly and that's something that's just as important in production as it is in debugging. -- greg
Greg Stark <gsstark@mit.edu> writes: > I happen to think that except for the rare assertion that has major > performance impact all the assertions should be on in production builds. The > goal of assertions is to catch corruption quickly and that's something that's > just as important in production as it is in debugging. You seem not to have read the documentation: <term><option>--enable-cassert</option></term> Enables <firstterm>assertion</> checks in the server, which test for many <quote>can't happen</> conditions. This is invaluable for code development purposes, but the tests slow things down a little. Also,having the tests turned on won't necessarily enhance the stability of your server! The assertion checks arenot categorized for severity, and so what might be a relatively harmless bug will still lead to server restartsif it triggers an assertion failure. Currently, this option is not recommended for production use,but you should have it on for development work or when running a beta version. The great thing about Assert() is that you can throw one in for any condition that your code is assuming-without-proof, without having to think too hard about consequences. If we were to recommend having enable-cassert on in production databases, we would need a MUCH higher standard of care about when to use Assert. I would bet that ninety percent of the Asserts in the existing code are on conditions that could represent, at worst, corruption of backend-local or even transaction-local data structures. Taking down the entire database cluster for that is not something that sounds like a stability-enhancing tradeoff to me. In other words, the "you don't know how bad it might be" theory has a flip side: you can't cry wolf when there's no wolf, either, at least not if you want to continue to be listened to. regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > Greg Stark <gsstark@mit.edu> writes: > > I happen to think that except for the rare assertion that has major > > performance impact all the assertions should be on in production builds. The > > goal of assertions is to catch corruption quickly and that's something that's > > just as important in production as it is in debugging. > > You seem not to have read the documentation: Sure I have, I just disagree. > I would bet that ninety percent of the Asserts in the existing code are on > conditions that could represent, at worst, corruption of backend-local or > even transaction-local data structures. Taking down the entire database > cluster for that is not something that sounds like a stability-enhancing > tradeoff to me. It may be minor corruption or it may be that the reason for the minor corruption comes from some larger bug. It may also be backend-local or transaction-local corruption at the time the assert catches it but cause major damage by the time it actually crashes a non-assert-enabled database. -- greg
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags
От
"Jim C. Nasby"
Дата:
On Wed, Nov 02, 2005 at 07:03:57AM -0500, Greg Stark wrote: > > I would bet that ninety percent of the Asserts in the existing code are on > > conditions that could represent, at worst, corruption of backend-local or > > even transaction-local data structures. Taking down the entire database > > cluster for that is not something that sounds like a stability-enhancing > > tradeoff to me. > > It may be minor corruption or it may be that the reason for the minor > corruption comes from some larger bug. It may also be backend-local or > transaction-local corruption at the time the assert catches it but cause major > damage by the time it actually crashes a non-assert-enabled database. Agreed. Personally I'd want to know about anything that corrupts my data, no matter what the locality. I would also argue that if people are seeing 'minor' asserts firing in production that there's a bug that needs to be tracked down. IF it comes out that there's some asserts that can be fired even though there's not anything really bad happening, they could always be relegated to a second class of assert that's not normally turned on. BTW, that's a reversal from what I was originally arguing for, which was due to the performance penalty associated with --enable-cassert. My client is now running with Tom's suggestion of commenting out CLOBBER_FREED_MEMORY and MEMORY_CONTEXT_CHECKING and performance is good. It appears to be as good as it was with asserts disabled. So I think it would definately be good to break those options out from --enable-cassert. That makes it viable to run with asserts in production, at least from a performance standpoint. BTW, they're also running with patch2 now. Previously, with asserts turned on and without the patch, they were seeing assert failures on average of 2/day. So hopefully tomorrow we'll have an idea if the patch fixed this or not. -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
"Jim C. Nasby" <jnasby@pervasive.com> writes: > BTW, that's a reversal from what I was originally arguing for, which was > due to the performance penalty associated with --enable-cassert. My > client is now running with Tom's suggestion of commenting out > CLOBBER_FREED_MEMORY and MEMORY_CONTEXT_CHECKING and performance is > good. It appears to be as good as it was with asserts disabled. Interesting. I've always wondered whether the "debug_assertions" GUC variable is worth the electrons it's printed on. If you are running with asserts active, that variable actually slows things down, by requiring an additional bool test for every Assert. I suppose the motivation was to allow the same compiled executable to be used for both assert-enabled and assert-disabled runs, but how many people really need that capability? regards, tom lane
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags
От
"Jim C. Nasby"
Дата:
On Wed, Nov 02, 2005 at 02:04:09PM -0500, Tom Lane wrote: > "Jim C. Nasby" <jnasby@pervasive.com> writes: > > BTW, that's a reversal from what I was originally arguing for, which was > > due to the performance penalty associated with --enable-cassert. My > > client is now running with Tom's suggestion of commenting out > > CLOBBER_FREED_MEMORY and MEMORY_CONTEXT_CHECKING and performance is > > good. It appears to be as good as it was with asserts disabled. > > Interesting. I've always wondered whether the "debug_assertions" GUC > variable is worth the electrons it's printed on. If you are running > with asserts active, that variable actually slows things down, by > requiring an additional bool test for every Assert. I suppose the > motivation was to allow the same compiled executable to be used for both > assert-enabled and assert-disabled runs, but how many people really need > that capability? Not sure how that relates to CLOBBER_FREED_MEMORY and MEMORY_CONTEXT_CHECKING :P, but I agree that it doesn't make sense to have a GUC, at least not if asserts default to being compiled out. Hrm... does debug_assertions end up changing assert_enabled? BTW, is MEMORY_CONTEXT_CHECKING that expensive? It seems like it shouldn't be, but I'm only guessing at what exactly it does... -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags
От
Alvaro Herrera
Дата:
Jim C. Nasby wrote: > BTW, is MEMORY_CONTEXT_CHECKING that expensive? It seems like it > shouldn't be, but I'm only guessing at what exactly it does... Yes, because not only it checks marker bytes at the end of palloc chunks and similar stuff, but it also overwrites whole contexts with 0x7f when they are reset. May I propose to make Assert() yield only WARNINGs, and take out the most expensive parts of MEMORY_CONTEXT_CHECKING, when --enable-cassert is disabled? Enabling it would trigger the current FailedAssertion and all of MEMORY_CONTEXT_CHECKING. That way we get all bug reports without the expensive runtime costs for in-production systems. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes: > May I propose to make Assert() yield only WARNINGs, That is a horrid idea --- for one thing, it means that Asserts inside the elog machinery itself would be instant infinite recursion, and even elsewhere you'd have to think a bit about whether it's ok to call the elog machinery. Plus, once you *have* detected an assertion failure, allowing the code to keep running is just silly. Either they dump core or they're disabled, there is no third option. I do think it would be reasonable to fix things so that MEMORY_CONTEXT_CHECKING could be turned on and off at runtime. Perhaps rather than an all-or-nothing debug_assertions GUC variable, what we want is something that turns on or off "expensive" assertion checks at runtime. This could include MEMORY_CONTEXT_CHECKING and anything else where the actual checking of the condition is nontrivial. (For instance, there is code in list.c that grovels over the whole list for a consistency check --- that is "expensive". There is some code in the bufmgr that scans through all the buffers --- ditto.) regards, tom lane
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags
От
"Jim C. Nasby"
Дата:
On Thu, Nov 03, 2005 at 11:11:42AM -0500, Tom Lane wrote: > Perhaps rather than an all-or-nothing debug_assertions GUC variable, > what we want is something that turns on or off "expensive" assertion > checks at runtime. This could include MEMORY_CONTEXT_CHECKING and > anything else where the actual checking of the condition is nontrivial. > (For instance, there is code in list.c that grovels over the whole > list for a consistency check --- that is "expensive". There is some > code in the bufmgr that scans through all the buffers --- ditto.) Two levels of assertions sounds like a great idea... wish I'd thought of it! ;P Seriously, I am wondering about the performance hit of always checking debug_assertions. http://archives.postgresql.org/pgsql-hackers/2005-08/msg00389.php indicates that even with debug_assertions=false, --enable-cassert has a big performance impact. I don't really see any reason for debug_assertions unless we start defaulting to assertions being compiled in. If we're going to split things up maybe the expensive assertions you mention should get a different macro so that there's no performance hit unless you specifically compile them in. Michael's email tends to indicate that going the other way around (one macro, two GUC's) wouldn't do any good. BTW, I can do some testing of assert performance impact with dbt2 on a Solaris box if anyone's interested in the data... -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
"Jim C. Nasby" <jnasby@pervasive.com> writes: > Seriously, I am wondering about the performance hit of always checking > debug_assertions. > http://archives.postgresql.org/pgsql-hackers/2005-08/msg00389.php > indicates that even with debug_assertions=false, --enable-cassert has a > big performance impact. That's because MEMORY_CONTEXT_CHECKING happens anyway --- it's not currently switched off by the GUC variable. I don't think we have any recent data point about the cost of Asserts per se, except your own report that it seems minimal. My thought is that it would be even more minimal if the tests of debug_assertion were removed ;-) ... except in association with Asserts that are actually testing an expensive-to-check condition, of which there are very few. regards, tom lane
Re: slru.c race condition (was Re: TRAP: FailedAssertion("!((itemid)->lp_flags
От
Bruce Momjian
Дата:
Added to TODO: * Consider increasing internal areas when shared buffers is increased http://archives.postgresql.org/pgsql-hackers/2005-10/msg01419.php --------------------------------------------------------------------------- Alvaro Herrera wrote: > Jim C. Nasby wrote: > > Now that I've got a little better idea of what this code does, I've > > noticed something interesting... this issue is happening on an 8-way > > machine, and NUM_SLRU_BUFFERS is currently defined at 8. Doesn't this > > greatly increase the odds of buffer conflicts? Bug aside, would it be > > better to set NUM_SLRU_BUFFERS higher for a larger number of CPUs? > > We had talked about increasing NUM_SLRU_BUFFERS depending on > shared_buffers, but it didn't get done. Something to consider for 8.2 > though. I think you could have better performance by increasing that > setting, while at the same time dimishing the possibility that the race > condition appears. > > I think you should also consider increasing PGPROC_MAX_CACHED_SUBXIDS > (src/include/storage/proc.h), because that should decrease the chance > that the subtrans area needs to be scanned. By how much, however, I > wouldn't know -- it depends on the number of subtransactions you > typically have; I guess you could activate the measuring code in > procarray.c to get a figure. > > -- > Alvaro Herrera http://www.amazon.com/gp/registry/CTMLCN8V17R4 > www.google.com: interfaz de l?nea de comando para la web. > > ---------------------------(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 > -- Bruce Momjian http://candle.pha.pa.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +