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/
Attachments:
predlock-seqscan.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/executor/nodeSeqscan.c b/src/backend/executor/nodeSeqscan.c
index f356874..32a8f56 100644
--- a/src/backend/executor/nodeSeqscan.c
+++ b/src/backend/executor/nodeSeqscan.c
@@ -113,9 +113,13 @@ SeqRecheck(SeqScanState *node, TupleTableSlot *slot)
TupleTableSlot *
ExecSeqScan(SeqScanState *node)
{
- PredicateLockRelation(node->ss_currentRelation,
- node->ss_currentScanDesc->rs_snapshot);
- node->ss_currentScanDesc->rs_relpredicatelocked = true;
+ if (!node->ss_currentScanDesc->rs_relpredicatelocked)
+ {
+ PredicateLockRelation(node->ss_currentRelation,
+ node->ss_currentScanDesc->rs_snapshot);
+ node->ss_currentScanDesc->rs_relpredicatelocked = true;
+ }
+
return ExecScan((ScanState *) node,
(ExecScanAccessMtd) SeqNext,
(ExecScanRecheckMtd) SeqRecheck);
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
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
Attachments:
ssi-lossy-bitmap.patchapplication/octet-stream; name=ssi-lossy-bitmap.patchDownload
*** a/src/backend/executor/nodeBitmapHeapscan.c
--- b/src/backend/executor/nodeBitmapHeapscan.c
***************
*** 42,47 ****
--- 42,48 ----
#include "executor/nodeBitmapHeapscan.h"
#include "pgstat.h"
#include "storage/bufmgr.h"
+ #include "storage/predicate.h"
#include "utils/memutils.h"
#include "utils/snapmgr.h"
#include "utils/tqual.h"
***************
*** 369,382 **** bitgetpage(HeapScanDesc scan, TBMIterateResult *tbmres)
{
ItemId lp;
HeapTupleData loctup;
lp = PageGetItemId(dp, offnum);
if (!ItemIdIsNormal(lp))
continue;
loctup.t_data = (HeapTupleHeader) PageGetItem((Page) dp, lp);
loctup.t_len = ItemIdGetLength(lp);
! if (HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer))
scan->rs_vistuples[ntup++] = offnum;
}
}
--- 370,390 ----
{
ItemId lp;
HeapTupleData loctup;
+ bool valid;
lp = PageGetItemId(dp, offnum);
if (!ItemIdIsNormal(lp))
continue;
loctup.t_data = (HeapTupleHeader) PageGetItem((Page) dp, lp);
loctup.t_len = ItemIdGetLength(lp);
! valid = HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer);
! if (valid)
! {
scan->rs_vistuples[ntup++] = offnum;
+ PredicateLockTuple(scan->rs_rd, &loctup, snapshot);
+ }
+ CheckForSerializableConflictOut(valid, scan->rs_rd, &loctup,
+ buffer, snapshot);
}
}
Import Notes
Resolved by subject fallback
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
"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
Import Notes
Resolved by subject fallback