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/

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(at)csail(dot)mit(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)csail(dot)mit(dot)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(dot)linnakangas(at)enterprisedb(dot)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(at)csail(dot)mit(dot)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