From: Tom Lane <tgl@sss.pgh.pa.us>
To: Paul Ramsey <pramsey@cleverelephant.ca>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Allowing extensions to supply operator-/function-specific info
Date: Mon, 04 Mar 2019 19:22:59 -0500
Message-ID: <914.1551745379@sss.pgh.pa.us> (raw)
In-Reply-To: <48F6F422-4F50-4DA6-A6A1-13C4AA1B9D27@cleverelephant.ca>
References: <15193.1548028093@sss.pgh.pa.us>
<6953.1548297558@sss.pgh.pa.us>
<6044.1548524131@sss.pgh.pa.us>
<CA+TgmoYkLjVcbZZ8PYyaCTMYZJCQ8eiod8MSRMDaczjBw=a0bQ@mail.gmail.com>
<19993.1548697885@sss.pgh.pa.us>
<CACowWR3jZ4Xo1atU5xTDpEoeeKSw1==sKPhCy3a29HtcE9oQVQ@mail.gmail.com>
<7671.1551135674@sss.pgh.pa.us>
<CACowWR3_RyPSYO6a8TVLZ7KnL_rDNh2ywkSTeqFmrAPpP4y6-g@mail.gmail.com>
<11051.1551139751@sss.pgh.pa.us>
<CACowWR1ycbLx0t7WwT8uuJnK7E3U+tfzKXBRPaQrgHiCG+hvbA@mail.gmail.com>
<14747.1551219541@sss.pgh.pa.us>
<91BAE1F4-7C16-4F87-849F-B01175A8D7D8@cleverelephant.ca>
<17244.1551226168@sss.pgh.pa.us>
<74C11775-67A7-48D2-B1E3-24C52A686F86@cleverelephant.ca>
<15086.1551310830@sss.pgh.pa.us>
<BBB5F584-5036-4A11-B5FF-D30632EC9E1A@cleverelephant.ca>
<24277.1551320775@sss.pgh.pa.us>
<CACowWR3fZRV6fGrH7vsVLqLz0TQJpmzb=wcipqAQBVZSn0sT4g@mail.gmail.com>
<26156.1551734021@sss.pgh.pa.us>
<B256FF5D-EF73-4FDE-A50D-9C9E9FB3B159@cleverelephant.ca>
<31607.1551739943@sss.pgh.pa.us>
<48F6F422-4! F50-4DA6-A6A1-13C4AA1B9D27@cleverelephant.ca>
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
Attachments:
[text/x-diff] tgl-support-fixes.patch (5.0K, ../914.1551745379@sss.pgh.pa.us/2-tgl-support-fixes.patch)
download | inline diff:diff --git a/postgis/gserialized_supportfn.c b/postgis/gserialized_supportfn.cindex 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));
}
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, pramsey@cleverelephant.ca, pgsql-hackers@lists.postgresql.org
Subject: Re: Allowing extensions to supply operator-/function-specific info
In-Reply-To: <914.1551745379@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox