Re: TABLESAMPLE patch

On 31/01/15 20:08, Petr Jelinek wrote:
> On 31/01/15 14:27, Amit Kapila wrote:
>> On Fri, Jan 23, 2015 at 5:39 AM, Petr Jelinek <petr(at)2ndquadrant(dot)com
>> <mailto:petr(at)2ndquadrant(dot)com>> wrote:
>>
>> On 19/01/15 07:08, Amit Kapila wrote:
>>
>> On Sun, Jan 18, 2015 at 12:46 AM, Petr Jelinek
>> <petr(at)2ndquadrant(dot)com <mailto:petr(at)2ndquadrant(dot)com>
>> <mailto:petr(at)2ndquadrant(dot)com <mailto:petr(at)2ndquadrant(dot)com>>>
>> wrote:
>>
>>
>> I think that's actually good to have, because we still do costing
>> and the partial index might help produce better estimate of number
>> of returned rows for tablesample as well.
>>
>>
>> I don't understand how will it help, because for tablesample scan
>> it doesn't consider partial index at all as per patch.
>>
>
> Oh I think we were talking abut different things then, I thought you
> were talking about the index checks that planner/optimizer sometimes
> does to get more accurate statistics. I'll take another look then.

You were correct here, I removed the index consideration code for
tablesample. I went through the planner to see if the RTE needs special
handling anywhere else and it does not seem like it to me.

>>
>>
>> Refer parameter (HeapScanDesc->rs_syncscan) and syncscan.c.
>> Basically during sequiantial scan on same table by different
>> backends, we attempt to keep them synchronized to reduce the I/O.
>>
>>
>> Ah this, yes, it makes sense for bernoulli, not for system though. I
>> guess it should be used for sampling methods that use SAS_SEQUENTIAL
>> strategy.
>>
>>
>> Have you taken care of this in your latest patch?
>

I added support for this, the sampling method has seqscan boolean now
which determines this.

>> +/* XXX: better error */
>> +if (found && !visible)
>> +elog(ERROR, "Sampling method wanted to return invisible tuple");
>> +}
>>
>> You have mentioned in comment that let it examine invisible tuple,
>> but it is not clear why you want to allow examining invisible tuple
>> and then later return error, why can't it skips invisible tuple.
>>
>
> Well I think returning invisible tuples to user is bad idea and that's
> why the check, but I also think it might make sense to examine the
> invisible tuple for the sampling function in case it wants to create
> some kind of stats about the scan and wants to use those for making the
> decision about returning other tuples. The interface should be probably
> called tsmexaminetuple instead to make it more clear what the intention is.
>

I added comment to the code about this, explaining why I think it's god
idea.

>> 1.
>> How about statistics (pgstat_count_heap_getnext()) during
>> SampleNext as we do in heap_getnext?
>

Added.

Otherwise I changed the code for getting next tuple to be in separate
function inside nodeSamplescan, it uses similar logic like heapgettup
and it uses heapgetpage (which is now exported) to read the blocks. And
uses normal HeapScanDesc like sequential and bitmapindex scans do.

I didn't add the whole page visibility caching as the tuple ids we get
from sampling methods don't map well to the visibility info we get from
heapgetpage (it maps to the values in the rs_vistuples array not to to
its indexes). Commented about it in code also.

I also fixed the estimation issue reported Kyotaro HORIGUCHI (that was
just me being stupid and using baserel->tuples instead of baserel->rows).

And I did some minor changes like pg_dump support, documented the new
catalog, rename tsmreturntuple to tsmexaminetuple which fits better
IMHO, etc.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services