Thread: Repeated PredicateLockRelation calls during seqscan
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. Dan -- Dan R. K. Ports MIT CSAIL http://drkp.net/
Attachment
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
Dan Ports <drkp@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
Tom Lane <tgl@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
On 22.06.2011 17:28, Tom Lane wrote: > Dan Ports<drkp@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
Heikki Linnakangas <heikki.linnakangas@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
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 CSAIL http://drkp.net/
Dan Ports <drkp@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