Re: [HACKERS] Index usage for elem-contained-by-const-range clauses - Mailing list pgsql-hackers

Pritam Baral <pritam@pritambaral.com> writes:
> The topic has been previously discussed[0] on the -performance mailing list,
> about four years ago.
> In that thread, Tom suggested[0] the planner could be made to "expand
> "intcol <@
> 'x,y'::int4range" into "intcol between x and y", using something similar
> to the
> index LIKE optimization (ie, the "special operator" stuff in indxpath.c)".

> This patch tries to do exactly that.

I took a quick look through this, and have some thoughts ---

* In match_special_index_operator, there are two switch statements and
you've added a clause to only one of them.  In the second one, you need to
add a check that you're working with a btree index.  I'd expect the patch
as-is to fall over if an "indexkey <@ range" clause were matched to a hash
index.

* You're failing to account for the case of "range @> indexkey", which
really this ought to work for.  That would take a bit of fooling around
with the structure of match_special_index_operator to allow indexkey on
the right, but we've foreseen since the beginning that that would someday
be necessary.  Looks like today is that day.

* The first bit in range_elem_contained_quals will fall over for an
indexkey that is an expression rather than a simple Var.  Probably
you should just be using exprType() instead.  (Not sure about whether
that's sufficient in domain cases, though.)  Or actually, why look at
that at all?  Seems like what you want is to look at the RHS input,
ie the range value, and get the relevant element datatype from it.
That would correspond to what will happen if the <@ operator executes
normally: elem_contained_by_range does not consult the type of its LHS.

* The "return NIL" for an empty range looks pretty dubious.  Even if it
fails to fail altogether, it's not doing what we really want, which is to
signal that the condition cannot succeed so we needn't search the index.
Maybe what we should do in that case is generate an "indexkey = NULL"
qual.

* Likewise, if the range is infinite, you're just returning NIL and that's
leaving something on the table.  Probably worth generating "indexkey IS
NOT NULL" in that case; it's not going to help much in typical usage, but
it would prevent scanning nulls if there are a lot of them in the index.

* elog(ERROR) doesn't return, so stuff like this is not idiomatic:

+            if (opr2oid == InvalidOid)
+            {
+                elog(ERROR, "no <= operator for opfamily %u", opfamily);
+                return NIL;
+            }

It'd be sufficient to do

+            if (opr2oid == InvalidOid)
+                elog(ERROR, "no <= operator for opfamily %u", opfamily);

* You're not bothering to insert any inputcollid into the generated
comparison operator nodes.  I'm not sure why that fails to fall over
for text comparisons (if indeed it does fail ...) but it's wrong.
Use the range type's collation there.

* It's sort of annoying that the whole thing only works for a Const range
value.  A different approach you might think about is to make this work
more like ScalarArrayOp, ie we pass the qual through to btree as-is and
teach relevant functions in access/nbtree/ how to extract index bound
conditions from the range datum at runtime.  That would likely end up
being a significantly larger patch, though, so you might reasonably
conclude it's not worth the effort.
        regards, tom lane