On Sat, 2023-04-29 at 17:07 +0200, Kim Johan Andersson wrote: > I had noticed that performance wasn't great when using the @> or <@ > operators when examining if an element is contained in a range. > Based on the discussion in [1] I would like to suggest the following > changes: > > This patch attempts to improve the row estimation, as well as opening > the possibility of using a btree index scan when using the containment > operators. > > This is done via a new support function handling the following 2 requests: > > * SupportRequestIndexCondition > find_index_quals will build an operator clause, given at least one > finite RangeBound. > > * SupportRequestSimplify > find_simplified_clause will rewrite the containment operator into a > clause using inequality operators from the btree family (if available > for the element type). > > A boolean constant is returned if the range is either empty or has no > bounds. > > Performing the rewrite here lets the clausesel machinery provide the > same estimates as for normal scalar inequalities. > > In both cases build_bound_expr is used to build the operator clauses > from RangeBounds.
I think that this is a small, but useful improvement. The patch applies and builds without warning and passes "make installcheck-world" with the (ample) new regression tests. Some comments: - About the regression tests: You are using EXPLAIN (ANALYZE, SUMMARY OFF, TIMING OFF, COSTS OFF). While that returns stable results, I don't see the added value. I think you should use EXPLAIN (COSTS OFF). You don't need to test the actual number of result rows; we can trust that the executor processes >= and < correctly. Plain EXPLAIN would speed up the regression tests, which is a good thing. - About the implementation: You implement both "SupportRequestIndexCondition" and "SupportRequestSimplify", but when I experimented, the former was never called. That does not surprise me, since any expression of the shape "expr <@ range constant" can be simplified. Is the "SupportRequestIndexCondition" branch dead code? If not, do you have an example that triggers it? - About the code: +static Node * +find_index_quals(Const *rangeConst, Expr *otherExpr, Oid opfamily) +{ [...] + + if (!(lower.infinite && upper.infinite)) + { [...] + } + + return NULL; To avoid deep indentation and to make the code more readable, I think it would be better to write if (!(lower.infinite && upper.infinite)) return NULL; and unindent the rest of the code +static Node * +match_support_request(Node *rawreq) +{ [...] + switch (req->funcid) + { + case F_ELEM_CONTAINED_BY_RANGE: [...] + case F_RANGE_CONTAINS_ELEM: [...] + default: + return NULL; + } (This code appears twice.) The default clause should not be reachable, right? I think that there should be an Assert() to verify that. Perhaps something like Assert(req->funcid == F_ELEM_CONTAINED_BY_RANGE || req->funcid == F_RANGE_CONTAINS_ELEM); if (req->funcid == F_ELEM_CONTAINED_BY_RANGE) { [...] } else if (req->funcid == F_RANGE_CONTAINS_ELEM) { [...] } Yours, Laurenz Albe