Re: [PATCH] Add support function for containment operators

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