> Good point. That's wrong, but I'm confused at why you kept the:
> + *indexTotalCost += selec * numTuples * cpu_index_tuple_cost;
> at all if that's the case. All the BRIN scan is going to do is build a
> bitmap of the matching ranges found.

My mind was not clear when I was working on it a year ago.

I've ended up with:
> + /*
> + * 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.

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().

I do still have concerns about how the correlation value is used, and
> the fact that we use the highest value, but then the whole use of this
> value is far from perfect, and is just a rough indication of how
> things are.

I agree.  I tried to document how incomplete it is on the comments I wrote
to help future developers improve the situation.

Reply via email to