Re: 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:
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
Attachments:
ssi-lossy-bitmap-2.patchapplication/octet-stream; name=ssi-lossy-bitmap-2.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"
***************
*** 364,369 **** bitgetpage(HeapScanDesc scan, TBMIterateResult *tbmres)
--- 365,371 ----
Page dp = (Page) BufferGetPage(buffer);
OffsetNumber maxoff = PageGetMaxOffsetNumber(dp);
OffsetNumber offnum;
+ bool valid;
for (offnum = FirstOffsetNumber; offnum <= maxoff; offnum = OffsetNumberNext(offnum))
{
***************
*** 375,382 **** bitgetpage(HeapScanDesc scan, TBMIterateResult *tbmres)
continue;
loctup.t_data = (HeapTupleHeader) PageGetItem((Page) dp, lp);
loctup.t_len = ItemIdGetLength(lp);
! if (HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer))
scan->rs_vistuples[ntup++] = offnum;
}
}
--- 377,392 ----
continue;
loctup.t_data = (HeapTupleHeader) PageGetItem((Page) dp, lp);
loctup.t_len = ItemIdGetLength(lp);
! loctup.t_tableOid = scan->rs_rd->rd_id;
! ItemPointerSet(&loctup.t_self, page, offnum);
! 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);
}
}
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