Re: SSI modularity questions

Heikki Linnakangas wrote:
> On 28.06.2011 20:47, Kevin Grittner wrote:

> Hmm, the calls in question are the ones in heapgettup() and
> heapgettup_pagemode(), which are subroutines of heap_getnext().
> heap_getnext() is only used in sequential scans, so it seems safe
> to remove those calls.

I haven't found anything to the contrary, if I understand correctly,
Dan found the same, and all the tests pass without them. Here's a
patch to remove them. This makes the recently-added
rs_relpredicatelocked boolean field unnecessary, so that's removed in
this patch, too.

>> I would like to add a test involving a lossy bitmap scan. How many
>> rows are normally needed to force a bitmap scan to be lossy?
>
> The size of bitmaps is controlled by work_mem, so you can set
> work_mem very small to cause them to become lossy earlier. Off the
> top of my head I don't have any guesstimate on how many rows you
> need.
>
>> What's the easiest way to check whether a plan is going to use (or
>> is using) a lossy bitmap scan?
>
> Good question. There doesn't seem to be anything in the EXPLAIN
> ANALYZE output to show that, so I think you'll have to resort to
> adding some elog()s in the right places.

OK, thanks.

-Kevin

On 29.06.2011 00:33, Kevin Grittner wrote:
> Heikki Linnakangas wrote:
>> On 28.06.2011 20:47, Kevin Grittner wrote:
>
>> Hmm, the calls in question are the ones in heapgettup() and
>> heapgettup_pagemode(), which are subroutines of heap_getnext().
>> heap_getnext() is only used in sequential scans, so it seems safe
>> to remove those calls.
>
> I haven't found anything to the contrary, if I understand correctly,
> Dan found the same, and all the tests pass without them. Here's a
> patch to remove them. This makes the recently-added
> rs_relpredicatelocked boolean field unnecessary, so that's removed in
> this patch, too.

Thanks, committed. I also moved the PredicateLockRelation() call to
heap_beginscan(), per earlier discussion.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> On 29.06.2011 00:33, Kevin Grittner wrote:
>> Heikki Linnakangas wrote:
>>> On 28.06.2011 20:47, Kevin Grittner wrote:
>>
>>> Hmm, the calls in question are the ones in heapgettup() and
>>> heapgettup_pagemode(), which are subroutines of heap_getnext().
>>> heap_getnext() is only used in sequential scans, so it seems
>>> safe to remove those calls.
>>
>> I haven't found anything to the contrary, if I understand
>> correctly, Dan found the same, and all the tests pass without
>> them. Here's a patch to remove them. This makes the
>> recently-added rs_relpredicatelocked boolean field unnecessary,
>> so that's removed in this patch, too.
>
> Thanks, committed. I also moved the PredicateLockRelation() call
> to heap_beginscan(), per earlier discussion.

Thanks!

Before we leave the subject of modularity, do you think the entire
"else" clause dealing with the lossy bitmaps should be a heapam.c
function called from nodeBitmapHeapscan.c? With the move of the
PredicateLockRelation() call you mention above, that leaves this as
the only place in the executor which references SSI, and it also is
the only place in the executor to call PageGetMaxOffsetNumber() and
OffsetNumberNext(), which seem like AM things. The logic seems
somewhat similar to heap_hot_search_buffer() and such a function
would take roughly the same parameters.

On the other hand, it's obviously not a bug, so maybe that's
something to put on a list to look at later.

-Kevin