Re: [HACKERS] Repeated PredicateLockRelation calls during seqscan
On 26.06.2011 23:49, Kevin Grittner wrote: Kevin Grittner wrote: Kevin Grittner wrote: Heikki Linnakangas wrote: BTW, isn't bitgetpage() in nodeBitmapHeapscan.c missing PredicateLockTuple() and CheckForSerializableConflictOut() calls in the codepath for a lossy bitmap? In the non-lossy case, heap_hot_search_buffer() takes care of it, but not in the lossy case. I think the attached addresses that. Don't commit that patch, it's not holding up in testing here. I'll look at it some more. Version 2 is attached. Thanks, applied this too. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Repeated PredicateLockRelation calls during seqscan
On 25.06.2011 22:29, Kevin Grittner wrote: In looking this over I noticed something else that doesn't seem quite right. In heapam.c there are two places where the execution of PredicateLockTuple() is conditioned not just on MVCC visibility, but also on HeapKeyTest(). I think those calls should be moved to not be conditioned on that. Otherwise we have a predicate condition being tested without locking the gaps, don't we? Locks on heap tuples don't serve the purpose of locking gaps, anyway, because you can't lock anything that doesn't yet exist that way. Locks on index pages and the heap relation serve that purpose. I wonder if we need those PredicateLockTuple() calls in heapam.c at all. You need to take a whole-relation lock on the heap to lock the gaps, to ensure that you conflict newly inserted rows. And if fetch a tuple via an index, you acquire locks on the index pages. What is the point of the PredicateLockTuple() calls? We added the rs_relpredicatelocked mechanism as an optimization, but I'm thinking that it's actually required for correctness to grab a predicate lock on the whole relation when you do a seqscan. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Repeated PredicateLockRelation calls during seqscan
Kevin Grittner wrote: Heikki Linnakangas wrote: BTW, isn't bitgetpage() in nodeBitmapHeapscan.c missing PredicateLockTuple() and CheckForSerializableConflictOut() calls in the codepath for a lossy bitmap? In the non-lossy case, heap_hot_search_buffer() takes care of it, but not in the lossy case. I think the attached addresses that. Don't commit that patch, it's not holding up in testing here. I'll look at it some more. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Repeated PredicateLockRelation calls during seqscan
Kevin Grittner wrote: Kevin Grittner wrote: Heikki Linnakangas wrote: BTW, isn't bitgetpage() in nodeBitmapHeapscan.c missing PredicateLockTuple() and CheckForSerializableConflictOut() calls in the codepath for a lossy bitmap? In the non-lossy case, heap_hot_search_buffer() takes care of it, but not in the lossy case. I think the attached addresses that. Don't commit that patch, it's not holding up in testing here. I'll look at it some more. Version 2 is attached. It initializes some data which was uninitialized in a HeapTableData structure which already existed in the code. I've been burned by this before -- making a seemingly innocuous change to code which then fails because the comments at lines 512 to 514 in htup.h are not actually true: http://git.postgresql.org/gitweb?p=postgresql.git;a=blob;f=src/include/access/htup.h;h=ba5d9b28ef19f3054191cf0f8b358ac5831a9e26;hb=8af3596d6bb6cfffb57161a62aa2f7f56d5ea3eb#l504 I asked about this the first time it bit me in this thread: http://archives.postgresql.org/pgsql-hackers/2010-03/msg00493.php which concluded here: http://archives.postgresql.org/pgsql-hackers/2010-03/msg00506.php Having been bitten by it a *second* time now, I'm inclined to go through and make the code match the comments wherever these structures are used. It's a bit late in the cycle to do that for 9.1, but I'll get something on the table for 9.2 if nobody wants to argue against that course. -Kevin ssi-lossy-bitmap-2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Repeated PredicateLockRelation calls during seqscan
Heikki Linnakangas wrote: BTW, isn't bitgetpage() in nodeBitmapHeapscan.c missing PredicateLockTuple() and CheckForSerializableConflictOut() calls in the codepath for a lossy bitmap? In the non-lossy case, heap_hot_search_buffer() takes care of it, but not in the lossy case. I think the attached addresses that. In looking this over I noticed something else that doesn't seem quite right. In heapam.c there are two places where the execution of PredicateLockTuple() is conditioned not just on MVCC visibility, but also on HeapKeyTest(). I think those calls should be moved to not be conditioned on that. Otherwise we have a predicate condition being tested without locking the gaps, don't we? -Kevin ssi-lossy-bitmap.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Repeated PredicateLockRelation calls during seqscan
On 22.06.2011 07:58, Dan Ports wrote: I was looking at ExecSeqScan today and noticed that it invokes PredicateLockRelation each time it's called, i.e. for each tuple returned. Any reason we shouldn't skip that call if rs_relpredicatelocked is already set, as in the attached patch? That would save us a bit of overhead, since checking that flag is cheaper than doing a hash lookup in the local predicate lock table before bailing out. Hmm, I wonder if we should move this logic to heapam.c. The optimization to acquire a relation lock straight away should apply to all heap scans, not only those coming from ExecSeqScan. The distinction is academic at the moment, because that's the only caller that uses a regular MVCC snapshot, but it seems like a modularity violation for nodeSeqscan.c to reach into the HeapScanDesc to set the flag and grab the whole-relation lock, while heapam.c contains the PredicateLockTuple and CheckForSerializableConflictOut() calls. BTW, isn't bitgetpage() in nodeBitmapHeapscan.c missing PredicateLockTuple() and CheckForSerializableConflictOut() calls in the codepath for a lossy bitmap? In the non-lossy case, heap_hot_search_buffer() takes care of it, but not in the lossy case. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Repeated PredicateLockRelation calls during seqscan
Dan Ports d...@csail.mit.edu writes: I was looking at ExecSeqScan today and noticed that it invokes PredicateLockRelation each time it's called, i.e. for each tuple returned. Any reason we shouldn't skip that call if rs_relpredicatelocked is already set, as in the attached patch? Why is the call in ExecSeqScan at all, and not in the node startup function? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Repeated PredicateLockRelation calls during seqscan
Tom Lane t...@sss.pgh.pa.us wrote: Why is the call in ExecSeqScan at all, and not in the node startup function? Because when I asked about the placement of such calls in January of 2010 I didn't get any advice which suggested that, and this was a place I was able to find which worked correctly. If there's a better place, based on performance and/or modularity needs, let's use it. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Repeated PredicateLockRelation calls during seqscan
On 22.06.2011 17:28, Tom Lane wrote: Dan Portsd...@csail.mit.edu writes: I was looking at ExecSeqScan today and noticed that it invokes PredicateLockRelation each time it's called, i.e. for each tuple returned. Any reason we shouldn't skip that call if rs_relpredicatelocked is already set, as in the attached patch? Why is the call in ExecSeqScan at all, and not in the node startup function? It makes sense to delay it until the scan is actually started, so that you don't get unnecessary serialization failures if the scan is never actually executed. I don't know if that was Kevin's original reason for putting it there, but that's why I left it there. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Repeated PredicateLockRelation calls during seqscan
Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: On 22.06.2011 17:28, Tom Lane wrote: Why is the call in ExecSeqScan at all, and not in the node startup function? It makes sense to delay it until the scan is actually started, so that you don't get unnecessary serialization failures if the scan is never actually executed. I don't know if that was Kevin's original reason for putting it there, but that's why I left it there. I honestly can't remember whether that was a factor. I went through the README files and source code comments and set breakpoints at the low level heap reads in gdb and captured stack traces from as many execution plans as I knew how to generate, and went looking through those for likely places to put the predicate locking calls. I reasoned through the alternatives as best I could coming in cold and having been discouraged from asking questions. It would not shock me if those with greater familiarity with the code and a deeper understanding of how the pieces fit together can improve on my work there. I certainly won't take offense at any improvements made there; but I do have some concern over making changes this late in the release cycle unless they are clearly safe. Anssi Kääriäinen put in days of testing with real production data and software, and YAMAMOTO Takashi put in what appears to have been weeks of solid run time with I don't know what testing setup, but one which was really good at exposing race conditions. I get nervous about invalidating those efforts if they won't be repeated before release. By the way, I didn't miss the concern about the lossy bitmaps in bitgetpage() -- I'm trying to work my way through that now. What's a good way to generate a plan which uses lossy bitmaps? I'd like to try to generate a failing test. That's often very useful to me during coding, and tends to make a good addition to the test suite. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Repeated PredicateLockRelation calls during seqscan
On Wed, Jun 22, 2011 at 12:07:04PM +0300, Heikki Linnakangas wrote: Hmm, I wonder if we should move this logic to heapam.c. The optimization to acquire a relation lock straight away should apply to all heap scans, not only those coming from ExecSeqScan. The distinction is academic at the moment, because that's the only caller that uses a regular MVCC snapshot, but it seems like a modularity violation for nodeSeqscan.c to reach into the HeapScanDesc to set the flag and grab the whole-relation lock, while heapam.c contains the PredicateLockTuple and CheckForSerializableConflictOut() calls. On modularity grounds, I think that's a good idea. The other PredicateLock* calls live in the access methods: heapam, nbtree, and indexam for the generic index support. heap_beginscan_internal seems like a reasonable place, as long as we're OK with taking the lock even if the scan is initialized but never called. Note that this hadn't been a reasonable option until last week when we added the check for non-MVCC snapshots, since there are lots of things that use heap scans but SeqScan is the only (currently-existing) one we want to lock. I am rather uneasy about making changes here unless we can be absolutely certain they're right... Dan -- Dan R. K. Ports MIT CSAILhttp://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
Re: [HACKERS] Repeated PredicateLockRelation calls during seqscan
Dan Ports d...@csail.mit.edu wrote: Note that this hadn't been a reasonable option until last week when we added the check for non-MVCC snapshots, since there are lots of things that use heap scans but SeqScan is the only (currently-existing) one we want to lock. That is the sort of thing that I tended to notice going through the backtraces from heap access I mentioned in another post, and is most likely the reason the call landed where it did. The MVCC snapshot tests are then a game-changer. It would be nice to find a way not to acquire the relation lock if the node is never used, though. I am rather uneasy about making changes here unless we can be absolutely certain they're right... Yeah -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
