Re: [PATCH] Add support function for containment operators

From: Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>
To: Kim Johan Andersson <kimjand(at)kimmet(dot)dk>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: [PATCH] Add support function for containment operators
Date: 2023-07-06 16:15:56
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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:
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)
+ {
+ 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)

Laurenz Albe

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2023-07-06 16:20:26 Re: check_strxfrm_bug()
Previous Message Peter Eisentraut 2023-07-06 16:14:53 Re: Does a cancelled REINDEX CONCURRENTLY need to be messy?