[PATCH] Add support function for containment operators

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.

Thanks to Laurenz Albe for giving the patch a look before submission.

[1]
https://www.postgresql.org/message-id/[email protected]

Regards,
Kim Johan Andersson

Any thoughts on this change?

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

> 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.

I managed to break the patch:

CREATE DATABASE czech ICU_LOCALE "cs-CZ" LOCALE "cs_CZ.utf8" TEMPLATE template0;

\c czech

CREATE TYPE textrange AS RANGE (SUBTYPE = text, SUBTYPE_OPCLASS = text_pattern_ops);

CREATE TABLE tx (t text);

INSERT INTO tx VALUES ('a'), ('c'), ('d'), ('ch');

EXPLAIN SELECT * FROM tx WHERE t <@ textrange('a', 'd');

QUERY PLAN
════════════════════════════════════════════════════
Seq Scan on tx (cost=0.00..30.40 rows=7 width=32)
Filter: ((t >= 'a'::text) AND (t < 'd'::text))
(2 rows)

SELECT * FROM tx WHERE t <@ textrange('a', 'd');
ERROR: could not determine which collation to use for string comparison
HINT: Use the COLLATE clause to set the collation explicitly.

The replacement operators are wrong; it should be ~>=~ and ~<~ .

Also, there should be no error message.
The result should be 'a', 'c' and 'ch'.

Yours,
Laurenz Albe

I wrote:
> 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?

I had an idea about this:
So far, you only consider constant ranges. But if we have a STABLE range
expression, you could use an index scan for "expr <@ range", for example
Index Cond (expr >= lower(range) AND expr < upper(range)).

Yours,
Laurenz Albe

On 07-07-2023 13:20, Laurenz Albe wrote:
> I wrote:
>> 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?

I would think it is dead code, I came to the same conclusion. So we can
drop SupportRequestIndexCondition, since the simplification happens to
take care of everything.

> I had an idea about this:
> So far, you only consider constant ranges. But if we have a STABLE range
> expression, you could use an index scan for "expr <@ range", for example
> Index Cond (expr >= lower(range) AND expr < upper(range)).
>

I will try to look into this. Originally that was what I was hoping for,
but didn't see way of going about it.

Thanks for your comments, I will also look at the locale-related
breakage you spotted.

Regards,
Kimjand

On Sat, 2023-07-08 at 08:11 +0200, Kim Johan Andersson wrote:
> On 07-07-2023 13:20, Laurenz Albe wrote:
> > I wrote:
> > > 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?
>
> I would think it is dead code, I came to the same conclusion. So we can
> drop SupportRequestIndexCondition, since the simplification happens to
> take care of everything.
>
>
> > I had an idea about this:
> > So far, you only consider constant ranges.  But if we have a STABLE range
> > expression, you could use an index scan for "expr <@ range", for example
> > Index Cond (expr >= lower(range) AND expr < upper(range)).
> >
>
> I will try to look into this. Originally that was what I was hoping for,
> but didn't see way of going about it.
>
> Thanks for your comments, I will also look at the locale-related
> breakage you spotted.

I have marked the patch as "returned with feedback".

I encourage you to submit an improved version in a future commitfest!

Yours,
Laurenz Albe

On Tue, Aug 1, 2023 at 10:07 AM Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at> wrote:
> >
> >
> > > I had an idea about this:
> > > So far, you only consider constant ranges. But if we have a STABLE range
> > > expression, you could use an index scan for "expr <@ range", for example
> > > Index Cond (expr >= lower(range) AND expr < upper(range)).
> > >

The above part, not sure how to implement it, not sure it is doable.

Refactor:
drop SupportRequestIndexCondition and related code, since mentioned in
upthread, it's dead code.
refactor the regression test. (since data types with infinity cover
more cases than int4range, so I deleted some tests).

now there are 3 helper functions (build_bound_expr,
find_simplified_clause, match_support_request), 2 entry functions
(elem_contained_by_range_support, range_contains_elem_support)

Collation problem seems solved. Putting the following test on the
src/test/regress/sql/rangetypes.sql will not work. Maybe because of
the order of the regression test, in SQL-ASCII encoding, I cannot
create collation="cs-CZ-x-icu".

drop type if EXISTS textrange1, textrange2;
drop table if EXISTS collate_test1, collate_test2;
CREATE TYPE textrange1 AS RANGE (SUBTYPE = text, collation="C");
create type textrange2 as range(subtype=text, collation="cs-CZ-x-icu");
CREATE TABLE collate_test1 (b text COLLATE "en-x-icu" NOT NULL);
INSERT INTO collate_test1(b) VALUES ('a'), ('c'), ('d'), ('ch');
CREATE TABLE collate_test2 (b text NOT NULL);
INSERT INTO collate_test2(b) VALUES ('a'), ('c'), ('d'), ('ch');

