Re: [PATCH] minor optimization for ineq_histogram_selectivity()

On 11/23/22 16:59, Tom Lane wrote:
> =?UTF-8?Q?Fr=c3=a9d=c3=a9ric_Yhuel?= <frederic(dot)yhuel(at)dalibo(dot)com> writes:
>> On 10/24/22 17:26, Frédéric Yhuel wrote:
>>> When studying the weird planner issue reported here [1], I came up with
>>> the attached patch. It reduces the probability of calling
>>> get_actual_variable_range().
>
>> This isn't very useful anymore thanks to this patch:
>> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=9c6ad5eaa957bdc2132b900a96e0d2ec9264d39c
>
> I hadn't looked at this patch before, but now that I have, I'm inclined
> to reject it anyway. It just moves the problem around: now, instead of
> possibly doing an unnecessary index probe at the right end, you're
> possibly doing an unnecessary index probe at the left end.

Indeed... it seemed to me that both versions would do an unnecessary
index probe at the left end, but I wasn't careful enough :-/

> It also
> looks quite weird compared to the normal coding of binary search.
>

That's right.

> I wonder if there'd be something to be said for leaving the initial
> probe calculation alone and doing this:
>
> else if (probe == sslot.nvalues - 1 && sslot.nvalues > 2)
> + {
> + /* Don't probe the endpoint until we have to. */
> + if (probe > lobound)
> + probe--;
> + else
> have_end = get_actual_variable_range(root,
> vardata,
> sslot.staop,
> collation,
> NULL,
> &sslot.values[probe]);
> + }
>
> On the whole though, it seems like a wart.
>
>

Yeah... it's probably wiser not risking introducing a bug, only to save
an index probe in rare cases (and only 100 reads, thanks to 9c6ad5ea).

Thank you for having had a look at it.

Best regards,
Frédéric