Re: BRIN cost estimate

On 6 April 2017 at 20:01, Emre Hasegeli <emre(at)hasegeli(dot)com> wrote:
>> + /*
>> + * Charge a small amount per range tuple which we expect to match to.
>> This
>> + * is meant to reflect the costs of manipulating the bitmap. The BRIN
>> scan
>> + * will set a bit for each page in the range when we find a matching
>> + * range, so we must multiply the charge by the number of pages in the
>> + * range.
>> + */
>> + *indexTotalCost += 0.1 * cpu_operator_cost * estimatedRanges *
>> + pagesPerRange;
>
>
> Isn't BRIN executes the operator only once per range? I think we can just
> multiply cpu_operator_cost and estimatedRanges.

The above line is for the bitmap maintenance accounting, not the
scanning of the BRIN index. That's costed by:

+ *indexTotalCost += indexRanges * (cpu_index_tuple_cost + qual_op_cost);

So not the estimated matching ranges, but the total ranges, since
we'll scan all of them.

+ *indexTotalCost += 0.1 * cpu_operator_cost * estimatedRanges *
+ pagesPerRange;

This is trying to cost up the following code in bringetbitmap()

if (addrange)
{
BlockNumber pageno;

for (pageno = heapBlk;
pageno <= heapBlk + opaque->bo_pagesPerRange - 1;
pageno++)
{
MemoryContextSwitchTo(oldcxt);
tbm_add_page(tbm, pageno);
totalpages++;
MemoryContextSwitchTo(perRangeCxt);
}

I'm charging 0.1 * cpu_operator_cost for each loop we expect to
perform here. There is no tbm_add_range(), so instead, it performs a
tbm_add_page() for each page in the range, which is why I multiply
that cost by pagesPerRange.

>
>> I also noticed that you were doing:
>>
>> + if (get_index_stats_hook &&
>> + (*get_index_stats_hook) (root, index->indexoid, 1, &vardata))
>>
>> and
>>
>> + vardata.statsTuple = SearchSysCache3(STATRELATTINH,
>> + ObjectIdGetDatum(index->indexoid),
>> + Int16GetDatum(1),
>>
>> I wondered why you picked to hardcode that as 1, and I thought that's
>> surely a bug. But then looking deeper it seems to be copied from
>> btcostestimate(), which also seems to be a long-standing bug
>> introduced in a536ed53bca40cb0d199824e358a86fcfd5db7f2. I'll go write
>> a patch for that one now.
>
>
> Yes, I copy-pasted from btcostestimate().

Turns out it's not a bug in btcostestimate(). It was just intending to
only ever get the stats for the first column in the index. Not what's
needed in the BRIN case.

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