Message-ID: <4e6efd382b3195eb7ea438b7219cc94d9399c0bd.camel@cybertec.at>
Subject: Re: [PATCH] Add support function for containment operators
From: Laurenz Albe <laurenz.albe@cybertec.at>
To: Kim Johan Andersson <kimjand@kimmet.dk>, 
	pgsql-hackers@lists.postgresql.org
Date: Thu, 06 Jul 2023 18:15:56 +0200
In-Reply-To: <94f64d1f-b8c0-b0c5-98bc-0793a34e0851@kimmet.dk>
References: <94f64d1f-b8c0-b0c5-98bc-0793a34e0851@kimmet.dk>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/4e6efd382b3195eb7ea438b7219cc94d9399c0bd.camel%40cybertec.at>
Precedence: bulk

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 <@=20
> operators when examining if an element is contained in a range.
> Based on the discussion in [1] I would like to suggest the following=20
> changes:
>=20
> This patch attempts to improve the row estimation, as well as opening=20
> the possibility of using a btree index scan when using the containment=
=20
> operators.
>=20
> This is done via a new support function handling the following 2 requests=
:
>=20
> * SupportRequestIndexCondition
> find_index_quals will build an operator clause, given at least one=20
> finite RangeBound.
>=20
> * SupportRequestSimplify
> find_simplified_clause will rewrite the containment operator into a=20
> clause using inequality operators from the btree family (if available=20
> for the element type).
>=20
> A boolean constant is returned if the range is either empty or has no=20
> bounds.
>=20
> Performing the rewrite here lets the clausesel machinery provide the=20
> same estimates as for normal scalar inequalities.
>=20
> In both cases build_bound_expr is used to build the operator clauses=20
> 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
  >=3D and < correctly.
  Plain EXPLAIN would speed up the regression tests, which is a good thing.

- About the implementation:
  You implement both "SupportRequestIndexCondition" and "SupportRequestSimp=
lify",
  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 cod=
e?
  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 =3D=3D F_ELEM_CONTAINED_BY_RANGE ||
         req->funcid =3D=3D F_RANGE_CONTAINS_ELEM);

  if (req->funcid =3D=3D F_ELEM_CONTAINED_BY_RANGE)
  {
      [...]
  }
  else if (req->funcid =3D=3D F_RANGE_CONTAINS_ELEM)
  {
      [...]
  }

Yours,
Laurenz Albe