Refactoring IndexPath representation of index conditions

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Refactoring IndexPath representation of index conditions
Date: 2019-02-02 16:29:10
Message-ID: 22182.1549124950@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I've been poking at the problem discussed in a couple of recent threads
of letting extensions in on the ability to create "lossy index conditions"
for complex operators/functions. The current design for that in
indxpath.c is frankly just a pile of kluges of varying ages. In the
initial pass, the code merely decides that a given clause is or is not
capable of being used with an index (match_clause_to_indexcol), and
then later it generates an actual indexqual for some lossy cases
(expand_indexqual_conditions), and then still later it generates an actual
indexqual for some other cases (in particular, commutation of reversed
clauses doesn't happen until fix_indexqual_references in createplan.c).
Both the second and third passes have to expensively rediscover some
things the first pass knew already, like which side of the operator the
index column is on. In between, still other places like costsize.c
and selfuncs.c also expensively rediscover that. And, because the
IndexPath's representation of original and derived index clauses doesn't
keep a clear association between an original clause and what was derived
from it, we also fail to handle some cases where we don't really need to
re-test an original clause, see for instance
https://www.postgresql.org/message-id/27810.1547651110@sss.pgh.pa.us

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,
considering that it complicates intermediate costing steps.

What finally drove me to the breaking point on this was seeing that
if we keep this design, we'd have to look up and call an extension
operator's planner support function twice, once during
match_clause_to_indexcol (to ask whether a lossy conversion is possible)
and again in expand_indexqual_conditions (to actually do it). That's
just silly. But thinking about how to fix that led me to the conclusion
that we need a more wide-ranging refactoring that will also eliminate
the inefficiencies cited above.

Hence, I propose the attached, which replaces the separate "indexclauses",
"indexquals" and "indexqualcols" lists of IndexPaths with a single list
of IndexClause nodes. That allows us to keep a clear association between
original and derived clauses, and provides us a place to put additional
data as needed. In this version I added a "lossy" flag to tell whether
the derived clauses are an exact implementation of the original or not,
which is enough to fix the boolean-index problem mentioned above.
I also added a field to allow storing the index column list for an
indexable RowCompareExpr, avoiding the need to re-do creation of that
list at createplan time.

In this patch I also legislate that commutation of a clause is a form
of making a derived clause, and it has to be done up-front and stored
explicitly. That's a debatable choice, but I think it's a win because
it allows code such as the index cost estimators to not have to deal
with un-commuted index clauses, and (after some more refactoring)
we'll be able to avoid looking up the commutator operator twice.

As best I can tell from microbenchmarking the planner, this
patch is about a wash as it stands for simple index clauses.
I expect it will come out ahead after I've refactored
match_clause_to_indexcol and expand_indexqual_conditions to avoid
duplication of effort in the latter. It is already a measurable
and very significant win for RowCompareExpr cases, eg planning
time for "select * from tenk1 where (thousand, tenthous) < (10,100)"
drops by 30%.

An interesting side effect, visible in the regression tests, is
that EXPLAIN now always shows index clauses with the index column
on the left, since commutation happens before we create the
"indexqualorig" component of the Plan. This seems all to the
good IMO; the old output always struck me as confusing.

The next step is to actually do that refactoring inside indxpath.c,
but I felt this patch was large enough already, so I'm putting
it up for comment as-is. (There's more cleanup and elimination
of duplicate work that could happen in the index cost estimators
too, I think.)

Thoughts? If there's not objections I'd like to push this soon.

regards, tom lane

Attachment Content-Type Size
refactor-indexpath-representation-1.patch text/x-diff 114.2 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message rajan 2019-02-02 18:03:57 Re: Able to do ALTER DEFAULT PRIVILEGES from a user who is not the owner
Previous Message Alvaro Herrera 2019-02-02 16:04:55 Re: New vacuum option to do only freezing