--should include 'ch'
SELECT * FROM collate_test1 WHERE b <@ textrange1('a', 'd');
--should not include 'ch'
SELECT * FROM collate_test1 WHERE b <@ textrange2('a', 'd');
--should include 'ch'
SELECT * FROM collate_test2 WHERE b <@ textrange1('a', 'd');
--should not include 'ch'
SELECT * FROM collate_test2 WHERE b <@ textrange2('a', 'd');
-----------------

On Fri, 2023-10-13 at 14:26 +0800, jian he wrote:
> Collation problem seems solved.

I didn't review your patch in detail, there is still a problem
with my example:

CREATE TYPE textrange AS RANGE (
SUBTYPE = text,
SUBTYPE_OPCLASS = text_pattern_ops
);

CREATE TABLE tx (t text COLLATE "cs-CZ-x-icu");

INSERT INTO tx VALUES ('a'), ('c'), ('d'), ('ch');

SELECT * FROM tx WHERE t <@ textrange('a', 'd');

t
════
a
c
ch
(3 rows)

That was correct.

EXPLAIN SELECT * FROM tx WHERE t <@ textrange('a', 'd');

QUERY PLAN
════════════════════════════════════════════════════
Seq Scan on tx (cost=0.00..30.40 rows=7 width=32)
Filter: ((t >= 'a'::text) AND (t < 'd'::text))
(2 rows)

But that was weird. The operators seem wrong. Look at that
query:

SELECT * FROM tx WHERE t >= 'a' AND t < 'd';

t
═══
a
c
(2 rows)

But the execution plan is identical...

I am not sure what is the problem here, but in my opinion the
operators shown in the execution plan should be like this:

SELECT * FROM tx WHERE t ~>=~ 'a' AND t ~<~ 'd';

t
════
a
c
ch
(3 rows)

Yours,
Laurenz Albe

On Fri, Oct 20, 2023 at 12:01 AM Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at> wrote:
>
> On Fri, 2023-10-13 at 14:26 +0800, jian he wrote:
> > Collation problem seems solved.
>
> I didn't review your patch in detail, there is still a problem
> with my example:
>
> CREATE TYPE textrange AS RANGE (
> SUBTYPE = text,
> SUBTYPE_OPCLASS = text_pattern_ops
> );
>
> CREATE TABLE tx (t text COLLATE "cs-CZ-x-icu");
>
> INSERT INTO tx VALUES ('a'), ('c'), ('d'), ('ch');
>
> SELECT * FROM tx WHERE t <@ textrange('a', 'd');
>
> t
> ════
> a
> c
> ch
> (3 rows)
>
> That was correct.
>
> EXPLAIN SELECT * FROM tx WHERE t <@ textrange('a', 'd');
>
> QUERY PLAN
> ════════════════════════════════════════════════════
> Seq Scan on tx (cost=0.00..30.40 rows=7 width=32)
> Filter: ((t >= 'a'::text) AND (t < 'd'::text))
> (2 rows)
>
> But that was weird. The operators seem wrong. Look at that

Thanks for pointing this out!

The problem is that TypeCacheEntry->rngelemtype typcaheentry don't
have the range's SUBTYPE_OPCLASS info.
So in find_simplified_clause, we need to get the range's
SUBTYPE_OPCLASS from the pg_catalog table.
Also in pg_range, column rngsubopc is not null. so this should be fine.

On Fri, 2023-10-20 at 16:24 +0800, jian he wrote:
> [new patch]

Thanks, that patch works as expected and passes regression tests.

Some comments about the code:

> --- a/src/backend/utils/adt/rangetypes.c
> +++ b/src/backend/utils/adt/rangetypes.c
> @@ -558,7 +570,6 @@ elem_contained_by_range(PG_FUNCTION_ARGS)
> PG_RETURN_BOOL(range_contains_elem_internal(typcache, r, val));
> }
>
> -
> /* range, range -> bool functions */
>
> /* equality (internal version) */

Please don't change unrelated whitespace.

