Refactoring IndexPath representation of index conditions
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Date: 2019-02-02T16:29:10Z
Lists: pgsql-hackers
Attachments
- refactor-indexpath-representation-1.patch (text/x-diff) patch
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
Commits
-
Refactor the representation of indexable clauses in IndexPaths.
- 1a8d5afb0dfc 12.0 landed