Обсуждение: SSI bug?
hi, it seems that PredicateLockTupleRowVersionLink sometimes create a loop of targets (it founds an existing 'newtarget' whose nextVersionOfRow chain points to the 'oldtarget') and it later causes CheckTargetForConflictsIn loop forever. YAMAMOTO Takashi
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> wrote: > it seems that PredicateLockTupleRowVersionLink sometimes create > a loop of targets (it founds an existing 'newtarget' whose > nextVersionOfRow chain points to the 'oldtarget') and it later > causes CheckTargetForConflictsIn loop forever. Is this a hypothetical risk based on looking at the code, or have you seen this actually happen? Either way, could you provide more details? (A reproducible test case would be ideal.) This being the newest part of the code, I'll grant that it is the most likely to have an unidentified bug; but given that the pointers are from one predicate lock target structure identified by a tuple ID to one identified by the tuple ID of the next version of the row, it isn't obvious to me how a cycle could develop. -Kevin
hi, > YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> wrote: > >> it seems that PredicateLockTupleRowVersionLink sometimes create >> a loop of targets (it founds an existing 'newtarget' whose >> nextVersionOfRow chain points to the 'oldtarget') and it later >> causes CheckTargetForConflictsIn loop forever. > > Is this a hypothetical risk based on looking at the code, or have > you seen this actually happen? Either way, could you provide more > details? (A reproducible test case would be ideal.) i have seen this actually happen. i've confirmed the creation of the loop with the attached patch. it's easily reproducable with my application. i can provide the full source code of my application if you want. (but it isn't easy to run unless you are familiar with the recent version of NetBSD) i haven't found a smaller reproducible test case yet. YAMAMOTO Takashi > > This being the newest part of the code, I'll grant that it is the > most likely to have an unidentified bug; but given that the pointers > are from one predicate lock target structure identified by a tuple > ID to one identified by the tuple ID of the next version of the row, > it isn't obvious to me how a cycle could develop. > > -Kevin
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> wrote: > i have seen this actually happen. i've confirmed the creation of > the loop with the attached patch. it's easily reproducable with > my application. i can provide the full source code of my > application if you want. (but it isn't easy to run unless you are > familiar with the recent version of NetBSD) > i haven't found a smaller reproducible test case yet. I've never used NetBSD, so maybe a few details will help point me in the right direction faster than the source code. Has your application ever triggered any of the assertions in the code? (In particular, it would be interesting if it ever hit the one right above where you patched.) How long was the loop? Did you notice whether the loop involved multiple tuples within a single page? Did this coincide with an autovacuum of the table? These last two are of interest because it seems likely that such a cycle might be related to this new code not properly allowing for some aspect of tuple cleanup. Thanks for finding this and reporting it, and thanks in advance for any further detail you can provide. -Kevin
I wrote: > it seems likely that such a cycle might be related to this new > code not properly allowing for some aspect of tuple cleanup. I found a couple places where cleanup could let these fall through the cracks long enough to get stale and still be around when a tuple ID is re-used, causing problems. Please try the attached patch and see if it fixes the problem for you. If it does, then there's no need to try to track the other things I was asking about. Thanks! -Kevin
Вложения
hi, > I wrote: > >> it seems likely that such a cycle might be related to this new >> code not properly allowing for some aspect of tuple cleanup. > > I found a couple places where cleanup could let these fall through > the cracks long enough to get stale and still be around when a tuple > ID is re-used, causing problems. Please try the attached patch and > see if it fixes the problem for you. > > If it does, then there's no need to try to track the other things I > was asking about. thanks. unfortunately the problem still happens with the patch. YAMAMOTO Takashi > > Thanks! > > -Kevin
hi,
all of the following answers are with the patch you provided in
other mail applied.
> YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> wrote:
>
>> i have seen this actually happen. i've confirmed the creation of
>> the loop with the attached patch. it's easily reproducable with
>> my application. i can provide the full source code of my
>> application if you want. (but it isn't easy to run unless you are
>> familiar with the recent version of NetBSD)
>> i haven't found a smaller reproducible test case yet.
>
> I've never used NetBSD, so maybe a few details will help point me in
> the right direction faster than the source code.
>
> Has your application ever triggered any of the assertions in the
> code? (In particular, it would be interesting if it ever hit the
> one right above where you patched.)
the assertion right above is sometimes triggered. sometimes not.
>
> How long was the loop?
see below.
> Did you notice whether the loop involved multiple tuples within a
> single page?
if i understand correctly, yes.
the following is a snippet of my debug code (dump targets when
triggerCheckTargetForConflictsIn loops >1000 times) and its output.
the same locktag_field3 value means the same page, right?
+ for (t = target, i = 0; t != NULL; i++) {
+ elog(WARNING, "[%u] target %p tag %" PRIx32 ":%" PRIx32 ":%" PRIx32
+ ":%" PRIx16 ":%" PRIx16 " prior %p next %p", i, t,
+ t->tag.locktag_field1,
+ t->tag.locktag_field2,
+ t->tag.locktag_field3,
+ t->tag.locktag_field4,
+ t->tag.locktag_field5,
+ t->priorVersionOfRow,
+ t->nextVersionOfRow);
+ t = t->priorVersionOfRow;
+ if (t == target) {
+ elog(WARNING, "found a loop");
+ break;
+ }
+ }
WARNING: [0] target 0xbb51f238 tag 4000:4017:53b:6c:0 prior 0xbb51f350 next 0xbb51f350
WARNING: [1] target 0xbb51f350 tag 4000:4017:53b:69:0 prior 0xbb51f238 next 0xbb51f238
WARNING: found a loop
another sample:
WARNING: [0] target 0xbb51f530 tag 4000:4017:565:ae:0 prior 0xbb51f1e8 next 0xbb51f300
WARNING: [1] target 0xbb51f1e8 tag 4000:4017:565:ad:0 prior 0xbb51f580 next 0xbb51f530
WARNING: [2] target 0xbb51f580 tag 4000:4017:565:ac:0 prior 0xbb51f300 next 0xbb51f1e8
WARNING: [3] target 0xbb51f300 tag 4000:4017:565:ab:0 prior 0xbb51f530 next 0xbb51f580
WARNING: found a loop
the table seems mostly hot-updated, if it matters.
hoge=# select * from pg_stat_user_tables where relid=16407;
-[ RECORD 1 ]-----+--------------------
relid | 16407
schemaname | pgfs
relname | file
seq_scan | 0
seq_tup_read | 0
idx_scan | 53681
idx_tup_fetch | 52253
n_tup_ins | 569
n_tup_upd | 12054
n_tup_del | 476
n_tup_hot_upd | 12041
n_live_tup | 93
n_dead_tup | 559
last_vacuum |
last_autovacuum |
last_analyze |
last_autoanalyze |
vacuum_count | 0
autovacuum_count | 0
analyze_count | 4922528128875102208
autoanalyze_count | 7598807461784802080
(values in the last two columns seems bogus.
i don't know if it's related or not.)
> Did this coincide with an autovacuum of the table?
no.
(assuming that autovacuum=off in postgresql.conf is enough to exclude
the possibility.)
>
> These last two are of interest because it seems likely that such a
> cycle might be related to this new code not properly allowing for
> some aspect of tuple cleanup.
>
> Thanks for finding this and reporting it, and thanks in advance for
> any further detail you can provide.
thanks for looking.
YAMAMOTO Takashi
>
> -Kevin
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
Looking at the prior/next version chaining, aside from the looping issue, isn't it broken by lock promotion too? There's a check in RemoveTargetIfNoLongerUsed() so that we don't release a lock target if its priorVersionOfRow is set, but what if the tuple lock is promoted to a page level lock first, and PredicateLockTupleRowVersionLink() is called only after that? Or can that not happen because of something else that I'm missing? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > Looking at the prior/next version chaining, aside from the > looping issue, isn't it broken by lock promotion too? There's a > check in RemoveTargetIfNoLongerUsed() so that we don't release a > lock target if its priorVersionOfRow is set, but what if the tuple > lock is promoted to a page level lock first, and > PredicateLockTupleRowVersionLink() is called only after that? Or > can that not happen because of something else that I'm missing? I had to ponder that a while. Here's my thinking. Predicate locks only matter when there is a write. Predicate locks on heap tuples only matter when there is an UPDATE or DELETE of a locked tuple. The problem these links are addressing is that an intervening transaction might UPDATE the transaction between the read of the tuple and a later UPDATE or DELETE. We want the second UPDATE to see that it conflicts with a read from before the first UPDATE. The first UPDATE creates the link from the "before" tuple ID the "after" tuple ID at the target level. What predicate locks exist on the second target are irrelevant when it comes to seeing the conflict between the second UPDATE (or DELETE) and the initial read. So I don't see where granularity promotion for locks on the second target is a problem as long as the target itself doesn't get deleted because of the link to the prior version of the tuple. Promotion of the lock granularity on the prior tuple is where we have problems. If the two tuple versions are in separate pages then the second UPDATE could miss the conflict. My first thought was to fix that by requiring promotion of a predicate lock on a tuple to jump straight to the relation level if nextVersionOfRow is set for the lock target and it points to a tuple in a different page. But that doesn't cover a situation where we have a heap tuple predicate lock which gets promoted to page granularity before the tuple is updated. To handle that we would need to say that an UPDATE to a tuple on a page which is predicate locked by the transaction would need to be promoted to relation granularity if the new version of the tuple wasn't on the same page as the old version. That's all doable without too much trouble, but more than I'm likely to get done today. It would be good if someone can confirm my thinking on this first, too. That said, the above is about eliminating false negatives from some corner cases which escaped notice until now. I don't think the changes described above will do anything to prevent the problems reported by YAMAMOTO Takashi. Unless I'm missing something, it sounds like tuple IDs are being changed or reused while predicate locks are held on the tuples. That's probably not going to be overwhelmingly hard to fix if we can identify how that can happen. I tried to cover HOT issues, but it seems likely I missed something.:-( I will be looking at it. -Kevin
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> wrote: >> Did you notice whether the loop involved multiple tuples within a >> single page? > > if i understand correctly, yes. > > the following is a snippet of my debug code (dump targets when > triggerCheckTargetForConflictsIn loops >1000 times) and its > output.the same locktag_field3 value means the same page, right? Right. > the table seems mostly hot-updated, if it matters. > idx_scan | 53681 > idx_tup_fetch | 52253 > n_tup_ins | 569 > n_tup_upd | 12054 > n_tup_del | 476 > n_tup_hot_upd | 12041 > n_live_tup | 93 > n_dead_tup | 559 That probably matters a lot. > analyze_count | 4922528128875102208 > autoanalyze_count | 7598807461784802080 > > (values in the last two columns seems bogus. > i don't know if it's related or not.) That seems unlikely to be related to this problem. It sure does look odd, though. Maybe post that in a separate thread? Thanks for all the additional info. I'll keep digging. -Kevin
On 14.02.2011 20:10, Kevin Grittner wrote: > Promotion of the lock granularity on the prior tuple is where we > have problems. If the two tuple versions are in separate pages then > the second UPDATE could miss the conflict. My first thought was to > fix that by requiring promotion of a predicate lock on a tuple to > jump straight to the relation level if nextVersionOfRow is set for > the lock target and it points to a tuple in a different page. But > that doesn't cover a situation where we have a heap tuple predicate > lock which gets promoted to page granularity before the tuple is > updated. To handle that we would need to say that an UPDATE to a > tuple on a page which is predicate locked by the transaction would > need to be promoted to relation granularity if the new version of > the tuple wasn't on the same page as the old version. Yeah, promoting the original lock on the UPDATE was my first thought too. Another idea is to duplicate the original predicate lock on the first update, so that the original reader holds a lock on both row versions. I think that would ultimately be simpler as we wouldn't need the next-prior chains anymore. For example, suppose that transaction X is holding a predicate lock on tuple A. Transaction Y updates tuple A, creating a new tuple B. Transaction Y sees that X holds a lock on tuple A (or the page containing A), so it acquires a new predicate lock on tuple B on behalf of X. If the updater aborts, the lock on the new tuple needs to be cleaned up, so that it doesn't get confused with later tuple that's stored in the same physical location. We could store the xmin of the tuple in the predicate lock to check for that. Whenever you check for conflict, if the xmin of the lock doesn't match the xmin on the tuple, you know that the lock belonged to an old dead tuple stored in the same location, and can be simply removed as the tuple doesn't exist anymore. > That said, the above is about eliminating false negatives from some > corner cases which escaped notice until now. I don't think the > changes described above will do anything to prevent the problems > reported by YAMAMOTO Takashi. Agreed, it's a separate issue. Although if we change the way we handle the read-update-update problem, the other issue might go away too. > Unless I'm missing something, it > sounds like tuple IDs are being changed or reused while predicate > locks are held on the tuples. That's probably not going to be > overwhelmingly hard to fix if we can identify how that can happen. > I tried to cover HOT issues, but it seems likely I missed something. Storing the xmin of the original tuple would probably help with that too. But it would be nice to understand and be able to reproduce the issue first. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> wrote: > might be unrelated to the loop problem, but... > > i got the following SEGV when runnning vacuum on a table. > vacuum on the table succeeded with the attached patch. Thanks! I appreciate the heavy testing and excellent diagnostics. On the face of it, this doesn't look related to the other problem, but I'll post again soon after closer review. -Kevin
YAMAMOTO Takashi wrote: > might be unrelated to the loop problem, but... Aha! I think it *is* related. There were several places where data was uninitialized here; mostly because Dan was working on this piece while I was working on separate issues which added the new fields. I missed the interaction on integrating the two efforts. :-( The uninitialized fields could lead to all the symptoms you saw. I've reviewed, looking for other similar issues and didn't find any. Could you try the attached patch and see if this fixes the issues you've seen? -Kevin
Вложения
YAMAMOTO Takashi wrote: > with your previous patch or not? With, thanks. -Kevin
hi, > YAMAMOTO Takashi wrote: > >> with your previous patch or not? > > With, thanks. i tried. unfortunately i can still reproduce the original loop problem. WARNING: [0] target 0xbb51ef18 tag 4000:4017:7e3:78:0 prior 0xbb51f148 next 0xb b51edb0 WARNING: [1] target 0xbb51f148 tag 4000:4017:7e3:77:0 prior 0xbb51ef90 next 0xb b51ef18 WARNING: [2] target 0xbb51ef90 tag 4000:4017:7e3:74:0 prior 0xbb51edb0 next 0xb b51f148 WARNING: [3] target 0xbb51edb0 tag 4000:4017:7e3:71:0 prior 0xbb51ef18 next 0xb b51ef90 WARNING: found a loop YAMAMOTO Takashi > > -Kevin > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers
hi, > YAMAMOTO Takashi wrote: > >> might be unrelated to the loop problem, but... > > Aha! I think it *is* related. There were several places where data > was uninitialized here; mostly because Dan was working on this piece > while I was working on separate issues which added the new fields. > I missed the interaction on integrating the two efforts. :-( > The uninitialized fields could lead to all the symptoms you saw. > I've reviewed, looking for other similar issues and didn't find any. > > Could you try the attached patch and see if this fixes the issues > you've seen? with your previous patch or not? YAMAMOTO Takashi > > -Kevin
hi,
might be unrelated to the loop problem, but...
i got the following SEGV when runnning vacuum on a table.
(the line numbers in predicate.c is different as i have local modifications.)
oldlocktag.myTarget was NULL.
it seems that TransferPredicateLocksToNewTarget sometimes use stack garbage
for newpredlocktag.myTarget. vacuum on the table succeeded with the attached
patch. the latter part of the patch was necessary to avoid targetList
corruption, which later seems to make DeleteChildTargetLocks loop inifinitely.
YAMAMOTO Takashi
#0 0x0823cf6c in PredicateLockAcquire (targettag=0xbfbfa734) at predicate.c:1835
#1 0x0823f18a in PredicateLockPage (relation=0x99b4dcf0, blkno=1259) at predicate.c:2206
#2 0x080ac978 in _bt_search (rel=0x99b4dcf0, keysz=2, scankey=0x99a05040, nextkey=0 '\0', bufP=0xbfbfa894,
access=1)at nbtsearch.c:97
#3 0x080a996d in _bt_pagedel (rel=0x99b4dcf0, buf=<value optimized out>, stack=0x0) at nbtpage.c:1059
#4 0x080aacc2 in btvacuumscan (info=0xbfbfbcc4, stats=0x99a01328, callback=0x8184d50 <lazy_tid_reaped>,
callback_state=0x99a012e0, cycleid=13675) at nbtree.c:981
#5 0x080ab15c in btbulkdelete (fcinfo=0xbfbfb9e0) at nbtree.c:573
#6 0x082fde74 in FunctionCall4 (flinfo=0x99b86958, arg1=3217013956, arg2=0, arg3=135810384, arg4=2577404640) at
fmgr.c:1437
#7 0x080a4fd0 in index_bulk_delete (info=0xbfbfbcc4, stats=0x0, callback=0x8184d50 <lazy_tid_reaped>,
callback_state=0x99a012e0) at indexam.c:738
#8 0x08184cd4 in lazy_vacuum_index (indrel=0x99b4dcf0, stats=0x99a023e0, vacrelstats=0x99a012e0) at
vacuumlazy.c:938
#9 0x081854b6 in lazy_vacuum_rel (onerel=0x99b47650, vacstmt=0x99b059d0, bstrategy=0x99a07018,
scanned_all=0xbfbfcfd8"") at vacuumlazy.c:762
#10 0x08184265 in vacuum_rel (relid=16424, vacstmt=0x99b059d0, do_toast=1 '\001', for_wraparound=0 '\0',
scanned_all=0xbfbfcfd8"") at vacuum.c:978
#11 0x081845ea in vacuum (vacstmt=0x99b059d0, relid=0, do_toast=1 '\001', bstrategy=0x0, for_wraparound=0 '\0',
isTopLevel=1'\001') at vacuum.c:230
#12 0xbbab50c3 in pgss_ProcessUtility (parsetree=0x99b059d0, queryString=0x99b05018 "vacuum (verbose,analyze)
pgfs.dirent;", params=0x0, isTopLevel=1 '\001', dest=0x99b05b80, completionTag=0xbfbfd21a "") at
pg_stat_statements.c:603
#13 0x082499ea in PortalRunUtility (portal=0x99b33018, utilityStmt=0x99b059d0, isTopLevel=1 '\001', dest=0x99b05b80,
completionTag=0xbfbfd21a"") at pquery.c:1191
#14 0x0824a79e in PortalRunMulti (portal=0x99b33018, isTopLevel=4 '\004', dest=0x99b05b80, altdest=0x99b05b80,
completionTag=0xbfbfd21a"") at pquery.c:1298
#15 0x0824b21a in PortalRun (portal=0x99b33018, count=2147483647, isTopLevel=1 '\001', dest=0x99b05b80,
altdest=0x99b05b80, completionTag=0xbfbfd21a "") at pquery.c:822
#16 0x08247dc7 in exec_simple_query ( query_string=0x99b05018 "vacuum (verbose,analyze) pgfs.dirent;") at
postgres.c:1059
#17 0x08248a79 in PostgresMain (argc=2, argv=0xbb912650, username=0xbb9125c0 "takashi") at postgres.c:3943
#18 0x0820e231 in ServerLoop () at postmaster.c:3590
#19 0x0820ef88 in PostmasterMain (argc=3, argv=0xbfbfe59c) at postmaster.c:1110
#20 0x081b6439 in main (argc=3, argv=0xbfbfe59c) at main.c:199
(gdb) list
1830 offsetof(PREDICATELOCK, xactLink));
1831
1832 oldlocktag = predlock->tag;
1833 Assert(oldlocktag.myXact == sxact);
1834 oldtarget = oldlocktag.myTarget;
1835 oldtargettag = oldtarget->tag;
1836
1837 if (TargetTagIsCoveredBy(oldtargettag, *newtargettag))
1838 {
1839 uint32 oldtargettaghash;
(gdb)
On Wed, Feb 16, 2011 at 10:13:35PM +0000, YAMAMOTO Takashi wrote: > i got the following SEGV when runnning vacuum on a table. > (the line numbers in predicate.c is different as i have local modifications.) > oldlocktag.myTarget was NULL. > it seems that TransferPredicateLocksToNewTarget sometimes use stack garbage > for newpredlocktag.myTarget. vacuum on the table succeeded with the attached > patch. the latter part of the patch was necessary to avoid targetList > corruption, which later seems to make DeleteChildTargetLocks loop inifinitely. Oops. Those are both definitely bugs (and my fault). Your patch looks correct. Thanks for catching that! Dan -- Dan R. K. Ports MIT CSAIL http://drkp.net/
Dan Ports <drkp@csail.mit.edu> wrote: > Oops. Those are both definitely bugs (and my fault). Your patch > looks correct. Thanks for catching that! Could a committer please apply the slightly modified version here?: http://archives.postgresql.org/message-id/4D5C46BB020000250003AB40@gw.wicourts.gov It is a pretty straightforward bug fix to initialize some currently uninitialized data which is causing occasional but severe problems, especially during vacuum. I'm still working on the other issues raised by YAMAMOTO Takashi and Heikki. I expect to have a solution for those issues this weekend, but this bug fix is needed regardless of how those issues are settled. -Kevin
On Thu, Feb 17, 2011 at 23:11, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > Dan Ports <drkp@csail.mit.edu> wrote: > >> Oops. Those are both definitely bugs (and my fault). Your patch >> looks correct. Thanks for catching that! > > Could a committer please apply the slightly modified version here?: > > http://archives.postgresql.org/message-id/4D5C46BB020000250003AB40@gw.wicourts.gov > > It is a pretty straightforward bug fix to initialize some currently > uninitialized data which is causing occasional but severe problems, > especially during vacuum. Done, thanks. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
> Heikki Linnakangas wrote: > On 14.02.2011 20:10, Kevin Grittner wrote: >> Promotion of the lock granularity on the prior tuple is where we >> have problems. If the two tuple versions are in separate pages >> then the second UPDATE could miss the conflict. My first thought >> was to fix that by requiring promotion of a predicate lock on a >> tuple to jump straight to the relation level if nextVersionOfRow >> is set for the lock target and it points to a tuple in a different >> page. But that doesn't cover a situation where we have a heap >> tuple predicate lock which gets promoted to page granularity >> before the tuple is updated. To handle that we would need to say >> that an UPDATE to a tuple on a page which is predicate locked by >> the transaction would need to be promoted to relation granularity >> if the new version of the tuple wasn't on the same page as the old >> version. > > Yeah, promoting the original lock on the UPDATE was my first > thought too. > > Another idea is to duplicate the original predicate lock on the > first update, so that the original reader holds a lock on both row > versions. I think that would ultimately be simpler as we wouldn't > need the next-prior chains anymore. > > For example, suppose that transaction X is holding a predicate lock > on tuple A. Transaction Y updates tuple A, creating a new tuple B. > Transaction Y sees that X holds a lock on tuple A (or the page > containing A), so it acquires a new predicate lock on tuple B on > behalf of X. > > If the updater aborts, the lock on the new tuple needs to be > cleaned up, so that it doesn't get confused with later tuple that's > stored in the same physical location. We could store the xmin of > the tuple in the predicate lock to check for that. Whenever you > check for conflict, if the xmin of the lock doesn't match the xmin > on the tuple, you know that the lock belonged to an old dead tuple > stored in the same location, and can be simply removed as the tuple > doesn't exist anymore. > >> That said, the above is about eliminating false negatives from >> some corner cases which escaped notice until now. I don't think >> the changes described above will do anything to prevent the >> problems reported by YAMAMOTO Takashi. > > Agreed, it's a separate issue. Although if we change the way we > handle the read-update-update problem, the other issue might go > away too. > >> Unless I'm missing something, it sounds like tuple IDs are being >> changed or reused while predicate locks are held on the tuples. >> That's probably not going to be overwhelmingly hard to fix if we >> can identify how that can happen. I tried to cover HOT issues, but >> it seems likely I missed something. > > Storing the xmin of the original tuple would probably help with > that too. But it would be nice to understand and be able to > reproduce the issue first. I haven't been able to produce a test case yet, but I'm getting clear enough about the issue that I think I can see my way to a good fix. Even if I have a fix first, I'll continue to try to create a test to show the pre-fix failure (and post-fix success), to include in the regression suite. This is the sort of thing which is hard enough to hit that a regression could slip in because of some change and make it out in a release unnoticed if we aren't specifically testing for it. It's pretty clear that the issue is this -- we are cleaning up the predicate locks for a transaction when the transaction which read the data becomes irrelevant; but a heap tuple predicate locked by a transaction may be updated or deleted and become eligible for cleanup before the transaction become irrelevant. This can lead to cycles in the target links if a tuple ID is reused before the transaction is cleaned up. Since we may want to detect the rw-conflicts with the reading transaction based on later versions of a row after the tuple it read has been updated and the old tuple removed and its ID reused, Heikki's suggestion of predicate lock duplication in these cases goes beyond being a simple way to avoid the particular symptoms reported; it is actually necessary for correct behavior. One adjustment I'm looking at for it is that I think the tuple xmin can go in the lock target structure rather than the lock structure, because if the tuple ID has already been reused, it's pretty clear that there are no transactions which can now update the old tuple which had that ID, so any predicate locks attached to a target with the old xmin can be released and the target reused with the new xmin. This does, however, raise the question of what happens if the tuple lock has been promoted to a page lock and a tuple on that page is non-HOT updated. (Clearly, it's not an issue if the locks on a heap relation for a transaction have been promoted all the way to the relation granularity, but it's equally clear we don't want to jump to that granularity when we can avoid it.) It seems to me that in addition to what Heikki suggested, we need to create a predicate lock on the new tuple or its (new) page for every transaction holding a predicate lock on the old page whenever a tuple on a locked page is updated. We won't know whether the updated tuple is one that was read, or by which of the transactions holding page locks, but we need to act as though it was read by all of them to prevent false negatives. Covering this just at the tuple level doesn't seem adequate. It looks like these changes will be very localized and unlikely to break other things. We're talking about corner cases which haven't surfaced in the hundreds of existing SSI regression tests, and for which it is hard to actually create a reproducible test case. The HOT updates tended to expose the issues because of the more aggressive cleanup in HOT pruning. That's going to be important to exploit in creating a reproducible test case. I'm proceeding on this basis. Any comments welcome. -Kevin
"Kevin Grittner" wrote: > I'm proceeding on this basis. Result attached. I found myself passing around the tuple xmin value just about everywhere that the predicate lock target tag was being passed, so it finally dawned on me that this logically belonged as part of the target tag. That simplified the changes, and the net result of following Heikki's suggestion here is the reduction of total lines of code by 178 while adding coverage for missed corner cases and fixing bugs. Thanks again, Heikki! I will test this some more tomorrow. So far I haven't done more than ensure it passes the standard regression tests and the isolation tests added for SSI. The latter took awhile because the hash_any function was including uninitialized bytes past the length of the tag in its calculations. We should probably either fix that or document it. I had to add another couple bytes to the tag to get it to a four byte boundary to fix it. Easy once you know that's how it works... The attached patch can also be viewed here: http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=46fd5ea6728b566c521ec83048bc00a207289dd9 If this stands up to further testing, the only issue I know of for the 9.1 release is to update the documentation of shared memory usage to include the new structures. -Kevin
Вложения
On Mon, Feb 21, 2011 at 11:42:36PM +0000, YAMAMOTO Takashi wrote: > i tested ede45e90dd1992bfd3e1e61ce87bad494b81f54d + ssi-multi-update-1.patch > with my application and got the following assertion failure. > > #4 0x0827977e in CheckTargetForConflictsIn (targettag=0xbfbfce78) > at predicate.c:3657 > 3657 Assert(locallock != NULL); It looks like CheckTargetForConflictsIn is making the assumption that the backend-local lock table is accurate, which was probably even true at the time it was written. Unfortunately, it hasn't been for a while, and the new changes for tuple versions make it more likely that this will actually come up. The solution is only slightly more complicated than just removing the assertion. Unless Kevin beats me to it, I'll put together a patch later tonight or tomorrow. (I'm at the airport right now.) Dan -- Dan R. K. Ports MIT CSAIL http://drkp.net/
Dan Ports <drkp@csail.mit.edu> wrote: > It looks like CheckTargetForConflictsIn is making the assumption > that the backend-local lock table is accurate, which was probably > even true at the time it was written. I remember we decided that it could only be false in certain ways which allowed us to use it as a "lossy" first-cut test in a couple places. I doubt that we can count on any of that any longer, and should drop those heuristics. > the new changes for tuple versions make it more likely that this > will actually come up. Yeah, as far as a I can recall the only divergence was in *page* level entries for *indexes* until this latest patch. We now have *tuple* level entries for *heap* relations, too. > The solution is only slightly more complicated than just removing > the assertion. That's certainly true for that one spot, but we need an overall review of where we might be trying to use LocalPredicateLockHash for "first cut" tests as an optimization. > Unless Kevin beats me to it, I'll put together a patch later > tonight or tomorrow. (I'm at the airport right now.) It would be great if you could get this one. Thanks. -Kevin
hi, > "Kevin Grittner" wrote: > >> I'm proceeding on this basis. > > Result attached. I found myself passing around the tuple xmin value > just about everywhere that the predicate lock target tag was being > passed, so it finally dawned on me that this logically belonged as > part of the target tag. That simplified the changes, and the net > result of following Heikki's suggestion here is the reduction of > total lines of code by 178 while adding coverage for missed corner > cases and fixing bugs. > > Thanks again, Heikki! > > I will test this some more tomorrow. So far I haven't done more than > ensure it passes the standard regression tests and the isolation > tests added for SSI. The latter took awhile because the hash_any > function was including uninitialized bytes past the length of the tag > in its calculations. We should probably either fix that or document > it. I had to add another couple bytes to the tag to get it to a four > byte boundary to fix it. Easy once you know that's how it works... > > The attached patch can also be viewed here: > > http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=46fd5ea6728b566c521ec83048bc00a207289dd9 > > If this stands up to further testing, the only issue I know of for > the 9.1 release is to update the documentation of shared memory usage > to include the new structures. > > -Kevin i tested ede45e90dd1992bfd3e1e61ce87bad494b81f54d + ssi-multi-update-1.patch with my application and got the following assertion failure. YAMAMOTO Takashi Core was generated by `postgres'. Program terminated with signal 6, Aborted. #0 0xbbba4cc7 in _lwp_kill () from /usr/lib/libc.so.12 (gdb) bt #0 0xbbba4cc7 in _lwp_kill () from /usr/lib/libc.so.12 #1 0xbbba4c85 in raise (s=6) at /siro/nbsd/src/lib/libc/gen/raise.c:48 #2 0xbbba445a in abort () at /siro/nbsd/src/lib/libc/stdlib/abort.c:74 #3 0x0833d9f2 in ExceptionalCondition ( conditionName=0x8493f6d "!(locallock != ((void *)0))", errorType=0x8370451"FailedAssertion", fileName=0x8493ec3 "predicate.c", lineNumber=3657) at assert.c:57 #4 0x0827977e in CheckTargetForConflictsIn (targettag=0xbfbfce78) at predicate.c:3657 #5 0x0827bd74 in CheckForSerializableConflictIn (relation=0x99b5bad4, tuple=0xbfbfcf78, buffer=234) at predicate.c:3772 #6 0x080a5be8 in heap_update (relation=0x99b5bad4, otid=0xbfbfd038, newtup=0x99a0aa08, ctid=0xbfbfd03e, update_xmax=0xbfbfd044,cid=1, crosscheck=0x0, wait=1 '\001') at heapam.c:2593 #7 0x081c81ca in ExecModifyTable (node=0x99a0566c) at nodeModifyTable.c:624 #8 0x081b2153 in ExecProcNode (node=0x99a0566c) at execProcnode.c:371 #9 0x081b0f82 in standard_ExecutorRun (queryDesc=0x99b378f0, direction=ForwardScanDirection, count=0) at execMain.c:1247 #10 0xbbaaf352 in pgss_ExecutorRun (queryDesc=0x99b378f0, direction=ForwardScanDirection, count=0) at pg_stat_statements.c:541 #11 0xbbab5ee5 in explain_ExecutorRun (queryDesc=0x99b378f0, direction=ForwardScanDirection, count=0) at auto_explain.c:201 #12 0x08288ae3 in ProcessQuery (plan=0x99bb404c, sourceText=0x99b3781c "UPDATE file SET ctime = $1 WHERE fileid = $2", params=<value optimized out>, dest=0x84d1f38, completionTag=0xbfbfd2f0 "") at pquery.c:197 #13 0x08288d0a in PortalRunMulti (portal=0x99b3f01c, isTopLevel=<value optimized out>, dest=dwarf2_read_address: CorruptedDWARF expression. ) at pquery.c:1268 #14 0x0828984a in PortalRun (portal=0x99b3f01c, count=2147483647, isTopLevel=1 '\001', dest=0x99b07428, altdest=0x99b07428, completionTag=0xbfbfd2f0 "") at pquery.c:822 #15 0x08286c22 in PostgresMain (argc=2, argv=0xbb9196a4, username=0xbb9195f8 "takashi") at postgres.c:2004 #16 0x082413f6 in ServerLoop () at postmaster.c:3590 #17 0x082421a8 in PostmasterMain (argc=3, argv=0xbfbfe594) at postmaster.c:1110 #18 0x081e0d09 in main (argc=3, argv=0xbfbfe594) at main.c:199 (gdb) fr 4 #4 0x0827977e in CheckTargetForConflictsIn (targettag=0xbfbfce78) at predicate.c:3657 3657 Assert(locallock != NULL); (gdb) list 3652 3653 locallock = (LOCALPREDICATELOCK *) 3654 hash_search_with_hash_value(LocalPredicateLockHash, 3655 targettag,targettaghash, 3656 HASH_FIND,NULL); 3657 Assert(locallock != NULL); 3658 Assert(locallock->held); 3659 locallock->held = false; 3660 3661 if (locallock->childLocks == 0) (gdb)
On Tue, Feb 22, 2011 at 10:51:05AM -0600, Kevin Grittner wrote: > Dan Ports <drkp@csail.mit.edu> wrote: > > > It looks like CheckTargetForConflictsIn is making the assumption > > that the backend-local lock table is accurate, which was probably > > even true at the time it was written. > > I remember we decided that it could only be false in certain ways > which allowed us to use it as a "lossy" first-cut test in a couple > places. I doubt that we can count on any of that any longer, and > should drop those heuristics. Hmm. The theory was before that the local lock table would only have false negatives, i.e. if it says we hold a lock then we really do. That makes it a useful heuristic because we can bail out quickly if we're trying to re-acquire a lock we already hold. It seems worthwhile to try to preserve that. This property holds as long as the only time one backend edits another's lock list is to *add* a new lock, not remove an existing one. Fortunately, this is true most of the time (at a glance, it seems to be the case for the new tuple update code). There are two exceptions; I think they're both OK but we need to be careful here.- if we're forced to promote an index page lock's granularity to avoid running out of shared memory, we removethe fine-grained one. This is fine as long as we relax our expectations to "if the local lock table says we holda lock, then we hold that lock or one that covers it", which is acceptable for current users of the table.- if we combinetwo index pages, we remove the lock entry for the page being deleted. I think that's OK because the page is beingremoved so we will not make any efforts to lock it. > Yeah, as far as a I can recall the only divergence was in *page* > level entries for *indexes* until this latest patch. We now have > *tuple* level entries for *heap* relations, too. *nod*. I'm slightly concerned about the impact of that on granularity promotion -- the new locks created by heap updates won't get counted toward the lock promotion thresholds. That's not a fatal problem of anything, but it could make granularity promotion less effective at conserving lock entries. > > The solution is only slightly more complicated than just removing > > the assertion. > > That's certainly true for that one spot, but we need an overall > review of where we might be trying to use LocalPredicateLockHash for > "first cut" tests as an optimization. Yes, I'd planned to go through the references to LocalPredicateLockHash to make sure none of them were making any unwarranted assumptions about the results. Fortunately, there are not too many of them... Dan -- Dan R. K. Ports MIT CSAIL http://drkp.net/
Dan Ports <drkp@csail.mit.edu> wrote: > On Tue, Feb 22, 2011 at 10:51:05AM -0600, Kevin Grittner wrote: > The theory was before that the local lock table would only have > false negatives, i.e. if it says we hold a lock then we really do. > That makes it a useful heuristic because we can bail out quickly > if we're trying to re-acquire a lock we already hold. It seems > worthwhile to try to preserve that. > > This property holds as long as the only time one backend edits > another's lock list is to *add* a new lock, not remove an existing > one. Fortunately, this is true most of the time (at a glance, it > seems to be the case for the new tuple update code). > > There are two exceptions; I think they're both OK but we need to > be careful here. > - if we're forced to promote an index page lock's granularity to > avoid running out of shared memory, we remove the fine-grained > one. This is fine as long as we relax our expectations to "if > the local lock table says we hold a lock, then we hold that > lock or one that covers it", which is acceptable for current > users of the table. That makes sense. This one sounds OK. > - if we combine two index pages, we remove the lock entry for the > page being deleted. I think that's OK because the page is being > removed so we will not make any efforts to lock it. I'm not sure it's safe to assume that the index page won't get reused before the local lock information is cleared. In the absence of a clear proof that it is safe, or some enforcement mechanism to ensure that it is, I don't think we should make this assumption. Off-hand I can't think of a clever way to make this safe which would cost less than taking out the LW lock and checking the definitive shared memory HTAB, but that might be for lack of creative thinking at the moment.. >> Yeah, as far as a I can recall the only divergence was in *page* >> level entries for *indexes* until this latest patch. We now have >> *tuple* level entries for *heap* relations, too. > > *nod*. I'm slightly concerned about the impact of that on > granularity promotion -- the new locks created by heap updates > won't get counted toward the lock promotion thresholds. That's not > a fatal problem of anything, but it could make granularity > promotion less effective at conserving lock entries. This pattern doesn't seem to come up very often in most workloads. Since it's feeding into a heuristic which already triggers pretty quickly I think we should be OK with this. It makes me less tempted to tinker with the threshold for promoting tuple locks to page locks, though. The only alternative I see would be to use some form of asynchronous notification of the new locks so that the local table can be maintained. That seems overkill without some clear evidence that it is needed. I *really* wouldn't want to go back to needing LW locks to maintain this info; as you know (and stated only for the benefit of the list), it was a pretty serious contention point in early profiling and adding the local table was a big part of getting an early benchmark down from a 14+% performance hit for SSI to a 1.8% performance hit. -Kevin
On Tue, Feb 22, 2011 at 05:54:49PM -0600, Kevin Grittner wrote: > I'm not sure it's safe to assume that the index page won't get > reused before the local lock information is cleared. In the absence > of a clear proof that it is safe, or some enforcement mechanism to > ensure that it is, I don't think we should make this assumption. > Off-hand I can't think of a clever way to make this safe which would > cost less than taking out the LW lock and checking the definitive > shared memory HTAB, but that might be for lack of creative thinking > at the moment.. Hmm. Yeah, I wasn't sure about that one, and having now thought about it some more I think it isn't safe -- consider adding a lock on an index page concurrently with another backend merging that page into another one. The obvious solution to me is to just keep the lock on both the old and new page. The downside is that because this requires allocating a new lock and is in a context where we're not allowed to fail, we'll need to fall back on acquiring the relation lock just as we do for page splits. I was going to bemoan the extra complexity this would add -- but actually, couldn't we just replace PredicateLockPageCombine with a call to PredicateLockPageSplit since they'd now do the same thing? > The only alternative I see would be to use some form of asynchronous > notification of the new locks so that the local table can be > maintained. That seems overkill without some clear evidence that it > is needed. I agree. It is certainly weird and undesirable that the backend-local lock table is not always accurate, but I don't see a good way to keep it up to date without the cure being worse than the disease. > I *really* wouldn't want to go back to needing LW locks > to maintain this info; as you know (and stated only for the benefit > of the list), it was a pretty serious contention point in early > profiling and adding the local table was a big part of getting an > early benchmark down from a 14+% performance hit for SSI to a 1.8% > performance hit. Yes, it's definitely important for a backend to be able to check whether it's already holding a lock (even if that's just a hint) without having to take locks. Let me add one more piece of info for the benefit of the list: a backend's local lock table contains not just locks held by the backend, but also an entry and refcount for every parent of a lock it holds. This is used to determine when to promote to one of the coarser-grained parent locks. It's both unnecessary and undesirable for that info to be in shared memory. Dan -- Dan R. K. Ports MIT CSAIL http://drkp.net/
Dan Ports wrote: > The obvious solution to me is to just keep the lock on both the old > and new page. That's the creative thinking I was failing to do. Keeping the old lock will generate some false positives, but it will be rare and those don't compromise correctness -- they just carry the cost of starting the transaction over. In exchange you buy a savings in predicate lock acquisition. In particular, by dodging LW locking for a large percentage of calls, you help scalability. It can't cause false negatives, so correctness is OK -- it's all about net costs. > I was going to bemoan the extra complexity this would add -- but > actually, couldn't we just replace PredicateLockPageCombine with a > call to PredicateLockPageSplit since they'd now do the same thing? I'd be inclined to leave the external interface alone, in case we conceive of an even better implementation. We can just remove the removeOld bool from the parameter list of the static TransferPredicateLocksToNewTarget function, and keep the behavior from the "false" case. -Kevin
On 23.02.2011 07:20, Kevin Grittner wrote: > Dan Ports wrote: > >> The obvious solution to me is to just keep the lock on both the old >> and new page. > > That's the creative thinking I was failing to do. Keeping the old > lock will generate some false positives, but it will be rare and > those don't compromise correctness -- they just carry the cost of > starting the transaction over. Sounds reasonable, but let me throw in another idea while we're at it: if there's a lock on the index page we're about to delete, we could just choose to not delete it. The next vacuum will pick it up. Presumably it will happen rarely, so index bloat won't be an issue. >> I was going to bemoan the extra complexity this would add -- but >> actually, couldn't we just replace PredicateLockPageCombine with a >> call to PredicateLockPageSplit since they'd now do the same thing? > > I'd be inclined to leave the external interface alone, in case we > conceive of an even better implementation. Agreed. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > On 23.02.2011 07:20, Kevin Grittner wrote: >> Dan Ports wrote: >> >>> The obvious solution to me is to just keep the lock on both the >>> old and new page. >> >> That's the creative thinking I was failing to do. Keeping the >> old lock will generate some false positives, but it will be rare >> and those don't compromise correctness -- they just carry the >> cost of starting the transaction over. > > Sounds reasonable, but let me throw in another idea while we're at > it: if there's a lock on the index page we're about to delete, we > could just choose to not delete it. The next vacuum will pick it > up. Presumably it will happen rarely, so index bloat won't be an > issue. Yeah, that's probably better. -Kevin
An updated patch to address this issue is attached. It fixes a couple
issues related to use of the backend-local lock table hint:
- CheckSingleTargetForConflictsIn now correctly handles the case
where a lock that's being held is not reflected in the local lock
table. This fixes the assertion failure reported in this thread.
- PredicateLockPageCombine now retains locks for the page that is
being removed, rather than removing them. This prevents a
potentially dangerous false-positive inconsistency where the local
lock table believes that a lock is held, but it is actually not.
- add some more comments documenting the times when the local lock
table can be inconsistent with reality, as reflected in the shared
memory table.
This patch also incorporates Kevin's changes to copy locks when
creating a new version of a tuple rather than trying to maintain a
linkage between different versions. So this is a patch that should
apply against HEAD and addresses all outstanding SSI bugs known to
Kevin or myself.
Besides the usual regression and isolation tests, I have tested this
by running DBT-2 on a 16-core machine to verify that there are no
assertion failures that only show up under concurrent access.
Dan
--
Dan R. K. Ports MIT CSAIL http://drkp.net/
Вложения
On 01.03.2011 02:03, Dan Ports wrote: > An updated patch to address this issue is attached. It fixes a couple > issues related to use of the backend-local lock table hint: > > - CheckSingleTargetForConflictsIn now correctly handles the case > where a lock that's being held is not reflected in the local lock > table. This fixes the assertion failure reported in this thread. > > - PredicateLockPageCombine now retains locks for the page that is > being removed, rather than removing them. This prevents a > potentially dangerous false-positive inconsistency where the local > lock table believes that a lock is held, but it is actually not. > > - add some more comments documenting the times when the local lock > table can be inconsistent with reality, as reflected in the shared > memory table. > > This patch also incorporates Kevin's changes to copy locks when > creating a new version of a tuple rather than trying to maintain a > linkage between different versions. So this is a patch that should > apply against HEAD and addresses all outstanding SSI bugs known to > Kevin or myself. Thanks, committed with minor changes. The ordering of the fields in PREDICATELOCKTAG was bizarre, so I just expanded the offsetnumber fields to an uint32, instead of having the padding field. I think that's a lot more readable. I also added an optimization in PredicateLockTupleRowVersionLink() to not try to transfer the page locks, if the new tuple is on the same page as the old one. That's very cheap to check, and it's very common for an update to stay within a page. Was there test cases for any of the issues fixed by this patch that we should add to the suite? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > committed with minor changes. Thanks! > The ordering of the fields in PREDICATELOCKTAG was bizarre, so I > just expanded the offsetnumber fields to an uint32, instead of > having the padding field. I think that's a lot more readable. I can understand that, but I wonder if we shouldn't have a comment mentioning that the offsetnumber field is larger than needed, in case someone later needs to add another 16 bit field for some reason, or we go to a hash function without the same quirks. > I also added an optimization in PredicateLockTupleRowVersionLink() > to not try to transfer the page locks, if the new tuple is on the > same page as the old one. That's very cheap to check, and it's > very common for an update to stay within a page. Thanks. I had it in mind to do that, but lost track of it. Definitely worth doing. > Was there test cases for any of the issues fixed by this patch > that we should add to the suite? No, but I'm still intending to look at that some more. It makes me nervous to have an area which would be pretty easy for someone to break without any tests to catch such breakage. -Kevin
On Tue, Mar 01, 2011 at 07:07:42PM +0200, Heikki Linnakangas wrote: > Was there test cases for any of the issues fixed by this patch that we > should add to the suite? Some of these issues are tricky to test, e.g. some of the code about transferring predicate locks to a new target doesn't get exercised unless an index page gets split while there are concurrent transactions holding locks on that page. I have not been able to find a good way to test these other than system-level testing using a concurrent workload (usually the DBT-2 benchmark). I'd certainly be open to suggestions! Dan -- Dan R. K. Ports MIT CSAIL http://drkp.net/
hi, thanks for quickly fixing problems. i tested the later version (a2eb9e0c08ee73208b5419f5a53a6eba55809b92) and only errors i got was "out of shared memory". i'm not sure if it was caused by SSI activities or not. YAMAMOTO Takashi the following is a snippet from my application log: PG_DIAG_SEVERITY: WARNING PG_DIAG_SQLSTATE: 53200 PG_DIAG_MESSAGE_PRIMARY: out of shared memory PG_DIAG_SOURCE_FILE: shmem.c PG_DIAG_SOURCE_LINE: 190 PG_DIAG_SOURCE_FUNCTION: ShmemAlloc PG_DIAG_SEVERITY: ERROR PG_DIAG_SQLSTATE: 53200 PG_DIAG_MESSAGE_PRIMARY: out of shared memory PG_DIAG_SOURCE_FILE: dynahash.c PG_DIAG_SOURCE_LINE: 925 PG_DIAG_SOURCE_FUNCTION: hash_search_with_hash_value PG_DIAG_SEVERITY: WARNING PG_DIAG_SQLSTATE: 53200 PG_DIAG_MESSAGE_PRIMARY: out of shared memory PG_DIAG_SOURCE_FILE: shmem.c PG_DIAG_SOURCE_LINE: 190 PG_DIAG_SOURCE_FUNCTION: ShmemAlloc PG_DIAG_SEVERITY: ERROR PG_DIAG_SQLSTATE: 53200 PG_DIAG_MESSAGE_PRIMARY: out of shared memory PG_DIAG_SOURCE_FILE: dynahash.c PG_DIAG_SOURCE_LINE: 925 PG_DIAG_SOURCE_FUNCTION: hash_search_with_hash_value PG_DIAG_SEVERITY: WARNING PG_DIAG_SQLSTATE: 53200 PG_DIAG_MESSAGE_PRIMARY: out of shared memory PG_DIAG_SOURCE_FILE: shmem.c PG_DIAG_SOURCE_LINE: 190 PG_DIAG_SOURCE_FUNCTION: ShmemAlloc PG_DIAG_SEVERITY: ERROR PG_DIAG_SQLSTATE: 53200 PG_DIAG_MESSAGE_PRIMARY: out of shared memory PG_DIAG_SOURCE_FILE: dynahash.c PG_DIAG_SOURCE_LINE: 925 PG_DIAG_SOURCE_FUNCTION: hash_search_with_hash_value
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> wrote: > thanks for quickly fixing problems. Thanks for the rigorous testing. :-) > i tested the later version > (a2eb9e0c08ee73208b5419f5a53a6eba55809b92) and only errors i got > was "out of shared memory". i'm not sure if it was caused by SSI > activities or not. > PG_DIAG_SEVERITY: WARNING > PG_DIAG_SQLSTATE: 53200 > PG_DIAG_MESSAGE_PRIMARY: out of shared memory > PG_DIAG_SOURCE_FILE: shmem.c > PG_DIAG_SOURCE_LINE: 190 > PG_DIAG_SOURCE_FUNCTION: ShmemAlloc > > PG_DIAG_SEVERITY: ERROR > PG_DIAG_SQLSTATE: 53200 > PG_DIAG_MESSAGE_PRIMARY: out of shared memory > PG_DIAG_SOURCE_FILE: dynahash.c > PG_DIAG_SOURCE_LINE: 925 > PG_DIAG_SOURCE_FUNCTION: hash_search_with_hash_value Nor am I. Some additional information would help. (1) Could you post the non-default configuration settings? (2) How many connections are in use in your testing? (3) Can you give a rough categorization of how many of what types of transactions are in the mix? (4) Are there any long-running transactions? (5) How many of these errors do you get in what amount of time? (6) Does the application continue to run relatively sanely, or does it fall over at this point? (7) The message hint would help pin it down, or a stack trace at the point of the error would help more. Is it possible to get either? Looking over the code, it appears that the only places that SSI could generate that error, it would cancel that transaction with the hint "You might need to increase max_pred_locks_per_transaction." and otherwise allow normal processing. Even with the above information it may be far from clear where allocations are going past their maximum, since one HTAB could grab more than its share and starve another which is staying below its "maximum". I'll take a look at the possibility of adding a warning or some such when an HTAB expands past its maximum size. -Kevin
It would probably also be worth monitoring the size of pg_locks to see how many predicate locks are being held. On Fri, Mar 18, 2011 at 12:50:16PM -0500, Kevin Grittner wrote: > Even with the above information it may be far from clear where > allocations are going past their maximum, since one HTAB could grab > more than its share and starve another which is staying below its > "maximum". I'll take a look at the possibility of adding a warning > or some such when an HTAB expands past its maximum size. Yes -- considering how few shared memory HTABs have sizes that are really dynamic, I'd be inclined to take a close look at SSI and max_predicate_locks_per_transaction regardless of where the failed allocation took place. But I am surprised to see that error message without SSI's hint about increasing max_predicate_locks_per_xact. Dan -- Dan R. K. Ports MIT CSAIL http://drkp.net/
Dan Ports <drkp@csail.mit.edu> wrote: > I am surprised to see that error message without SSI's hint about > increasing max_predicate_locks_per_xact. After reviewing this, I think something along the following lines might be needed, for a start. I'm not sure the Asserts are actually needed; they basically are checking that the current behavior of hash_search doesn't change. I'm still looking at whether it's sane to try to issue a warning when an HTAB exceeds the number of entries declared as its max_size when it was created. -Kevin
Вложения
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote: > I'm still looking at whether it's sane to try to issue a warning > when an HTAB exceeds the number of entries declared as its > max_size when it was created. I think this does it. If nothing else, it might be instructive to use it while testing the SSI patch. Would it make any sense to slip this into 9.1, or should I add it to the first 9.2 CF? -Kevin
Вложения
hi, > YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> wrote: > >> thanks for quickly fixing problems. > > Thanks for the rigorous testing. :-) > >> i tested the later version >> (a2eb9e0c08ee73208b5419f5a53a6eba55809b92) and only errors i got >> was "out of shared memory". i'm not sure if it was caused by SSI >> activities or not. > >> PG_DIAG_SEVERITY: WARNING >> PG_DIAG_SQLSTATE: 53200 >> PG_DIAG_MESSAGE_PRIMARY: out of shared memory >> PG_DIAG_SOURCE_FILE: shmem.c >> PG_DIAG_SOURCE_LINE: 190 >> PG_DIAG_SOURCE_FUNCTION: ShmemAlloc >> >> PG_DIAG_SEVERITY: ERROR >> PG_DIAG_SQLSTATE: 53200 >> PG_DIAG_MESSAGE_PRIMARY: out of shared memory >> PG_DIAG_SOURCE_FILE: dynahash.c >> PG_DIAG_SOURCE_LINE: 925 >> PG_DIAG_SOURCE_FUNCTION: hash_search_with_hash_value > > Nor am I. Some additional information would help. > > (1) Could you post the non-default configuration settings? none. it can happen with just initdb+createdb'ed database. > (2) How many connections are in use in your testing? 4. > (3) Can you give a rough categorization of how many of what types > of transactions are in the mix? all transactions are SERIALIZABLE. no transactions are with READ ONLY. (but some of them are actually select-only.) > (4) Are there any long-running transactions? no. > (5) How many of these errors do you get in what amount of time? once it start happening, i see them somehow frequently. > (6) Does the application continue to run relatively sanely, or does > it fall over at this point? my application just exits on the error. if i re-run the application without rebooting postgres, it seems that i will get the error sooner than the first run. (but it might be just a matter of luck) > (7) The message hint would help pin it down, or a stack trace at > the point of the error would help more. Is it possible to get > either? Looking over the code, it appears that the only places that > SSI could generate that error, it would cancel that transaction with > the hint "You might need to increase > max_pred_locks_per_transaction." and otherwise allow normal > processing. no message hints. these errors are not generated by SSI code, at least directly. (please look at PG_DIAG_SOURCE_xxx in the above log.) YAMAMOTO Takashi > Even with the above information it may be far from clear where > allocations are going past their maximum, since one HTAB could grab > more than its share and starve another which is staying below its > "maximum". I'll take a look at the possibility of adding a warning > or some such when an HTAB expands past its maximum size. > > -Kevin
On Fri, Mar 18, 2011 at 5:57 PM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > "Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote: > >> I'm still looking at whether it's sane to try to issue a warning >> when an HTAB exceeds the number of entries declared as its >> max_size when it was created. > > I think this does it. > > If nothing else, it might be instructive to use it while testing the > SSI patch. Would it make any sense to slip this into 9.1, or should > I add it to the first 9.2 CF? I don't think it's too late to commit something like this, but I'm not clear on whether we want it. Is this checking for what should be a can't-happen case, or are these soft limits that we expect to be exceeded from time to time? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Mar 18, 2011 at 4:51 PM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > Dan Ports <drkp@csail.mit.edu> wrote: > >> I am surprised to see that error message without SSI's hint about >> increasing max_predicate_locks_per_xact. > > After reviewing this, I think something along the following lines > might be needed, for a start. I'm not sure the Asserts are actually > needed; they basically are checking that the current behavior of > hash_search doesn't change. > > I'm still looking at whether it's sane to try to issue a warning > when an HTAB exceeds the number of entries declared as its max_size > when it was created. I don't see much advantage in changing these to asserts - in a debug build, that will promote ERROR to PANIC; whereas in a production build, they'll cause a random failure somewhere downstream. The HASH_ENTER to HASH_ENTER_NULL changes look like they might be needed, though. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Mar 18, 2011 at 5:57 PM, Kevin Grittner
> <Kevin.Grittner@wicourts.gov> wrote:
>>> I'm still looking at whether it's sane to try to issue a warning
>>> when an HTAB exceeds the number of entries declared as its
>>> max_size when it was created.
> I don't think it's too late to commit something like this, but I'm not
> clear on whether we want it.
We do *not* want that.
Up to now, I believe the lockmgr's lock table is the only shared hash
table that is expected to grow past the declared size; that can happen
anytime a session exceeds max_locks_per_transaction, which we consider
to be only a soft limit. I don't know whether SSI has introduced any
other hash tables that behave similarly. (If it has, we might want to
rethink the amount of "slop" space we leave in shmem...)
There might perhaps be some value in adding a warning like this if it
were enabled per-table (and not enabled by default). But I'd want to
see a specific reason for it, not just "let's see if we can scare users
with a WARNING appearing out of nowhere".
regards, tom lane
On Fri, Mar 25, 2011 at 4:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Fri, Mar 18, 2011 at 5:57 PM, Kevin Grittner >> <Kevin.Grittner@wicourts.gov> wrote: >>>> I'm still looking at whether it's sane to try to issue a warning >>>> when an HTAB exceeds the number of entries declared as its >>>> max_size when it was created. > >> I don't think it's too late to commit something like this, but I'm not >> clear on whether we want it. > > We do *not* want that. > > Up to now, I believe the lockmgr's lock table is the only shared hash > table that is expected to grow past the declared size; that can happen > anytime a session exceeds max_locks_per_transaction, which we consider > to be only a soft limit. I don't know whether SSI has introduced any > other hash tables that behave similarly. (If it has, we might want to > rethink the amount of "slop" space we leave in shmem...) > > There might perhaps be some value in adding a warning like this if it > were enabled per-table (and not enabled by default). But I'd want to > see a specific reason for it, not just "let's see if we can scare users > with a WARNING appearing out of nowhere". What about a system view that shows declared & actual sizes of all these hash tables? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Mar 25, 2011 at 04:06:30PM -0400, Tom Lane wrote: > Up to now, I believe the lockmgr's lock table is the only shared hash > table that is expected to grow past the declared size; that can happen > anytime a session exceeds max_locks_per_transaction, which we consider > to be only a soft limit. I don't know whether SSI has introduced any > other hash tables that behave similarly. (If it has, we might want to > rethink the amount of "slop" space we leave in shmem...) I looked into this recently and the two lockmgr tables were indeed the only ones that could vary in size. IIRC, the only other shared hash tables were the shared buffer index and the shmem index. SSI adds two more analogous tables (per-lock-target and per-xact-lock), both of which are sized according to max_pred_locks_per_transaction, which is also a soft limit. It also adds a per-transaction shared hash table, but that has a clear maximum size. I find the soft limit on htab size a strange model, and I suspect it might be a source of problems now that we've got more than one table that can actually exceed it its limit. (Particularly so given that once shmem gets allocated to one htab, there's no getting it back.) Dan -- Dan R. K. Ports MIT CSAIL http://drkp.net/
Robert Haas wrote:
> I don't see much advantage in changing these to asserts - in a
> debug build, that will promote ERROR to PANIC; whereas in a
> production build, they'll cause a random failure somewhere
> downstream.
The reason Assert is appropriate is that it is *impossible* to hit
that condition right now for two independent reasons. Both would
need to be broken by someone doing clumsy programming changes for
these assertions to fire.
(1) Both assertions follow the attempt to get record from the
structure allocated with this call:
SerializableXidHash = ShmemInitHash("SERIALIZABLEXID hash", max_table_size,
max_table_size, &info,
hash_flags);
Note that it is allocated at its maximum size, and that one of these
records can't exist without a corresponding SERIALIZABLEXACT
structure, and these are allocated out of a fixed-sized area which is
allocated based on the same max_table_size value.
(2) The preceding call to hash_search is made with a HASHACTION of
HASH_ENTER, not HASH_ENTER_NULL. So it can't return without a valid
pointer. It will ereport at the ERROR level rather than do that.
So I'm not sure the Assert is even warranted, versus just doing
nothing; but the ereport is certainly pointless.
> The HASH_ENTER to HASH_ENTER_NULL changes look like they might be
> needed, though.
That would provide a hint pointing toward the GUC which controls
allocation space for the structure we failed to get. That seems
better than the generic message we're now getting. If we don't
change them to HASH_ENTER_NULL we can get rid of our ereport since it
will never be hit.
Of course I'm sure Tom will clean such stuff up if it comes to him in
the current form; it just seems nicer to clean up known issues that
surface from testing before it gets to him, where possible.
-Kevin
Tom Lane wrote: > There might perhaps be some value in adding a warning like this if > it were enabled per-table (and not enabled by default). It only fires where a maximum has been declared and is exceeded. Most HTABs don't declare a maximum -- they leave it at zero. These are ignored. Where this fires on a table in shared memory, we're into a situation where the over-allocation to one table may cause failure in an unrelated area. If we're not going to change that, some indication of which one actually exceeded its limits seems like a helpful bit of diagnostic information. > But I'd want to see a specific reason for it, not just "let's see > if we can scare users with a WARNING appearing out of nowhere". Perhaps LOG would be more appropriate than WARNING? -Kevin
YAMAMOTO Takashi wrote: > Kevin Grittner wrote: >> (1) Could you post the non-default configuration settings? > > none. it can happen with just initdb+createdb'ed database. > >> (2) How many connections are in use in your testing? > > 4. > >> (3) Can you give a rough categorization of how many of what types >> of transactions are in the mix? > > all transactions are SERIALIZABLE. > > no transactions are with READ ONLY. > (but some of them are actually select-only.) > >> (4) Are there any long-running transactions? > > no. > >> (5) How many of these errors do you get in what amount of time? > > once it start happening, i see them somehow frequently. > >> (6) Does the application continue to run relatively sanely, or >> does it fall over at this point? > > my application just exits on the error. > > if i re-run the application without rebooting postgres, it seems > that i will get the error sooner than the first run. (but it might > be just a matter of luck) If your application hits this again, could you check pg_stat_activity and pg_locks and see if any SIReadLock entries are lingering after the owning transaction and all overlapping transactions are completed? If anything is lingering between runs of your application, it *should* show up in one or the other of these. >> (7) The message hint would help pin it down, or a stack trace at >> the point of the error would help more. Is it possible to get >> either? Looking over the code, it appears that the only places >> that SSI could generate that error, it would cancel that >> transaction with the hint "You might need to increase >> max_pred_locks_per_transaction." and otherwise allow normal >> processing. > > no message hints. these errors are not generated by SSI code, > at least directly. Right, that's because we were using HASH_ENTER instead of HASH_ENTER_NULL. I've posted a patch which should correct that. >> Even with the above information it may be far from clear where >> allocations are going past their maximum, since one HTAB could >> grab more than its share and starve another which is staying below >> its "maximum". I'll take a look at the possibility of adding a >> warning or some such when an HTAB expands past its maximum size. I see from your later post that you are running with this patch. Has that reported anything yet? Thanks, -Kevin
YAMAMOTO Takashi wrote: > this psql session was the only activity to the server at this > point. > [no residual SIReadLock] >> Right, that's because we were using HASH_ENTER instead of >> HASH_ENTER_NULL. I've posted a patch which should correct that. > sure, with your patch it seems that they turned into different > ones. > PG_DIAG_SEVERITY: ERROR > PG_DIAG_SQLSTATE: 53200 > PG_DIAG_MESSAGE_PRIMARY: out of shared memory > PG_DIAG_MESSAGE_HINT: You might need to increase > max_pred_locks_per_transaction. > PG_DIAG_SOURCE_FILE: predicate.c > PG_DIAG_SOURCE_LINE: 2049 > PG_DIAG_SOURCE_FUNCTION: CreatePredicateLock >>> Even with the above information it may be far from clear where >>> allocations are going past their maximum, since one HTAB could >>> grab more than its share and starve another which is staying >>> below its "maximum". I'll take a look at the possibility of >>> adding a warning or some such when an HTAB expands past its >>> maximum size. >> >> I see from your later post that you are running with this patch. >> Has that reported anything yet? > > i got nothing except the following one. (in the server log) > > WARNING: hash table "ShmemIndex" has more entries than expected > DETAIL: The maximum was set to 32 on creation. That doesn't seem likely to be causing the problem, but maybe the allocations for that should be bumped a bit. These results suggest that there is some sort of a leak in the cleanup of the PredicateLockTargetHash HTAB entries. Will look into it. Thanks again. -Kevin
hi, >>> (6) Does the application continue to run relatively sanely, or >>> does it fall over at this point? >> >> my application just exits on the error. >> >> if i re-run the application without rebooting postgres, it seems >> that i will get the error sooner than the first run. (but it might >> be just a matter of luck) > > If your application hits this again, could you check pg_stat_activity > and pg_locks and see if any SIReadLock entries are lingering after > the owning transaction and all overlapping transactions are > completed? If anything is lingering between runs of your > application, it *should* show up in one or the other of these. this is 71ac48fd9cebd3d2a873635a04df64096c981f73 with your two patches. this psql session was the only activity to the server at this point. hoge=# select * from pg_stat_activity; -[ RECORD 1 ]----+-------------------------------- datid | 16384 datname | hoge procpid | 7336 usesysid | 10 usename | takashi application_name | psql client_addr | client_hostname | client_port | -1 backend_start | 2011-03-26 12:28:21.882226+09 xact_start | 2011-03-28 11:55:19.300027+09 query_start | 2011-03-28 11:55:19.300027+09 waiting | f current_query | select * from pg_stat_activity; hoge=# select count(*) from pg_locks where mode='SIReadLock'; -[ RECORD 1 ] count | 7057 hoge=# select locktype,count(*) from pg_locks group by locktype; -[ RECORD 1 ]-------- locktype | virtualxid count | 1 -[ RECORD 2 ]-------- locktype | relation count | 1 -[ RECORD 3 ]-------- locktype | tuple count | 7061 hoge=# > >>> (7) The message hint would help pin it down, or a stack trace at >>> the point of the error would help more. Is it possible to get >>> either? Looking over the code, it appears that the only places >>> that SSI could generate that error, it would cancel that >>> transaction with the hint "You might need to increase >>> max_pred_locks_per_transaction." and otherwise allow normal >>> processing. >> >> no message hints. these errors are not generated by SSI code, >> at least directly. > > Right, that's because we were using HASH_ENTER instead of > HASH_ENTER_NULL. I've posted a patch which should correct that. sure, with your patch it seems that they turned into different ones. PG_DIAG_SEVERITY: WARNING PG_DIAG_SQLSTATE: 53200 PG_DIAG_MESSAGE_PRIMARY: out of shared memory PG_DIAG_SOURCE_FILE: shmem.c PG_DIAG_SOURCE_LINE: 190 PG_DIAG_SOURCE_FUNCTION: ShmemAlloc PG_DIAG_SEVERITY: ERROR PG_DIAG_SQLSTATE: 53200 PG_DIAG_MESSAGE_PRIMARY: out of shared memory PG_DIAG_MESSAGE_HINT: You might need to increase max_pred_locks_per_transaction. PG_DIAG_SOURCE_FILE: predicate.c PG_DIAG_SOURCE_LINE: 2049 PG_DIAG_SOURCE_FUNCTION: CreatePredicateLock >>> Even with the above information it may be far from clear where >>> allocations are going past their maximum, since one HTAB could >>> grab more than its share and starve another which is staying below >>> its "maximum". I'll take a look at the possibility of adding a >>> warning or some such when an HTAB expands past its maximum size. > > I see from your later post that you are running with this patch. Has > that reported anything yet? i got nothing except the following one. (in the server log) WARNING: hash table "ShmemIndex" has more entries than expected DETAIL: The maximum was set to 32 on creation. YAMAMOTO Takashi > > Thanks, > > -Kevin
hi, >> [no residual SIReadLock] i read it as there are many (7057) SIReadLocks somehow leaked. am i wrong? YAMAMOTO Takashi
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> wrote: >>> [no residual SIReadLock] > > i read it as there are many (7057) SIReadLocks somehow leaked. > am i wrong? No, I am. Could you send the full SELECT * of pg_locks when this is manifest? (Probably best to do that off-list.) -Kevin
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> wrote: > hoge=# select locktype,count(*) from pg_locks group by locktype; > -[ RECORD 1 ]-------- > locktype | virtualxid > count | 1 > -[ RECORD 2 ]-------- > locktype | relation > count | 1 > -[ RECORD 3 ]-------- > locktype | tuple > count | 7061 I've stared at the code for hours and have only come up with one race condition which can cause this, although the window is so small it's hard to believe that you would get this volume of orphaned locks. I'll keep looking, but if you could try this to see if it has a material impact, that would be great. I am very sure this patch is needed and that it is safe. It moves a LWLockAcquire statement up to cover the setup for the loop that it already covers. It also includes a fix to a comment that got missed when we switched from the pointer between lock targets to duplicating the locks. -Kevin
Вложения
On 31.03.2011 16:31, Kevin Grittner wrote: > I've stared at the code for hours and have only come up with one > race condition which can cause this, although the window is so small > it's hard to believe that you would get this volume of orphaned > locks. I'll keep looking, but if you could try this to see if it > has a material impact, that would be great. > > I am very sure this patch is needed and that it is safe. It moves a > LWLockAcquire statement up to cover the setup for the loop that it > already covers. It also includes a fix to a comment that got missed > when we switched from the pointer between lock targets to > duplicating the locks. Ok, committed. Did we get anywhere with the sizing of the various shared memory structures? Did we find the cause of the "out of shared memory" warnings? Would it help if we just pre-allocated all the shared memory hash tables and didn't allow them to grow? It's bizarre that the hash table that requests the slack shared memory space first gets it, and it can't be reused for anything else without a server restart. I feel it would make better to not allow the tables to grow, so that you get consistent behavior across restarts. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > Did we get anywhere with the sizing of the various shared memory > structures? Did we find the cause of the "out of shared memory" > warnings? The patch you just committed is related to that. Some tuple locks for summarized transactions were not getting cleaned up. I found one access to the list not protected by the appropriate LW lock, which is what this patch fixed. I'm not satisfied that was the only issue, though; I'm still looking. > Would it help if we just pre-allocated all the shared memory hash > tables and didn't allow them to grow? I've been thinking that it might be wise. > It's bizarre that the hash table that requests the slack shared > memory space first gets it, and it can't be reused for anything > else without a server restart. I feel it would make better to not > allow the tables to grow, so that you get consistent behavior > across restarts. Agreed. I think it was OK in prior releases because there was just one HTAB in shared memory doing this. With multiple such tables, it doesn't seem sane to allow unbounded lazy grabbing of the space this way. The only thing I've been on the fence about is whether it makes more sense to allocate it all up front or to continue to allow incremental allocation but set a hard limit on the number of entries allocated for each shared memory HTAB. Is there a performance- related reason to choose one path or the other? -Kevin
On Thu, Mar 31, 2011 at 11:06:30AM -0500, Kevin Grittner wrote: > The only thing I've been on the fence about is whether it > makes more sense to allocate it all up front or to continue to allow > incremental allocation but set a hard limit on the number of entries > allocated for each shared memory HTAB. Is there a performance- > related reason to choose one path or the other? Seems like it would be marginally better to allocate it up front -- then you don't have the cost of having to split buckets later as it grows. Dan -- Dan R. K. Ports MIT CSAIL http://drkp.net/
Dan Ports <drkp@csail.mit.edu> wrote: > On Thu, Mar 31, 2011 at 11:06:30AM -0500, Kevin Grittner wrote: >> The only thing I've been on the fence about is whether it >> makes more sense to allocate it all up front or to continue to allow >> incremental allocation but set a hard limit on the number of entries >> allocated for each shared memory HTAB. Is there a performance- >> related reason to choose one path or the other? > > Seems like it would be marginally better to allocate it up front -- then > you don't have the cost of having to split buckets later as it grows. The attached patch should cover that. -Kevin
Вложения
On 31.03.2011 21:23, Kevin Grittner wrote: > Dan Ports<drkp@csail.mit.edu> wrote: >> On Thu, Mar 31, 2011 at 11:06:30AM -0500, Kevin Grittner wrote: >>> The only thing I've been on the fence about is whether it >>> makes more sense to allocate it all up front or to continue to > allow >>> incremental allocation but set a hard limit on the number of > entries >>> allocated for each shared memory HTAB. Is there a performance- >>> related reason to choose one path or the other? >> >> Seems like it would be marginally better to allocate it up front -- > then >> you don't have the cost of having to split buckets later as it > grows. > > The attached patch should cover that. That's not enough. The hash tables can grow beyond the maximum size you specify in ShmemInitHash. It's just a hint to size the directory within the hash table. We'll need to teach dynahash not to allocate any more entries after the preallocation. A new HASH_NO_GROW flag to hash_create() seems like a suitable interface. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > That's not enough. The hash tables can grow beyond the maximum > size you specify in ShmemInitHash. It's just a hint to size the > directory within the hash table. > > We'll need to teach dynahash not to allocate any more entries > after the preallocation. A new HASH_NO_GROW flag to hash_create() > seems like a suitable interface. OK. If we're doing that, is it worth taking a look at the "safety margin" added to the size calculations, and try to make the calculations more accurate? Would you like me to code a patch for this? There are a couple other patches which I think should be applied, if you have time to deal with them. There was a fix for an assertion failure here: http://archives.postgresql.org/pgsql-bugs/2011-03/msg00352.php It just rechecks some conditions after dropping a shared LW lock and acquiring an exclusive LW lock. Without this recheck there is a window for the other transaction involved in the conflict to also detect a conflict and flag first, leading to the assertion. There's another area I need to review near there, but that is orthogonal. There is a patch to improve out-of-shared-memory error handling and reporting here: http://archives.postgresql.org/pgsql-hackers/2011-03/msg01170.php This one is due to my earlier failure to spot the difference between HASH_ENTER and HASH_ENTER_NULL. For a shared memory HTAB the HASH_ENTER_NULL will return a null if unable to allocate the entry, while HASH_ENTER will ereport ERROR with a generic message. This patch leaves HASH_ENTER on the "can't happen" cases, but replaces the ereport ERROR after it with an Assert because it's something which should never happen. The other cases are changed to HASH_ENTER_NULL so that the error message with the hint will be used instead of the more generic message. These patches are both in direct response to problems found during testing by YAMAMOTO Takashi. -Kevin
I think I see what is going on now. We are sometimes failing to set the commitSeqNo correctly on the lock. In particular, if a lock assigned to OldCommittedSxact is marked with InvalidSerCommitNo, it will never be cleared. The attached patch corrects this: TransferPredicateLocksToNewTarget should initialize a new lock entry's commitSeqNo to that of the old one being transferred, or take the minimum commitSeqNo if it is merging two lock entries. Also, CreatePredicateLock should initialize commitSeqNo for to InvalidSerCommitSeqNo instead of to 0. (I don't think using 0 would actually affect anything, but we should be consistent.) I also added a couple of assertions I used to track this down: a lock's commitSeqNo should never be zero, and it should be InvalidSerCommitSeqNo if and only if the lock is not held by OldCommittedSxact. Takashi, does this patch fix your problem with leaked SIReadLocks? Dan -- Dan R. K. Ports MIT CSAIL http://drkp.net/
Вложения
hi, > I think I see what is going on now. We are sometimes failing to set the > commitSeqNo correctly on the lock. In particular, if a lock assigned to > OldCommittedSxact is marked with InvalidSerCommitNo, it will never be > cleared. > > The attached patch corrects this: > TransferPredicateLocksToNewTarget should initialize a new lock > entry's commitSeqNo to that of the old one being transferred, or take > the minimum commitSeqNo if it is merging two lock entries. > > Also, CreatePredicateLock should initialize commitSeqNo for to > InvalidSerCommitSeqNo instead of to 0. (I don't think using 0 would > actually affect anything, but we should be consistent.) > > I also added a couple of assertions I used to track this down: a > lock's commitSeqNo should never be zero, and it should be > InvalidSerCommitSeqNo if and only if the lock is not held by > OldCommittedSxact. > > Takashi, does this patch fix your problem with leaked SIReadLocks? i'm currently running bf6848bc8c82e82f857d48185554bc3e6dcf1013 with this patch applied. i haven't seen the symptom yet. i'll keep it running for a while. btw, i've noticed the following message in the server log. is it normal? LOG: could not truncate directory "pg_serial": apparent wraparound YAMAMOTO Takashi > > Dan > > > -- > Dan R. K. Ports MIT CSAIL http://drkp.net/
YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> wrote: > LOG: could not truncate directory "pg_serial": apparent > wraparound Did you get a warning with this text?: memory for serializable conflict tracking is nearly exhausted If not, there's some sort of cleanup bug to fix in the predicate locking's use of SLRU. It may be benign, but we won't really know until we find it. I'm investigating. -Kevin
I wrote: > YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> wrote: > >> LOG: could not truncate directory "pg_serial": apparent >> wraparound > there's some sort of cleanup bug to fix in the predicate > locking's use of SLRU. It may be benign, but we won't really know > until we find it. I'm investigating. I'm pretty sure I found it. When the number serializable transactions which need to be tracked gets high enough to push things to the SLRU summarization, and then drops back down, we haven't been truncating the head page of the active SLRU region because if we go back into SLRU summarization that saves us from zeroing the page again. The problem is that if we don't go back into SLRU summarization for a long time, we might wrap around to where SLRU is upset that our head is chasing our tail. This seems like a bigger problem than we were trying to solve by not truncating the page. The issue is complicated a little bit by the fact that the SLRU API has you specify the *page* for the truncation point, but silently ignores the request unless the page is in a segment which is past a segment in use. So adding the number of pages per SLRU segment to the head page position should do the right thing. But it's all weird enough that I felt it need a bit of commenting. While I was there I noticed that we're doing the unnecessary flushing (so people can glean information about the SLRU activity from watching the disk files) right before truncating. I switched the truncation to come before the flushing, since flushing pages to a file and then deleting that file didn't seem productive. Attached find a patch which modifies one line of code, switches the order of two lines of code, and adds comments. I will add this to the open items for 9.1. Thanks again to YAMAMOTO Takashi for his rigorous testing. -Kevin
Вложения
hi, > YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp> wrote: > >> LOG: could not truncate directory "pg_serial": apparent >> wraparound > > Did you get a warning with this text?: > > memory for serializable conflict tracking is nearly exhausted there is not such a warning near the above "aparent wraparound" record. not sure if it was far before the record as i've lost the older log files. YAMAMOTO Takashi > > If not, there's some sort of cleanup bug to fix in the predicate > locking's use of SLRU. It may be benign, but we won't really know > until we find it. I'm investigating. > > -Kevin
hi, > hi, > >> I think I see what is going on now. We are sometimes failing to set the >> commitSeqNo correctly on the lock. In particular, if a lock assigned to >> OldCommittedSxact is marked with InvalidSerCommitNo, it will never be >> cleared. >> >> The attached patch corrects this: >> TransferPredicateLocksToNewTarget should initialize a new lock >> entry's commitSeqNo to that of the old one being transferred, or take >> the minimum commitSeqNo if it is merging two lock entries. >> >> Also, CreatePredicateLock should initialize commitSeqNo for to >> InvalidSerCommitSeqNo instead of to 0. (I don't think using 0 would >> actually affect anything, but we should be consistent.) >> >> I also added a couple of assertions I used to track this down: a >> lock's commitSeqNo should never be zero, and it should be >> InvalidSerCommitSeqNo if and only if the lock is not held by >> OldCommittedSxact. >> >> Takashi, does this patch fix your problem with leaked SIReadLocks? > > i'm currently running bf6848bc8c82e82f857d48185554bc3e6dcf1013 with this > patch applied. i haven't seen the symptom yet. i'll keep it running for > a while. i haven't seen the symptom since them. so i guess it was fixed by your patch. thanks! YAMAMOTO Takashi > > btw, i've noticed the following message in the server log. is it normal? > > LOG: could not truncate directory "pg_serial": apparent wraparound > > YAMAMOTO Takashi > >> >> Dan >> >> >> -- >> Dan R. K. Ports MIT CSAIL http://drkp.net/ > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers
On 11.04.2011 11:33, Heikki Linnakangas wrote: > I also noticed that there's a few hash_search(HASH_ENTER) calls in > predicate.c followed by check for a NULL result. But with HASH_ENTER, > hash_search never returns NULL, it throws an "out of shared memory" > error internally. I changed those calls to use HASH_ENTER_NULL, so you > now get the intended error message with the hint to raise > max_pred_locks_per_transaction. Oops, those were already fixed. Never mind. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On 31.03.2011 22:06, Kevin Grittner wrote: > Heikki Linnakangas<heikki.linnakangas@enterprisedb.com> wrote: > >> That's not enough. The hash tables can grow beyond the maximum >> size you specify in ShmemInitHash. It's just a hint to size the >> directory within the hash table. >> >> We'll need to teach dynahash not to allocate any more entries >> after the preallocation. A new HASH_NO_GROW flag to hash_create() >> seems like a suitable interface. > > OK. If we're doing that, is it worth taking a look at the "safety > margin" added to the size calculations, and try to make the > calculations more accurate? > > Would you like me to code a patch for this? I finally got around to look at this. Attached patch adds a HASH_FIXED_SIZE flag, which disables the allocation of new entries after the initial allocation. I believe we have consensus to make the predicate lock hash tables fixed-size, so that there's no competition of the slack shmem space between predicate lock structures and the regular lock maanager. I also noticed that there's a few hash_search(HASH_ENTER) calls in predicate.c followed by check for a NULL result. But with HASH_ENTER, hash_search never returns NULL, it throws an "out of shared memory" error internally. I changed those calls to use HASH_ENTER_NULL, so you now get the intended error message with the hint to raise max_pred_locks_per_transaction. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Вложения
On 11.04.2011 11:33, Heikki Linnakangas wrote: > On 31.03.2011 22:06, Kevin Grittner wrote: >> Heikki Linnakangas<heikki.linnakangas@enterprisedb.com> wrote: >> >>> That's not enough. The hash tables can grow beyond the maximum >>> size you specify in ShmemInitHash. It's just a hint to size the >>> directory within the hash table. >>> >>> We'll need to teach dynahash not to allocate any more entries >>> after the preallocation. A new HASH_NO_GROW flag to hash_create() >>> seems like a suitable interface. >> >> OK. If we're doing that, is it worth taking a look at the "safety >> margin" added to the size calculations, and try to make the >> calculations more accurate? >> >> Would you like me to code a patch for this? > > I finally got around to look at this. Attached patch adds a > HASH_FIXED_SIZE flag, which disables the allocation of new entries after > the initial allocation. I believe we have consensus to make the > predicate lock hash tables fixed-size, so that there's no competition of > the slack shmem space between predicate lock structures and the regular > lock maanager. Ok, committed that. I left the safety margins in the size calculations alone for now. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On 03.04.2011 09:16, Dan Ports wrote: > I think I see what is going on now. We are sometimes failing to set the > commitSeqNo correctly on the lock. In particular, if a lock assigned to > OldCommittedSxact is marked with InvalidSerCommitNo, it will never be > cleared. > > The attached patch corrects this: > TransferPredicateLocksToNewTarget should initialize a new lock > entry's commitSeqNo to that of the old one being transferred, or take > the minimum commitSeqNo if it is merging two lock entries. > > Also, CreatePredicateLock should initialize commitSeqNo for to > InvalidSerCommitSeqNo instead of to 0. (I don't think using 0 would > actually affect anything, but we should be consistent.) > > I also added a couple of assertions I used to track this down: a > lock's commitSeqNo should never be zero, and it should be > InvalidSerCommitSeqNo if and only if the lock is not held by > OldCommittedSxact. > Thanks, committed this. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > I finally got around to look at this. Attached patch adds a > HASH_FIXED_SIZE flag, which disables the allocation of new entries > after the initial allocation. I believe we have consensus to make > the predicate lock hash tables fixed-size, so that there's no > competition of the slack shmem space between predicate lock > structures and the regular lock maanager. OK, I can see why you preferred this -- the existing exchange of slack space with the HW lock tables remains unchanged this way, and only the new tables for predicate locking have the stricter limits. This makes it very unlikely to break current apps which might be unknowingly relying on existing allocation behavior in the HW locking area. Smart. I hadn't picked up on your intent that the new flag would only be used for the new tables, which is why it wasn't quite making sense to me before. Thanks! -Kevin