> +static Node *
> +find_simplified_clause(Const *rangeConst, Expr *otherExpr)
> +{
> + Form_pg_range pg_range;
> + HeapTuple tup;
> + Oid opclassOid;
> + RangeBound lower;
> + RangeBound upper;
> + bool empty;
> + Oid rng_collation;
> + TypeCacheEntry *elemTypcache;
> + Oid opfamily = InvalidOid;
> +
> + RangeType *range = DatumGetRangeTypeP(rangeConst->constvalue);
> + TypeCacheEntry *rangetypcache = lookup_type_cache(RangeTypeGetOid(range), TYPECACHE_RANGE_INFO);
> + {

This brace is unnecessary. Perhaps a leftover from a removed conditional statement.

> + /* this part is get the range's SUBTYPE_OPCLASS from pg_range catalog.
> + * Refer load_rangetype_info function last line.
> + * TypeCacheEntry->rngelemtype typcaheenetry either don't have opclass entry or with default opclass.
> + * Range's subtype opclass only in catalog table.
> + */

The comments in the patch need some more love.
Apart from the language, you should have a look at the style guide:

- single-line comments should start with lower case and have no period:

/* example of a single-line comment */

- Multi-line comments should start with /* on its own line and end with */ on its
own line. They should use whole sentences:

/*
* In case a comment spans several lines, it should look like
* this. Try not to exceed 80 characters.
*/

> + tup = SearchSysCache1(RANGETYPE, ObjectIdGetDatum(RangeTypeGetOid(range)));
> +
> + /* should not fail, since we already checked typtype ... */
> + if (!HeapTupleIsValid(tup))
> + elog(ERROR, "cache lookup failed for range type %u", RangeTypeGetOid(range));

If this is a "can't happen" case, it should be an Assert.

> +
> + pg_range = (Form_pg_range) GETSTRUCT(tup);
> +
> + opclassOid = pg_range->rngsubopc;
> +
> + ReleaseSysCache(tup);
> +
> + /* get opclass properties and look up the comparison function */
> + opfamily = get_opclass_family(opclassOid);
> + }
> +
> + range_deserialize(rangetypcache, range, &lower, &upper, &empty);
> + rng_collation = rangetypcache->rng_collation;
> +
> + if (empty)
> + {
> + /* If the range is empty, then there can be no matches. */
> + return makeBoolConst(false, false);
> + }
> + else if (lower.infinite && upper.infinite)
> + {
> + /* The range has no bounds, so matches everything. */
> + return makeBoolConst(true, false);
> + }
> + else
> + {

Many of the variables declared at the beginning of the function are only used in
this branch. You should declare them here.

> +static Node *
> +match_support_request(Node *rawreq)
> +{
> + if (IsA(rawreq, SupportRequestSimplify))
> + {

To keep the indentation shallow, the preferred style is:

if (/* case we don't handle */)
return NULL;
/* proceed without indentation */

> + SupportRequestSimplify *req = (SupportRequestSimplify *) rawreq;
> + FuncExpr *fexpr = req->fcall;
> + Node *leftop;
> + Node *rightop;
> + Const *rangeConst;
> + Expr *otherExpr;
> +
> + Assert(list_length(fexpr->args) == 2);
> +
> + leftop = linitial(fexpr->args);
> + rightop = lsecond(fexpr->args);
> +
> + switch (fexpr->funcid)
> + {
> + case F_ELEM_CONTAINED_BY_RANGE:
> + if (!IsA(rightop, Const) || ((Const *) rightop)->constisnull)
> + return NULL;
> +
> + rangeConst = (Const *) rightop;
> + otherExpr = (Expr *) leftop;
> + break;
> +
> + case F_RANGE_CONTAINS_ELEM:
> + if (!IsA(leftop, Const) || ((Const *) leftop)->constisnull)
> + return NULL;
> +
> + rangeConst = (Const *) leftop;
> + otherExpr = (Expr *) rightop;
> + break;
> +
> + default:
> + return NULL;
> + }
> +
> + return find_simplified_clause(rangeConst, otherExpr);
> + }
> + return NULL;
> +}
> \ No newline at end of file

You are calling this funtion from both "elem_contained_by_range_support" and
"range_contains_elem_support", only to branch based on the function type.
I think the code would be simpler if you did away with "match_support_request" at all.

I adjusted your patch according to my comments; what do you think?

I also went over the regression tests. I did away with the comparison function, instead
I used examples that don't return too many rows. I cut down on the test cases a little
bit. I added a test that uses the "text_pattern_ops" operator class.

Yours,
Laurenz Albe

On Sun, 2023-11-12 at 18:15 +0100, Laurenz Albe wrote:
> I adjusted your patch according to my comments; what do you think?

I have added the patch to the January commitfest, with Jian and Kim as authors.
I hope that is OK with you.

Yours,
Laurenz Albe

On 12-11-2023 20:20, Laurenz Albe wrote:
> On Sun, 2023-11-12 at 18:15 +0100, Laurenz Albe wrote:
>> I adjusted your patch according to my comments; what do you think?
>
> I have added the patch to the January commitfest, with Jian and Kim as authors.
> I hope that is OK with you.

Sounds great to me. Thanks to Jian for picking this up.

Regards,
Kim Johan Andersson

fix a typo and also did a minor change.

from
+ if (lowerExpr != NULL && upperExpr != NULL)
+ return (Node *) makeBoolExpr(AND_EXPR, list_make2(lowerExpr, upperExpr),
-1);
+ else if (lowerExpr != NULL)
+ return (Node *) lowerExpr;
+ else if (upperExpr != NULL)
+ return (Node *) upperExpr;

to

+ if (lowerExpr != NULL && upperExpr != NULL)
+ return (Node *) makeBoolExpr(AND_EXPR, list_make2(lowerExpr, upperExpr),
-1);
+ else if (lowerExpr != NULL)
+ return (Node *) lowerExpr;
+ else if (upperExpr != NULL)
+ return (Node *) upperExpr;
+ else
+ {
+ Assert(false);
+ return NULL;
+ }

because cfbot says:

15:04:38.116] make -s -j${BUILD_JOBS} clean
[15:04:38.510] time make -s -j${BUILD_JOBS} world-bin
[15:04:43.272] rangetypes.c:2908:1: error: non-void function does not
return a value in all control paths [-Werror,-Wreturn-type]
[15:04:43.272] }
[15:04:43.272] ^
[15:04:43.272] 1 error generated.

