Re: Allowing extensions to supply operator-/function-specific info

Paul Ramsey <pramsey@cleverelephant.ca> writes:
>> On Mar 4, 2019, at 2:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> BTW, if you'd like me to review the code you added for this, I'd be happy
>> to do so.  I've never looked at PostGIS' innards, but probably I can make
>> sense of the code for this despite that.

> I would be ecstatic for a review, I'm sure I've left a million loose threads dangling.

I took a look, and saw that you'd neglected to check pseudoconstantness
of the non-index argument, so this'd fail on cases like ST_DWithin(x, y)
where x is indexed and y is another column in the same table.  Also
I thought the handling of commutation could be done better.  Attached is
a suggested patch atop your f731c1b7022381dbf627cae311c3d37791bf40c3 to
fix those and a couple of nitpicky other things.  (I haven't tested this,
mind you.)

One thing that makes me itch, but I didn't address in the attached,
is that expandFunctionOid() is looking up a function by name without
any schema-qualification.  That'll fail outright if PostGIS isn't in
the search path, and even if it is, you've got security issues there.
One way to address this is to assume that the expandfn is in the same
schema as the ST_XXX function you're attached to, so you could
do "get_namespace_name(get_func_namespace(funcid))" and then include
that in the list passed to LookupFuncName.

Also, this might be as-intended but I was wondering: I'd sort of expected
you to make, eg, _ST_DWithin() and ST_DWithin() into exact synonyms.
They aren't, since the former is not connected to the support function.
Is that intentional?  I guess if you had a situation where you wanted to
force non-use of an index, being able to use _ST_DWithin() for that would
be helpful.

            regards, tom lane

diff --git a/postgis/gserialized_supportfn.c b/postgis/gserialized_supportfn.c
index 90c61f3..66b699b 100644
--- a/postgis/gserialized_supportfn.c
+++ b/postgis/gserialized_supportfn.c
@@ -60,7 +60,7 @@ Datum postgis_index_supportfn(PG_FUNCTION_ARGS);
 */
 typedef struct
 {
-    char *fn_name;
+    const char *fn_name;
     int   strategy_number; /* Index strategy to add */
     int   nargs;           /* Expected number of function arguments */
     int   expand_arg;      /* Radius argument for "within distance" search */
@@ -230,22 +230,42 @@ Datum postgis_index_supportfn(PG_FUNCTION_ARGS)
                 }

                 /*
-                * Somehow an indexed third argument has slipped in. That
-                * should not happen.
+                * We can only do something with index matches on the first or
+                * second argument.
                 */
                 if (req->indexarg > 1)
                     PG_RETURN_POINTER((Node *)NULL);

                 /*
-                * Need the argument types (which should always be geometry or geography
-                * since this function is only bound to those functions)
-                * to use in the operator function lookup
+                * Make sure we have enough arguments (just paranoia really).
                 */
-                if (nargs < 2)
+                if (nargs < 2 || nargs < idxfn.expand_arg)
                     elog(ERROR, "%s: associated with function with %d arguments", __func__, nargs);

-                leftarg = linitial(clause->args);
-                rightarg = lsecond(clause->args);
+                /*
+                * Extract "leftarg" as the arg matching the index, and
+                * "rightarg" as the other one, even if they were in the
+                * opposite order in the call.  NOTE: the functions we deal
+                * with here treat their first two arguments symmetrically
+                * enough that we needn't distinguish the two cases beyond
+                * this.  This might need more work later.
+                */
+                if (req->indexarg == 0)
+                {
+                    leftarg = linitial(clause->args);
+                    rightarg = lsecond(clause->args);
+                }
+                else
+                {
+                    rightarg = linitial(clause->args);
+                    leftarg = lsecond(clause->args);
+                }
+
+                /*
+                * Need the argument types (which should always be geometry or geography
+                * since this function is only bound to those functions)
+                * to use in the operator function lookup.
+                */
                 leftdatatype = exprType(leftarg);
                 rightdatatype = exprType(rightarg);

@@ -267,20 +287,27 @@ Datum postgis_index_supportfn(PG_FUNCTION_ARGS)
                 */
                 if (idxfn.expand_arg)
                 {
-                    Node *indexarg = req->indexarg ? rightarg : leftarg;
-                    Node *otherarg = req->indexarg ? leftarg : rightarg;
                     Node *radiusarg = (Node *) list_nth(clause->args, idxfn.expand_arg-1);
-                    // Oid indexdatatype = exprType(indexarg);
-                    Oid otherdatatype = exprType(otherarg);
-                    Oid expandfn_oid = expandFunctionOid(otherdatatype);
+                    Oid expandfn_oid = expandFunctionOid(rightdatatype);
+
+                    FuncExpr *expandexpr = makeFuncExpr(expandfn_oid, rightdatatype,
+                        list_make2(rightarg, radiusarg),
+                        InvalidOid, InvalidOid, COERCE_EXPLICIT_CALL);

-                    FuncExpr *expandexpr = makeFuncExpr(expandfn_oid, otherdatatype,
-                        list_make2(otherarg, radiusarg),
-                        InvalidOid, req->indexcollation, COERCE_EXPLICIT_CALL);
+                    /*
+                    * The comparison expression has to be a pseudoconstant,
+                    * ie not volatile nor dependent on the target index's
+                    * table.  (Including the expandfn itself in this test is
+                    * probably unnecessary, but let's be paranoid.)
+                    */
+                    if (!is_pseudo_constant_for_index((Node *) expandexpr,
+                                                      req->index))
+                        PG_RETURN_POINTER((Node *)NULL);

+                    /* OK, we can make the index condition */
                     Expr *expr = make_opclause(oproid, BOOLOID, false,
-                                  (Expr *) indexarg, (Expr *) expandexpr,
-                                  InvalidOid, req->indexcollation);
+                                  (Expr *) leftarg, (Expr *) expandexpr,
+                                  InvalidOid, InvalidOid);

                     ret = (Node *)(list_make1(expr));
                 }
@@ -289,28 +316,24 @@ Datum postgis_index_supportfn(PG_FUNCTION_ARGS)
                 * an index OpExpr with the original arguments on each
                 * side.
                 * st_intersects(g1, g2) yields: g1 && g2
+                * if g1 is the indexable expression (otherwise g2 && g1)
                 */
                 else
                 {
-                    Expr *expr;
                     /*
-                    * PgSQL wants the left-hand side to be the non-const
-                    * term, so if we have a const left we swap with
-                    * the right
+                    * The comparison expression has to be a pseudoconstant,
+                    * ie not volatile nor dependent on the target index's
+                    * table.
                     */
-                    if (IsA(leftarg, Const))
-                    {
-                        Node *tmp;
-                        oproid = get_commutator(oproid);
-                        if (!OidIsValid(oproid))
-                            PG_RETURN_POINTER((Node *)NULL);
-                        tmp = leftarg;
-                        leftarg = rightarg;
-                        rightarg = tmp;
-                    }
-                    expr = make_opclause(oproid, BOOLOID, false,
+                    if (!is_pseudo_constant_for_index(rightarg,
+                                                      req->index))
+                        PG_RETURN_POINTER((Node *)NULL);
+
+                    /* OK, we can make the index condition */
+                    Expr *expr = make_opclause(oproid, BOOLOID, false,
                                     (Expr *) leftarg, (Expr *) rightarg,
                                     InvalidOid, InvalidOid);
+
                     ret = (Node *)(list_make1(expr));
                 }