Re: TABLESAMPLE patch

On Mon, Mar 9, 2015 at 3:08 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
>
> On 09/03/15 04:51, Amit Kapila wrote:
>>
>> On Sat, Mar 7, 2015 at 10:37 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com
>> > Double checking for tuple visibility is the only downside I can think
>> of.
>>
>> That will happen if we use heapgetpage and the way currently
>> code is written in patch, however we can easily avoid double
>> checking if we don't call heapgetpage and rather do the required
>> work at caller's place.
>>
>
> What's the point of pagemode then if the caller code does the visibility
checks still one by one on each call. I thought one of the points of
pagemode was to do this in one step (and one buffer lock).
>

You only need one buffer lock for doing at caller's location
as well something like we do in acquire_sample_rows().

> And if the caller will try to do it in one step and cache the visibility
info then we'll end up with pretty much same structure as rs_vistuples -
there isn't saner way to cache this info other than ordered vector of tuple
offsets, unless we assume that most pages have close to MaxOffsetNumber of
tuples which they don't, so why not just use the heapgetpage directly and
do the binary search over rs_vistuples.
>

The downside of doing it via heapgetpage is that it will do
visibility test for tuples which we might not even need (I think
we should do visibility test for tuples retrurned by tsmnexttuple).

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com