also add some commit messages, I hope it will be useful.

jian he <jian(dot)universality(at)gmail(dot)com> writes:
> [ v5-0001-Simplify-containment-in-range-constants-with-supp.patch ]

I spent some time reviewing and cleaning up this code. The major
problem I noted was that it doesn't spend any effort thinking about
cases where it'd be unsafe or undesirable to apply the transformation.
In particular, it's entirely uncool to produce a double-sided
comparison if the elemExpr is volatile. These two expressions
do not have the same result:

select random() <@ float8range(0.1, 0.2);
select random() >= 0.1 and random() < 0.2;

(Yes, I'm aware that BETWEEN is broken in this respect. All the
more reason why we mustn't break <@.)

Another problem is that even if the elemExpr isn't volatile,
it might be expensive enough that evaluating it twice is bad.
I am not sure where we ought to put the cutoff. There are some
existing places where we set a 10X cpu_operator_cost limit on
comparable transformations, so I stole that logic in the attached.
But perhaps someone has an argument for a different rule?

Anyway, pending discussion of that point, I think the code is good
to go. I don't like the test cases much though: they expend many more
cycles than necessary. You could prove the same points just by
looking at the expansion of expressions, eg.

regression=# explain (verbose, costs off) select current_date <@ daterange(null,null);
QUERY PLAN
----------------
Result
Output: true
(2 rows)

regression=# explain (verbose, costs off) select current_date <@ daterange('-Infinity', '1997-04-10'::date, '[)');
QUERY PLAN
-----------------------------------------------------------------------------------------
Result
Output: ((CURRENT_DATE >= '-infinity'::date) AND (CURRENT_DATE < '1997-04-10'::date))
(2 rows)

I'd suggest losing the temp table and just coding tests like these.

regards, tom lane

On Wed, Jan 17, 2024 at 5:46 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> But perhaps someone has an argument for a different rule?
>
> Anyway, pending discussion of that point, I think the code is good
> to go. I don't like the test cases much though: they expend many more
> cycles than necessary. You could prove the same points just by
> looking at the expansion of expressions, eg.
>

your patch is far better!

IMHO, worried about the support function, the transformed plan
generates the wrong result,
so we add the tests to make it bullet proof.
Now I see your point. If the transformed plan is right, the whole
added code should be fine.
but keeping the textrange_supp related test should be a good idea.
since we don't have SUBTYPE_OPCLASS related sql tests.

jian he <jian(dot)universality(at)gmail(dot)com> writes:
> Now I see your point. If the transformed plan is right, the whole
> added code should be fine.
> but keeping the textrange_supp related test should be a good idea.
> since we don't have SUBTYPE_OPCLASS related sql tests.

Yeah, it's a little harder to make a table-less test for that case.
I thought about using current_user or the like as a stable comparison
value, but that introduces some doubt about what the collation would
be. That test seems cheap enough as-is, since it's handling only a
tiny amount of data.

Committed.

regards, tom lane

On Sun, 21 Jan 2024 at 00:31, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> jian he <jian(dot)universality(at)gmail(dot)com> writes:
> > Now I see your point. If the transformed plan is right, the whole
> > added code should be fine.
> > but keeping the textrange_supp related test should be a good idea.
> > since we don't have SUBTYPE_OPCLASS related sql tests.
>
> Yeah, it's a little harder to make a table-less test for that case.
> I thought about using current_user or the like as a stable comparison
> value, but that introduces some doubt about what the collation would
> be. That test seems cheap enough as-is, since it's handling only a
> tiny amount of data.
>
> Committed.

I have updated the commitfest entry to Committed as the patch is committed.

Regards,
Vignesh