Re: Refactoring IndexPath representation of index conditions

Andres Freund <andres@anarazel.de> writes:
> On 2019-02-02 11:29:10 -0500, Tom Lane wrote:
>> I think that the original idea here was that we should do as little
>> work as possible "up front", since most index paths will get discarded
>> before we reach createplan.c.  But to the extent that that was valid
>> at all, it's gotten overtaken by circumstances.  In particular,
>> postponing work to expand_indexqual_conditions (which is called by
>> create_index_path) is just stupid, because these days we typically
>> call that several times with the same index conditions.  It's really
>> dubious that postponing commutation to createplan.c is a net win either,

> It seems your approach isn't particularly in contradiction to the
> stated historical goal. We could create the new struct, but just not
> populate it eagerly, right?

Not really.  A large part of the point here is to record what
match_clause_to_indexcol has found out rather than have to rediscover
it later (perhaps repeatedly).

Concretely, the main extra cost that I've added in this patch
(that wouldn't have been paid anyway by the time we've finished
create_index_path) is creation of a commuted OpExpr and a
RestrictInfo for it, in cases where we have an operator that
matches the index but the index column is on the right.  I was
able to reduce that cost quite a bit by adding a bespoke
"commute_restrictinfo" function, but it's still a few palloc's
more than we did before.  However, I think the benefits are
substantial: subsequent code can uniformly assume that indexquals
have indexcol-on-left, rather than having to figure that out repeatedly,
and we can avoid repeated syscache lookups to find out the OID of
the commutator operator.  The patch as posted is still doing
one more commutator lookup than it needs to, but that'll be fixed
by folding expand_indexqual_conditions and match_clause_to_indexcol
into one step.  Also I'm not sure if I've found all the places that
are expending now-useless effort for indexcol-on-right or not;
I've not looked exhaustively.  So at the worst this choice seems to
be a wash in terms of cycles, but I have hopes that it'll be a win
before all the dust settles.  In any case I think it makes things
simpler and clearer, which is worth a good deal.

Another idea that I looked into is to not create RestrictInfos for
derived indexqual clauses, with the hope that that would further
reduce the added overhead for the commuted-clause case.  However
that crashed and burned when I found out that the extended-stats
machinery punts when given a bare clause rather than a RestrictInfo.
It could possibly be fixed to not do that, but it looks like the
consequences would be extra lookups that'd probably cost more than
we saved by omitting the RestrictInfo.  Also, having RestrictInfos
means that we can cache selectivity estimates across multiple
calls.  I'm not entirely sure how much that matters in this
context, but it's probably not negligible.

            regards, tom lane