Обсуждение: Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types)
On Sun Mar 7, 2021 at 2:37 AM EST, Dian M Fay wrote: > > What'd be better, if we could do it, is to ship the clause in > > the form > > WHERE foreigncol = 'one' > > that is, instead of plastering a cast on the Var, try to not put > > any explicit cast on the constant. That fixes your original use > > case better than what you've proposed, and I think it might be > > possible to do it unconditionally instead of needing a hacky > > column property to enable it. The reason this could be okay > > is that it seems reasonable for postgres_fdw to rely on the > > core parser's heuristic that an unknown-type literal is the > > same type as what it's being compared to. So, if we are trying > > to deparse something of the form "foreigncol operator constant", > > and the foreigncol and constant are of the same type locally, > > we could leave off the cast on the constant. (There might need > > to be some restrictions about the operator taking those types > > natively with no cast, not sure; also this doesn't apply to > > constants that are going to be printed as non-string literals.) > > > > Slipping this heuristic into the code structure of deparse.c > > might be rather messy, though. I've not looked at just how > > to implement it. > > This doesn't look too bad from here, at least so far. The attached > change adds a new const_showtype field to the deparse_expr_cxt, and > passes that instead of the hardcoded 0 to deparseConst. deparseOpExpr > modifies const_showtype if both sides of a binary operation are text, > and resets it to 0 after the recursion. > > I restricted it to text-only after seeing a regression test fail: while > deparsing `percentile_cont(c2/10::numeric)`, c2, an integer column, is a > FuncExpr with a numeric return type. That matches the numeric 10, and > without the explicit cast, integer-division-related havoc ensues. I > don't know why it's a FuncExpr, and I don't know why it's not an int, > but the constant is definitely a non-string, in any case. > > In the course of testing, I discovered that the @@ text-search operator > works against textified enums on my stock 13.1 server (a "proper" enum > column yields "operator does not exist"). I'm rather wary of actually > trying to depend on that behavior, although it seems probably-safe in > the same character set and collation. hello again! My second version of this change (suppressing the cast entirely as Tom suggested) seemed to slip under the radar back in March and then other matters intervened. I'm still interested in making it happen, though, and now that we're out of another commitfest it seems like a good time to bring it back up. Here's a rebased patch to start.
Вложения
"Dian M Fay" <dian.m.fay@gmail.com> writes:
> [ 0001-Suppress-explicit-casts-of-text-constants-in-postgre.patch ]
I took a quick look at this. The restriction to type text seems like
very obviously a hack rather than something we actually want; wouldn't
it mean we fail to act in a large fraction of the cases where we'd
like to suppress the cast?
A second problem is that I don't think the business with a const_showtype
context field is safe at all. As you've implemented it here, it would
affect the entire RHS tree, including constants far down inside complex
expressions that have nothing to do with the top-level semantics.
(I didn't look closely, but I wonder if the regression failure you
mentioned is associated with that.)
I think that we only want to suppress the cast in cases where
(1) the constant is directly an operand of the operator we're
expecting the remote parser to use its same-type heuristic for, and
(2) the constant will be deparsed as a string literal. (If it's
deparsed as a number, boolean, etc, then it won't be initially
UNKNOWN, so that heuristic won't be applied.)
Now point 1 means that we don't really need to mess with keeping
state in the recursion context. If we've determined at the level
of the OpExpr that we can do this, including checking that the
RHS operand IsA(Const), then we can just invoke deparseConst() on
it directly instead of recursing via deparseExpr().
Meanwhile, I suspect that point 2 might be best checked within
deparseConst() itself, as that contains both the decision and the
mechanism about how the Const will be printed. So that suggests
that we should invent a new showtype code telling deparseConst()
to act this way, and then supply that code directly when we
invoke deparseConst directly from deparseOpExpr.
BTW, don't we also want to be able to optimize cases where the Const
is on the LHS rather than the RHS?
regards, tom lane
On Sun Sep 5, 2021 at 6:43 PM EDT, Tom Lane wrote: > "Dian M Fay" <dian.m.fay@gmail.com> writes: > > [ 0001-Suppress-explicit-casts-of-text-constants-in-postgre.patch ] > > I took a quick look at this. The restriction to type text seems like > very obviously a hack rather than something we actually want; wouldn't > it mean we fail to act in a large fraction of the cases where we'd > like to suppress the cast? > > A second problem is that I don't think the business with a > const_showtype > context field is safe at all. As you've implemented it here, it would > affect the entire RHS tree, including constants far down inside complex > expressions that have nothing to do with the top-level semantics. > (I didn't look closely, but I wonder if the regression failure you > mentioned is associated with that.) > > I think that we only want to suppress the cast in cases where > (1) the constant is directly an operand of the operator we're > expecting the remote parser to use its same-type heuristic for, and > (2) the constant will be deparsed as a string literal. (If it's > deparsed as a number, boolean, etc, then it won't be initially > UNKNOWN, so that heuristic won't be applied.) > > Now point 1 means that we don't really need to mess with keeping > state in the recursion context. If we've determined at the level > of the OpExpr that we can do this, including checking that the > RHS operand IsA(Const), then we can just invoke deparseConst() on > it directly instead of recursing via deparseExpr(). > > Meanwhile, I suspect that point 2 might be best checked within > deparseConst() itself, as that contains both the decision and the > mechanism about how the Const will be printed. So that suggests > that we should invent a new showtype code telling deparseConst() > to act this way, and then supply that code directly when we > invoke deparseConst directly from deparseOpExpr. > > BTW, don't we also want to be able to optimize cases where the Const > is on the LHS rather than the RHS? > > regards, tom lane Thanks Tom, that makes way more sense! I've attached a new patch which tests operands and makes sure one side is a Const before feeding it to deparseConst with a new showtype code, -2. The one regression is gone, but I've left a couple of test output discrepancies for now which showcase lost casts on the following predicates: * date(c5) = '1970-01-17'::date * ctid = '(0,2)'::tid These aren't exactly failures -- both implicit string comparisons work just fine -- but I don't know Postgres well enough to be sure that that's true more generally. I did try checking that the non-Const member of the predicate is a Var; that left the date cast alone, since date(c5) is a FuncExpr, but obviously can't do anything about the tid. There's also an interesting case where `val::text LIKE 'foo'` works when val is an enum column in the local table, and breaks, castless, with an operator mismatch when it's altered to text: Postgres' statement parser recognizes the cast as redundant and creates a Var node instead of a RelabelType (as it will for, say, `val::varchar(10)`) before the FDW is even in the picture. It's a little discomfiting, but I suppose a certain level of "caveat emptor" entails when disregarding foreign types. > (val as enum on local and remote) > explain verbose select * from test where (val::text) like 'foo'; > > Foreign Scan on public.test (cost=100.00..169.06 rows=8 width=28) > Output: id, val, on_day, ts, ts2 > Filter: ((test.val)::text ~~ 'foo'::text) > Remote SQL: SELECT id, val, on_day, ts, ts2 FROM public.test > > (val as local text, remote enum) > explain verbose select * from test where (val::text) like 'foo'; > > Foreign Scan on public.test (cost=100.00..122.90 rows=5 width=56) > Output: id, val, on_day, ts, ts2 > Remote SQL: SELECT id, val, on_day, ts, ts2 FROM public.test WHERE ((val ~~ 'foo')) > > explain verbose select * from test where (val::varchar(10)) like 'foo'; > > Foreign Scan on public.test (cost=100.00..125.46 rows=5 width=56) > Output: id, val, on_day, ts, ts2 > Remote SQL: SELECT id, val, on_day, ts, ts2 FROM public.test WHERE ((val::character varying(10) ~~ 'foo')) Outside that, deparseConst also contains a note about keeping the code in sync with the parser (make_const in particular); from what I could tell, I don't think there's anything in this that necessitates changes there.
Вложения
"Dian M Fay" <dian.m.fay@gmail.com> writes:
> Thanks Tom, that makes way more sense! I've attached a new patch which
> tests operands and makes sure one side is a Const before feeding it to
> deparseConst with a new showtype code, -2. The one regression is gone,
> but I've left a couple of test output discrepancies for now which
> showcase lost casts on the following predicates:
> * date(c5) = '1970-01-17'::date
> * ctid = '(0,2)'::tid
> These aren't exactly failures -- both implicit string comparisons work
> just fine -- but I don't know Postgres well enough to be sure that
> that's true more generally.
These seem fine to me. The parser heuristic that we're relying on
acts at the level of the operator --- it doesn't really care whether
the other input argument is a simple Var or not.
Note that we're *not* doing an "implicit string comparison" in either
case. The point here is that the remote parser will resolve the
unknown-type literal as being the same type as the other operator input,
that is date or tid in these two cases.
That being the goal, I think you don't have the logic right at all,
even if it happens to accidentally work in the tested cases. We
can only drop the cast if it's a binary operator and the two inputs
are of the same type. Testing "leftType == form->oprleft" is pretty
close to a no-op, because the input will have been coerced to the
operator's expected type. And the code as you had it could do
indefensible things with a unary operator. (It's probably hard to
get here with a unary operator applied to a constant, but I'm not
sure it's impossible.)
Attached is a rewrite that does what I think we want to do, and
also adds comments because there weren't any.
Now that I've looked this over I'm starting to feel uncomfortable
again, because we can't actually be quite sure about how the remote
parser's heuristic will act. What we're checking is that leftType
and rightType match, but that condition is applied to the inputs
*after implicit type coercion to the operator's input types*.
We can't be entirely sure about what our parser saw to begin with.
Perhaps it'd be a good idea to strip any implicit coercions on
the non-Const input before checking its type. I'm not sure how
much that helps though. For one thing, by the time this code
sees the expression, eval_const_expressions could have collapsed
coercion steps in a way that obscures how it looked originally.
For another thing, in the cases we're interested in, it's kind of
a stretch to suppose that implicit coercions applied locally are
a good model of the way things will look to the remote parser.
So I'm feeling a bit itchy. I'm still willing to push forward
with this, but I won't be terribly surprised if it breaks cases
that ought to work and we end up having to revert it.
regards, tom lane
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index d98bd66681..67d397c354 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -2695,9 +2695,12 @@ deparseVar(Var *node, deparse_expr_cxt *context)
* Deparse given constant value into context->buf.
*
* This function has to be kept in sync with ruleutils.c's get_const_expr.
- * As for that function, showtype can be -1 to never show "::typename" decoration,
- * or +1 to always show it, or 0 to show it only if the constant wouldn't be assumed
- * to be the right type by default.
+ * As in that function, showtype can be -1 to never show "::typename"
+ * decoration, or +1 to always show it, or 0 to show it only if the constant
+ * wouldn't be assumed to be the right type by default. In addition,
+ * this code allows showtype to be -2 to indicate that we should not show
+ * "::typename" decoration if the constant is printed as an untyped literal
+ * or NULL (while in other cases, behaving as for showtype == 0).
*/
static void
deparseConst(Const *node, deparse_expr_cxt *context, int showtype)
@@ -2707,6 +2710,7 @@ deparseConst(Const *node, deparse_expr_cxt *context, int showtype)
bool typIsVarlena;
char *extval;
bool isfloat = false;
+ bool isstring = false;
bool needlabel;
if (node->constisnull)
@@ -2762,13 +2766,14 @@ deparseConst(Const *node, deparse_expr_cxt *context, int showtype)
break;
default:
deparseStringLiteral(buf, extval);
+ isstring = true;
break;
}
pfree(extval);
- if (showtype < 0)
- return;
+ if (showtype == -1)
+ return; /* never print type label */
/*
* For showtype == 0, append ::typename unless the constant will be
@@ -2788,7 +2793,13 @@ deparseConst(Const *node, deparse_expr_cxt *context, int showtype)
needlabel = !isfloat || (node->consttypmod >= 0);
break;
default:
- needlabel = true;
+ if (showtype == -2)
+ {
+ /* label unless we printed it as an untyped string */
+ needlabel = !isstring;
+ }
+ else
+ needlabel = true;
break;
}
if (needlabel || showtype > 0)
@@ -2953,6 +2964,8 @@ deparseOpExpr(OpExpr *node, deparse_expr_cxt *context)
StringInfo buf = context->buf;
HeapTuple tuple;
Form_pg_operator form;
+ Expr *right;
+ bool treatRightConstSpecially = false;
char oprkind;
/* Retrieve information about the operator from system catalog. */
@@ -2966,13 +2979,51 @@ deparseOpExpr(OpExpr *node, deparse_expr_cxt *context)
Assert((oprkind == 'l' && list_length(node->args) == 1) ||
(oprkind == 'b' && list_length(node->args) == 2));
+ right = llast(node->args);
+
/* Always parenthesize the expression. */
appendStringInfoChar(buf, '(');
/* Deparse left operand, if any. */
if (oprkind == 'b')
{
- deparseExpr(linitial(node->args), context);
+ Expr *left = linitial(node->args);
+ Oid leftType = exprType((Node *) left);
+ Oid rightType = exprType((Node *) right);
+ bool treatLeftConstSpecially = false;
+
+ /*
+ * When considering a binary operator, if one input is a Const that
+ * can be printed as a bare string literal or NULL (i.e., it will look
+ * like type UNKNOWN to the remote parser), while the other input
+ * isn't a Const, we prefer to suppress the explicit cast that would
+ * normally get attached to the Const. We can do so if its type is
+ * the same as the other input's type. This relies on the remote
+ * parser applying the heuristic that says to resolve ambiguous
+ * operator calls with one unknown and one known input type by
+ * assuming that the unknown input is of the same type as the known
+ * input.
+ *
+ * Doing things this way allows some cases to succeed where a remote
+ * table's column is not of the identical type it was declared as in
+ * the local foreign table. By emitting, say, "foreigncol = 'foo'"
+ * not "foreigncol = 'foo'::text", we allow the remote parser to pick
+ * an "=" operator that's compatible with whatever the column really
+ * is remotely.
+ */
+ if (leftType == rightType)
+ {
+ if (IsA(left, Const) && !IsA(right, Const))
+ treatLeftConstSpecially = true;
+ else if (IsA(right, Const) && !IsA(left, Const))
+ treatRightConstSpecially = true;
+ }
+
+ if (treatLeftConstSpecially)
+ deparseConst((Const *) left, context, -2);
+ else
+ deparseExpr(left, context);
+
appendStringInfoChar(buf, ' ');
}
@@ -2981,7 +3032,11 @@ deparseOpExpr(OpExpr *node, deparse_expr_cxt *context)
/* Deparse right operand. */
appendStringInfoChar(buf, ' ');
- deparseExpr(llast(node->args), context);
+
+ if (treatRightConstSpecially)
+ deparseConst((Const *) right, context, -2);
+ else
+ deparseExpr(right, context);
appendStringInfoChar(buf, ')');
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index fd141a0fa5..8b4305ef88 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -341,11 +341,11 @@ SELECT * FROM ft1 WHERE false;
-- with WHERE clause
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE t1.c1 = 101 AND t1.c6 = '1' AND t1.c7 >= '1';
- QUERY PLAN
-------------------------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+----------------------------------------------------------------------------------------------------------------------------------
Foreign Scan on public.ft1 t1
Output: c1, c2, c3, c4, c5, c6, c7, c8
- Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((c7 >= '1'::bpchar)) AND (("C 1" =
101))AND ((c6 = '1'::text))
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((c7 >= '1')) AND (("C 1" = 101)) AND
((c6= '1'))
(3 rows)
SELECT * FROM ft1 t1 WHERE t1.c1 = 101 AND t1.c6 = '1' AND t1.c7 >= '1';
@@ -707,11 +707,11 @@ EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c1 = (ARRAY[c1,c2,3])[1]
(3 rows)
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c6 = E'foo''s\\bar'; -- check special chars
- QUERY PLAN
--------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------------
Foreign Scan on public.ft1 t1
Output: c1, c2, c3, c4, c5, c6, c7, c8
- Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((c6 = E'foo''s\\bar'::text))
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((c6 = E'foo''s\\bar'))
(3 rows)
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c8 = 'foo'; -- can't be sent to remote
@@ -1130,20 +1130,20 @@ SELECT * FROM ft1 WHERE c1 > (CASE random()::integer WHEN 0 THEN 1 WHEN 2 THEN 5
-- these are shippable
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 WHERE CASE c6 WHEN 'foo' THEN true ELSE c3 < 'bar' END;
- QUERY PLAN
---------------------------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+--------------------------------------------------------------------------------------------------------------------------------------------
Foreign Scan on public.ft1
Output: c1, c2, c3, c4, c5, c6, c7, c8
- Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((CASE c6 WHEN 'foo'::text THEN true
ELSE(c3 < 'bar'::text) END))
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((CASE c6 WHEN 'foo'::text THEN true
ELSE(c3 < 'bar') END))
(3 rows)
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 WHERE CASE c3 WHEN c6 THEN true ELSE c3 < 'bar' END;
- QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------------------------------
Foreign Scan on public.ft1
Output: c1, c2, c3, c4, c5, c6, c7, c8
- Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((CASE c3 WHEN c6 THEN true ELSE (c3 <
'bar'::text)END))
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((CASE c3 WHEN c6 THEN true ELSE (c3 <
'bar')END))
(3 rows)
-- but this is not because of collation
@@ -3491,12 +3491,12 @@ ORDER BY ref_0."C 1";
Index Cond: (ref_0."C 1" < 10)
-> Foreign Scan on public.ft1 ref_1
Output: ref_1.c3, ref_0.c2
- Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE ((c3 = '00001'::text))
+ Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE ((c3 = '00001'))
-> Materialize
Output: ref_3.c3
-> Foreign Scan on public.ft2 ref_3
Output: ref_3.c3
- Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE ((c3 = '00001'::text))
+ Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE ((c3 = '00001'))
(15 rows)
SELECT ref_0.c2, subq_1.*
@@ -3841,8 +3841,8 @@ EXECUTE st2(101, 121);
-- subquery using immutable function (can be sent to remote)
PREPARE st3(int) AS SELECT * FROM ft1 t1 WHERE t1.c1 < $2 AND t1.c3 IN (SELECT c3 FROM ft2 t2 WHERE c1 > $1 AND
date(c5)= '1970-01-17'::date) ORDER BY c1;
EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st3(10, 20);
- QUERY PLAN
------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------------
Sort
Output: t1.c1, t1.c2, t1.c3, t1.c4, t1.c5, t1.c6, t1.c7, t1.c8
Sort Key: t1.c1
@@ -3856,7 +3856,7 @@ EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st3(10, 20);
Output: t2.c3
-> Foreign Scan on public.ft2 t2
Output: t2.c3
- Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE (("C 1" > 10)) AND ((date(c5) = '1970-01-17'::date))
+ Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE (("C 1" > 10)) AND ((date(c5) = '1970-01-17'))
(14 rows)
EXECUTE st3(10, 20);
@@ -4114,11 +4114,11 @@ SELECT tableoid::regclass, * FROM ft1 t1 LIMIT 1;
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 t1 WHERE t1.ctid = '(0,2)';
- QUERY PLAN
--------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+--------------------------------------------------------------------------------------------------
Foreign Scan on public.ft1 t1
Output: c1, c2, c3, c4, c5, c6, c7, c8
- Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((ctid = '(0,2)'::tid))
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((ctid = '(0,2)'))
(3 rows)
SELECT * FROM ft1 t1 WHERE t1.ctid = '(0,2)';
@@ -4205,6 +4205,53 @@ ERROR: invalid input syntax for type integer: "foo"
CONTEXT: column "c8" of foreign table "ft1"
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
-- ===================================================================
+-- local type can be different from remote type in some cases,
+-- in particular if similarly-named operators do equivalent things
+-- ===================================================================
+ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE text;
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM ft1 WHERE c8 = 'foo' LIMIT 1;
+ QUERY PLAN
+--------------------------------------------------------------------------------------------------------------
+ Foreign Scan on public.ft1
+ Output: c1, c2, c3, c4, c5, c6, c7, c8
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((c8 = 'foo')) LIMIT 1::bigint
+(3 rows)
+
+SELECT * FROM ft1 WHERE c8 = 'foo' LIMIT 1;
+ c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8
+----+----+-------+------------------------------+--------------------------+----+------------+-----
+ 1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM ft1 WHERE 'foo' = c8 LIMIT 1;
+ QUERY PLAN
+--------------------------------------------------------------------------------------------------------------
+ Foreign Scan on public.ft1
+ Output: c1, c2, c3, c4, c5, c6, c7, c8
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (('foo' = c8)) LIMIT 1::bigint
+(3 rows)
+
+SELECT * FROM ft1 WHERE 'foo' = c8 LIMIT 1;
+ c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8
+----+----+-------+------------------------------+--------------------------+----+------------+-----
+ 1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo
+(1 row)
+
+-- we declared c8 to be text locally, but it's still the same type on
+-- the remote which will balk if we try to do anything incompatible
+-- with that remote type
+SELECT * FROM ft1 WHERE c8 LIKE 'foo' LIMIT 1; -- ERROR
+ERROR: operator does not exist: public.user_enum ~~ unknown
+HINT: No operator matches the given name and argument types. You might need to add explicit type casts.
+CONTEXT: remote SQL command: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((c8 ~~ 'foo')) LIMIT
1::bigint
+SELECT * FROM ft1 WHERE c8::text LIKE 'foo' LIMIT 1; -- ERROR
+ERROR: operator does not exist: public.user_enum ~~ unknown
+HINT: No operator matches the given name and argument types. You might need to add explicit type casts.
+CONTEXT: remote SQL command: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((c8 ~~ 'foo')) LIMIT
1::bigint
+ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
+-- ===================================================================
-- subtransaction
-- + local/remote error doesn't break cursor
-- ===================================================================
@@ -4254,35 +4301,35 @@ create foreign table ft3 (f1 text collate "C", f2 text, f3 varchar(10))
server loopback options (table_name 'loct3', use_remote_estimate 'true');
-- can be sent to remote
explain (verbose, costs off) select * from ft3 where f1 = 'foo';
- QUERY PLAN
-------------------------------------------------------------------------------
+ QUERY PLAN
+------------------------------------------------------------------------
Foreign Scan on public.ft3
Output: f1, f2, f3
- Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f1 = 'foo'::text))
+ Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f1 = 'foo'))
(3 rows)
explain (verbose, costs off) select * from ft3 where f1 COLLATE "C" = 'foo';
- QUERY PLAN
-------------------------------------------------------------------------------
+ QUERY PLAN
+------------------------------------------------------------------------
Foreign Scan on public.ft3
Output: f1, f2, f3
- Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f1 = 'foo'::text))
+ Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f1 = 'foo'))
(3 rows)
explain (verbose, costs off) select * from ft3 where f2 = 'foo';
- QUERY PLAN
-------------------------------------------------------------------------------
+ QUERY PLAN
+------------------------------------------------------------------------
Foreign Scan on public.ft3
Output: f1, f2, f3
- Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f2 = 'foo'::text))
+ Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f2 = 'foo'))
(3 rows)
explain (verbose, costs off) select * from ft3 where f3 = 'foo';
- QUERY PLAN
-------------------------------------------------------------------------------
+ QUERY PLAN
+------------------------------------------------------------------------
Foreign Scan on public.ft3
Output: f1, f2, f3
- Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f3 = 'foo'::text))
+ Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f3 = 'foo'))
(3 rows)
explain (verbose, costs off) select * from ft3 f, loct3 l
@@ -4384,22 +4431,22 @@ INSERT INTO ft2 (c1,c2,c3)
INSERT INTO ft2 (c1,c2,c3) VALUES (1104,204,'ddd'), (1105,205,'eee');
EXPLAIN (verbose, costs off)
UPDATE ft2 SET c2 = c2 + 300, c3 = c3 || '_update3' WHERE c1 % 10 = 3; -- can be pushed down
- QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+----------------------------------------------------------------------------------------------------------------
Update on public.ft2
-> Foreign Update on public.ft2
- Remote SQL: UPDATE "S 1"."T 1" SET c2 = (c2 + 300), c3 = (c3 || '_update3'::text) WHERE ((("C 1" % 10) = 3))
+ Remote SQL: UPDATE "S 1"."T 1" SET c2 = (c2 + 300), c3 = (c3 || '_update3') WHERE ((("C 1" % 10) = 3))
(3 rows)
UPDATE ft2 SET c2 = c2 + 300, c3 = c3 || '_update3' WHERE c1 % 10 = 3;
EXPLAIN (verbose, costs off)
UPDATE ft2 SET c2 = c2 + 400, c3 = c3 || '_update7' WHERE c1 % 10 = 7 RETURNING *; -- can be pushed down
- QUERY PLAN
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+------------------------------------------------------------------------------------------------------------------------------------------------------------
Update on public.ft2
Output: c1, c2, c3, c4, c5, c6, c7, c8
-> Foreign Update on public.ft2
- Remote SQL: UPDATE "S 1"."T 1" SET c2 = (c2 + 400), c3 = (c3 || '_update7'::text) WHERE ((("C 1" % 10) = 7))
RETURNING"C 1", c2, c3, c4, c5, c6, c7, c8
+ Remote SQL: UPDATE "S 1"."T 1" SET c2 = (c2 + 400), c3 = (c3 || '_update7') WHERE ((("C 1" % 10) = 7))
RETURNING"C 1", c2, c3, c4, c5, c6, c7, c8
(4 rows)
UPDATE ft2 SET c2 = c2 + 400, c3 = c3 || '_update7' WHERE c1 % 10 = 7 RETURNING *;
@@ -4512,11 +4559,11 @@ UPDATE ft2 SET c2 = c2 + 400, c3 = c3 || '_update7' WHERE c1 % 10 = 7 RETURNING
EXPLAIN (verbose, costs off)
UPDATE ft2 SET c2 = ft2.c2 + 500, c3 = ft2.c3 || '_update9', c7 = DEFAULT
FROM ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 9; -- can be pushed down
- QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Update on public.ft2
-> Foreign Update
- Remote SQL: UPDATE "S 1"."T 1" r1 SET c2 = (r1.c2 + 500), c3 = (r1.c3 || '_update9'::text), c7 = 'ft2
'::character(10)FROM "S 1"."T 1" r2 WHERE ((r1.c2 = r2."C 1")) AND (((r2."C 1" % 10) = 9))
+ Remote SQL: UPDATE "S 1"."T 1" r1 SET c2 = (r1.c2 + 500), c3 = (r1.c3 || '_update9'), c7 = 'ft2
'::character(10)FROM "S 1"."T 1" r2 WHERE ((r1.c2 = r2."C 1")) AND (((r2."C 1" % 10) = 9))
(3 rows)
UPDATE ft2 SET c2 = ft2.c2 + 500, c3 = ft2.c3 || '_update9', c7 = DEFAULT
@@ -8129,7 +8176,7 @@ select tableoid::regclass, * FROM locp;
update utrtest set a = 2 where b = 'foo' returning *;
ERROR: new row for relation "loct" violates check constraint "loct_a_check"
DETAIL: Failing row contains (2, foo).
-CONTEXT: remote SQL command: UPDATE public.loct SET a = 2 WHERE ((b = 'foo'::text)) RETURNING a, b
+CONTEXT: remote SQL command: UPDATE public.loct SET a = 2 WHERE ((b = 'foo')) RETURNING a, b
-- But the reverse is allowed
update utrtest set a = 1 where b = 'qux' returning *;
ERROR: cannot route tuples into foreign table to be updated "remp"
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 43c30d492d..654db23844 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1167,6 +1167,24 @@ SELECT sum(c2), array_agg(c8) FROM ft1 GROUP BY c8; -- ERROR
ANALYZE ft1; -- ERROR
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
+-- ===================================================================
+-- local type can be different from remote type in some cases,
+-- in particular if similarly-named operators do equivalent things
+-- ===================================================================
+ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE text;
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM ft1 WHERE c8 = 'foo' LIMIT 1;
+SELECT * FROM ft1 WHERE c8 = 'foo' LIMIT 1;
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM ft1 WHERE 'foo' = c8 LIMIT 1;
+SELECT * FROM ft1 WHERE 'foo' = c8 LIMIT 1;
+-- we declared c8 to be text locally, but it's still the same type on
+-- the remote which will balk if we try to do anything incompatible
+-- with that remote type
+SELECT * FROM ft1 WHERE c8 LIKE 'foo' LIMIT 1; -- ERROR
+SELECT * FROM ft1 WHERE c8::text LIKE 'foo' LIMIT 1; -- ERROR
+ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
+
-- ===================================================================
-- subtransaction
-- + local/remote error doesn't break cursor
I wrote:
> Now that I've looked this over I'm starting to feel uncomfortable
> again, because we can't actually be quite sure about how the remote
> parser's heuristic will act.
Actually ... we could make that a lot safer by insisting that the
other input be a plain Var, which'd necessarily be a column of the
foreign table. That would still cover most cases of practical
interest, I think, and it would remove any question of whether
implicit coercions had snuck in. It's more restrictive than I'd
really like, but I think it's less likely to cause problems.
regards, tom lane
On Tue Nov 2, 2021 at 7:10 PM EDT, Tom Lane wrote: > I wrote: > > Now that I've looked this over I'm starting to feel uncomfortable > > again, because we can't actually be quite sure about how the remote > > parser's heuristic will act. > > Actually ... we could make that a lot safer by insisting that the > other input be a plain Var, which'd necessarily be a column of the > foreign table. That would still cover most cases of practical > interest, I think, and it would remove any question of whether > implicit coercions had snuck in. It's more restrictive than I'd > really like, but I think it's less likely to cause problems. Here's v6! I started with restricting cast suppression to Const-Var comparisons as you suggested. A few tests did regress (relative to the unrestricted version) right out of the gate with comparisons to varchar columns, since those become RelabelType nodes instead of Vars. After reading the notes on RelabelType in primnodes.h, I *think* that that "dummy" coercion is distinct from the operator input type coercion you're talking about here: > What we're checking is that leftType and rightType match, but that > condition is applied to the inputs *after implicit type coercion to > the operator's input types*. We can't be entirely sure about what our > parser saw to begin with. Perhaps it'd be a good idea to strip any > implicit coercions on the non-Const input before checking its type. I allowed RelabelTypes over Vars to suppress casts as well. It's working for me so far and the varchar comparison tests are back to passing, sans casts.
Вложения
"Dian M Fay" <dian.m.fay@gmail.com> writes:
> On Tue Nov 2, 2021 at 7:10 PM EDT, Tom Lane wrote:
>> Actually ... we could make that a lot safer by insisting that the
>> other input be a plain Var, which'd necessarily be a column of the
>> foreign table. That would still cover most cases of practical
>> interest, I think, and it would remove any question of whether
>> implicit coercions had snuck in. It's more restrictive than I'd
>> really like, but I think it's less likely to cause problems.
> I allowed RelabelTypes over Vars to suppress casts as well. It's working
> for me so far and the varchar comparison tests are back to passing, sans
> casts.
Um. I doubt that that's any safer than the v5 patch. As an example,
casting between int4 and oid is just a RelabelType, but the comparison
semantics change completely (signed vs. unsigned); so there's not a
good reason to think this is constraining things more than v5 did.
It might be better if you'd further restricted the structure to be only
COERCE_IMPLICIT_CAST RelabelTypes, since we don't normally make casts
implicit if they significantly change semantics. Also, this'd ensure
that the operand printed for the remote server is just a bare Var
(cf. deparseRelabelType). But even with that I'm feeling antsy about
whether this will allow any semantic surprises.
regards, tom lane
On Mon Nov 8, 2021 at 4:50 PM EST, Tom Lane wrote: > Um. I doubt that that's any safer than the v5 patch. As an example, > casting between int4 and oid is just a RelabelType, but the comparison > semantics change completely (signed vs. unsigned); so there's not a > good reason to think this is constraining things more than v5 did. > > It might be better if you'd further restricted the structure to be only > COERCE_IMPLICIT_CAST RelabelTypes, since we don't normally make casts > implicit if they significantly change semantics. Also, this'd ensure > that the operand printed for the remote server is just a bare Var > (cf. deparseRelabelType). But even with that I'm feeling antsy about > whether this will allow any semantic surprises. I've split the suppression for RelabelTypes with implicit cast check into a second patch over the core v7 change. As far as testing goes, \dC lists implicit casts, but most of those I've tried seem to wind up deparsing as Vars. I've been able to manifest RelabelTypes with varchar, cidr, and remote char to local varchar, but that's about it. Any ideas for validating it further, off the top of your head?
Вложения
"Dian M Fay" <dian.m.fay@gmail.com> writes:
> I've split the suppression for RelabelTypes with implicit cast check
> into a second patch over the core v7 change. As far as testing goes, \dC
> lists implicit casts, but most of those I've tried seem to wind up
> deparsing as Vars. I've been able to manifest RelabelTypes with varchar,
> cidr, and remote char to local varchar, but that's about it. Any ideas
> for validating it further, off the top of your head?
I thought about this some more and realized exactly why I wanted to
restrict the change to cases where the other side is a plain foreign Var:
that way, if anything surprising happens, we can blame it directly on the
user having declared a local column with a different type from the
remote column.
That being the case, I took a closer look at deparseVar and realized that
we can't simply check "IsA(node, Var)": some Vars in the expression can
belong to local tables. We need to verify that the Var is one that will
print as a remote column reference.
So that leads me to v8, attached. I think we are getting there.
regards, tom lane
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index d98bd66681..b27689d086 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -152,6 +152,7 @@ static void deparseParam(Param *node, deparse_expr_cxt *context);
static void deparseSubscriptingRef(SubscriptingRef *node, deparse_expr_cxt *context);
static void deparseFuncExpr(FuncExpr *node, deparse_expr_cxt *context);
static void deparseOpExpr(OpExpr *node, deparse_expr_cxt *context);
+static bool isPlainForeignVar(Expr *node, deparse_expr_cxt *context);
static void deparseOperatorName(StringInfo buf, Form_pg_operator opform);
static void deparseDistinctExpr(DistinctExpr *node, deparse_expr_cxt *context);
static void deparseScalarArrayOpExpr(ScalarArrayOpExpr *node,
@@ -2695,9 +2696,14 @@ deparseVar(Var *node, deparse_expr_cxt *context)
* Deparse given constant value into context->buf.
*
* This function has to be kept in sync with ruleutils.c's get_const_expr.
- * As for that function, showtype can be -1 to never show "::typename" decoration,
- * or +1 to always show it, or 0 to show it only if the constant wouldn't be assumed
- * to be the right type by default.
+ *
+ * As in that function, showtype can be -1 to never show "::typename"
+ * decoration, +1 to always show it, or 0 to show it only if the constant
+ * wouldn't be assumed to be the right type by default.
+ *
+ * In addition, this code allows showtype to be -2 to indicate that we should
+ * not show "::typename" decoration if the constant is printed as an untyped
+ * literal or NULL (while in other cases, behaving as for showtype == 0).
*/
static void
deparseConst(Const *node, deparse_expr_cxt *context, int showtype)
@@ -2707,6 +2713,7 @@ deparseConst(Const *node, deparse_expr_cxt *context, int showtype)
bool typIsVarlena;
char *extval;
bool isfloat = false;
+ bool isstring = false;
bool needlabel;
if (node->constisnull)
@@ -2762,13 +2769,14 @@ deparseConst(Const *node, deparse_expr_cxt *context, int showtype)
break;
default:
deparseStringLiteral(buf, extval);
+ isstring = true;
break;
}
pfree(extval);
- if (showtype < 0)
- return;
+ if (showtype == -1)
+ return; /* never print type label */
/*
* For showtype == 0, append ::typename unless the constant will be
@@ -2788,7 +2796,13 @@ deparseConst(Const *node, deparse_expr_cxt *context, int showtype)
needlabel = !isfloat || (node->consttypmod >= 0);
break;
default:
- needlabel = true;
+ if (showtype == -2)
+ {
+ /* label unless we printed it as an untyped string */
+ needlabel = !isstring;
+ }
+ else
+ needlabel = true;
break;
}
if (needlabel || showtype > 0)
@@ -2953,6 +2967,8 @@ deparseOpExpr(OpExpr *node, deparse_expr_cxt *context)
StringInfo buf = context->buf;
HeapTuple tuple;
Form_pg_operator form;
+ Expr *right;
+ bool canSuppressRightConstCast = false;
char oprkind;
/* Retrieve information about the operator from system catalog. */
@@ -2966,13 +2982,58 @@ deparseOpExpr(OpExpr *node, deparse_expr_cxt *context)
Assert((oprkind == 'l' && list_length(node->args) == 1) ||
(oprkind == 'b' && list_length(node->args) == 2));
+ right = llast(node->args);
+
/* Always parenthesize the expression. */
appendStringInfoChar(buf, '(');
/* Deparse left operand, if any. */
if (oprkind == 'b')
{
- deparseExpr(linitial(node->args), context);
+ Expr *left = linitial(node->args);
+ Oid leftType = exprType((Node *) left);
+ Oid rightType = exprType((Node *) right);
+ bool canSuppressLeftConstCast = false;
+
+ /*
+ * When considering a binary operator, if one operand is a Const that
+ * can be printed as a bare string literal or NULL (i.e., it will look
+ * like type UNKNOWN to the remote parser), the Const normally
+ * receives an explicit cast to the operator's input type. However,
+ * in Const-to-Var comparisons where both operands are of the same
+ * type, we prefer to suppress the explicit cast, leaving the Const's
+ * type resolution up to the remote parser. The remote's resolution
+ * heuristic will assume that an unknown input type being compared to
+ * a known input type is of that known type as well.
+ *
+ * This hack allows some cases to succeed where a remote column is
+ * declared with a different type in the local (foreign) table. By
+ * emitting "foreigncol = 'foo'" not "foreigncol = 'foo'::text" or the
+ * like, we allow the remote parser to pick an "=" operator that's
+ * compatible with whatever type the remote column really is, such as
+ * an enum.
+ *
+ * We allow cast suppression to happen only when the other operand is
+ * a plain foreign Var. Although the remote's unknown-type heuristic
+ * would apply to other cases just as well, we would be taking a
+ * bigger risk that the inferred type is something unexpected. With
+ * this restriction, if anything goes wrong it's the user's fault for
+ * not declaring the local column with the same type as the remote
+ * column.
+ */
+ if (leftType == rightType)
+ {
+ if (IsA(left, Const))
+ canSuppressLeftConstCast = isPlainForeignVar(right, context);
+ else if (IsA(right, Const))
+ canSuppressRightConstCast = isPlainForeignVar(left, context);
+ }
+
+ if (canSuppressLeftConstCast)
+ deparseConst((Const *) left, context, -2);
+ else
+ deparseExpr(left, context);
+
appendStringInfoChar(buf, ' ');
}
@@ -2981,13 +3042,52 @@ deparseOpExpr(OpExpr *node, deparse_expr_cxt *context)
/* Deparse right operand. */
appendStringInfoChar(buf, ' ');
- deparseExpr(llast(node->args), context);
+
+ if (canSuppressRightConstCast)
+ deparseConst((Const *) right, context, -2);
+ else
+ deparseExpr(right, context);
appendStringInfoChar(buf, ')');
ReleaseSysCache(tuple);
}
+/*
+ * Will "node" deparse as a plain foreign Var?
+ */
+static bool
+isPlainForeignVar(Expr *node, deparse_expr_cxt *context)
+{
+ /*
+ * We allow the foreign Var to have an implicit RelabelType, mainly so
+ * that this'll work with varchar columns. Note that deparseRelabelType
+ * will not print such a cast, so we're not breaking the restriction that
+ * the expression print as a plain Var. We won't risk it for an implicit
+ * cast that requires a function, nor for non-implicit RelabelType; such
+ * cases seem too likely to involve semantics changes compared to what
+ * would happen on the remote side.
+ */
+ if (IsA(node, RelabelType) &&
+ ((RelabelType *) node)->relabelformat == COERCE_IMPLICIT_CAST)
+ node = ((RelabelType *) node)->arg;
+
+ if (IsA(node, Var))
+ {
+ /*
+ * The Var must be one that'll deparse as a foreign column reference
+ * (cf. deparseVar).
+ */
+ Var *var = (Var *) node;
+ Relids relids = context->scanrel->relids;
+
+ if (bms_is_member(var->varno, relids) && var->varlevelsup == 0)
+ return true;
+ }
+
+ return false;
+}
+
/*
* Print the name of an operator.
*/
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index fd141a0fa5..3cee0a8c12 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -341,11 +341,11 @@ SELECT * FROM ft1 WHERE false;
-- with WHERE clause
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE t1.c1 = 101 AND t1.c6 = '1' AND t1.c7 >= '1';
- QUERY PLAN
-------------------------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+----------------------------------------------------------------------------------------------------------------------------------
Foreign Scan on public.ft1 t1
Output: c1, c2, c3, c4, c5, c6, c7, c8
- Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((c7 >= '1'::bpchar)) AND (("C 1" =
101))AND ((c6 = '1'::text))
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((c7 >= '1')) AND (("C 1" = 101)) AND
((c6= '1'))
(3 rows)
SELECT * FROM ft1 t1 WHERE t1.c1 = 101 AND t1.c6 = '1' AND t1.c7 >= '1';
@@ -707,11 +707,11 @@ EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c1 = (ARRAY[c1,c2,3])[1]
(3 rows)
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c6 = E'foo''s\\bar'; -- check special chars
- QUERY PLAN
--------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------------
Foreign Scan on public.ft1 t1
Output: c1, c2, c3, c4, c5, c6, c7, c8
- Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((c6 = E'foo''s\\bar'::text))
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((c6 = E'foo''s\\bar'))
(3 rows)
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c8 = 'foo'; -- can't be sent to remote
@@ -1130,20 +1130,20 @@ SELECT * FROM ft1 WHERE c1 > (CASE random()::integer WHEN 0 THEN 1 WHEN 2 THEN 5
-- these are shippable
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 WHERE CASE c6 WHEN 'foo' THEN true ELSE c3 < 'bar' END;
- QUERY PLAN
---------------------------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+--------------------------------------------------------------------------------------------------------------------------------------------
Foreign Scan on public.ft1
Output: c1, c2, c3, c4, c5, c6, c7, c8
- Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((CASE c6 WHEN 'foo'::text THEN true
ELSE(c3 < 'bar'::text) END))
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((CASE c6 WHEN 'foo'::text THEN true
ELSE(c3 < 'bar') END))
(3 rows)
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 WHERE CASE c3 WHEN c6 THEN true ELSE c3 < 'bar' END;
- QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------------------------------
Foreign Scan on public.ft1
Output: c1, c2, c3, c4, c5, c6, c7, c8
- Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((CASE c3 WHEN c6 THEN true ELSE (c3 <
'bar'::text)END))
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((CASE c3 WHEN c6 THEN true ELSE (c3 <
'bar')END))
(3 rows)
-- but this is not because of collation
@@ -3491,12 +3491,12 @@ ORDER BY ref_0."C 1";
Index Cond: (ref_0."C 1" < 10)
-> Foreign Scan on public.ft1 ref_1
Output: ref_1.c3, ref_0.c2
- Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE ((c3 = '00001'::text))
+ Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE ((c3 = '00001'))
-> Materialize
Output: ref_3.c3
-> Foreign Scan on public.ft2 ref_3
Output: ref_3.c3
- Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE ((c3 = '00001'::text))
+ Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE ((c3 = '00001'))
(15 rows)
SELECT ref_0.c2, subq_1.*
@@ -4114,11 +4114,11 @@ SELECT tableoid::regclass, * FROM ft1 t1 LIMIT 1;
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 t1 WHERE t1.ctid = '(0,2)';
- QUERY PLAN
--------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+--------------------------------------------------------------------------------------------------
Foreign Scan on public.ft1 t1
Output: c1, c2, c3, c4, c5, c6, c7, c8
- Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((ctid = '(0,2)'::tid))
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((ctid = '(0,2)'))
(3 rows)
SELECT * FROM ft1 t1 WHERE t1.ctid = '(0,2)';
@@ -4205,6 +4205,53 @@ ERROR: invalid input syntax for type integer: "foo"
CONTEXT: column "c8" of foreign table "ft1"
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
-- ===================================================================
+-- local type can be different from remote type in some cases,
+-- in particular if similarly-named operators do equivalent things
+-- ===================================================================
+ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE text;
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM ft1 WHERE c8 = 'foo' LIMIT 1;
+ QUERY PLAN
+--------------------------------------------------------------------------------------------------------------
+ Foreign Scan on public.ft1
+ Output: c1, c2, c3, c4, c5, c6, c7, c8
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((c8 = 'foo')) LIMIT 1::bigint
+(3 rows)
+
+SELECT * FROM ft1 WHERE c8 = 'foo' LIMIT 1;
+ c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8
+----+----+-------+------------------------------+--------------------------+----+------------+-----
+ 1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo
+(1 row)
+
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM ft1 WHERE 'foo' = c8 LIMIT 1;
+ QUERY PLAN
+--------------------------------------------------------------------------------------------------------------
+ Foreign Scan on public.ft1
+ Output: c1, c2, c3, c4, c5, c6, c7, c8
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (('foo' = c8)) LIMIT 1::bigint
+(3 rows)
+
+SELECT * FROM ft1 WHERE 'foo' = c8 LIMIT 1;
+ c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8
+----+----+-------+------------------------------+--------------------------+----+------------+-----
+ 1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo
+(1 row)
+
+-- we declared c8 to be text locally, but it's still the same type on
+-- the remote which will balk if we try to do anything incompatible
+-- with that remote type
+SELECT * FROM ft1 WHERE c8 LIKE 'foo' LIMIT 1; -- ERROR
+ERROR: operator does not exist: public.user_enum ~~ unknown
+HINT: No operator matches the given name and argument types. You might need to add explicit type casts.
+CONTEXT: remote SQL command: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((c8 ~~ 'foo')) LIMIT
1::bigint
+SELECT * FROM ft1 WHERE c8::text LIKE 'foo' LIMIT 1; -- ERROR; cast not pushed down
+ERROR: operator does not exist: public.user_enum ~~ unknown
+HINT: No operator matches the given name and argument types. You might need to add explicit type casts.
+CONTEXT: remote SQL command: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((c8 ~~ 'foo')) LIMIT
1::bigint
+ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
+-- ===================================================================
-- subtransaction
-- + local/remote error doesn't break cursor
-- ===================================================================
@@ -4254,35 +4301,35 @@ create foreign table ft3 (f1 text collate "C", f2 text, f3 varchar(10))
server loopback options (table_name 'loct3', use_remote_estimate 'true');
-- can be sent to remote
explain (verbose, costs off) select * from ft3 where f1 = 'foo';
- QUERY PLAN
-------------------------------------------------------------------------------
+ QUERY PLAN
+------------------------------------------------------------------------
Foreign Scan on public.ft3
Output: f1, f2, f3
- Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f1 = 'foo'::text))
+ Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f1 = 'foo'))
(3 rows)
explain (verbose, costs off) select * from ft3 where f1 COLLATE "C" = 'foo';
- QUERY PLAN
-------------------------------------------------------------------------------
+ QUERY PLAN
+------------------------------------------------------------------------
Foreign Scan on public.ft3
Output: f1, f2, f3
- Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f1 = 'foo'::text))
+ Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f1 = 'foo'))
(3 rows)
explain (verbose, costs off) select * from ft3 where f2 = 'foo';
- QUERY PLAN
-------------------------------------------------------------------------------
+ QUERY PLAN
+------------------------------------------------------------------------
Foreign Scan on public.ft3
Output: f1, f2, f3
- Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f2 = 'foo'::text))
+ Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f2 = 'foo'))
(3 rows)
explain (verbose, costs off) select * from ft3 where f3 = 'foo';
- QUERY PLAN
-------------------------------------------------------------------------------
+ QUERY PLAN
+------------------------------------------------------------------------
Foreign Scan on public.ft3
Output: f1, f2, f3
- Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f3 = 'foo'::text))
+ Remote SQL: SELECT f1, f2, f3 FROM public.loct3 WHERE ((f3 = 'foo'))
(3 rows)
explain (verbose, costs off) select * from ft3 f, loct3 l
@@ -4384,22 +4431,22 @@ INSERT INTO ft2 (c1,c2,c3)
INSERT INTO ft2 (c1,c2,c3) VALUES (1104,204,'ddd'), (1105,205,'eee');
EXPLAIN (verbose, costs off)
UPDATE ft2 SET c2 = c2 + 300, c3 = c3 || '_update3' WHERE c1 % 10 = 3; -- can be pushed down
- QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+----------------------------------------------------------------------------------------------------------------
Update on public.ft2
-> Foreign Update on public.ft2
- Remote SQL: UPDATE "S 1"."T 1" SET c2 = (c2 + 300), c3 = (c3 || '_update3'::text) WHERE ((("C 1" % 10) = 3))
+ Remote SQL: UPDATE "S 1"."T 1" SET c2 = (c2 + 300), c3 = (c3 || '_update3') WHERE ((("C 1" % 10) = 3))
(3 rows)
UPDATE ft2 SET c2 = c2 + 300, c3 = c3 || '_update3' WHERE c1 % 10 = 3;
EXPLAIN (verbose, costs off)
UPDATE ft2 SET c2 = c2 + 400, c3 = c3 || '_update7' WHERE c1 % 10 = 7 RETURNING *; -- can be pushed down
- QUERY PLAN
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+------------------------------------------------------------------------------------------------------------------------------------------------------------
Update on public.ft2
Output: c1, c2, c3, c4, c5, c6, c7, c8
-> Foreign Update on public.ft2
- Remote SQL: UPDATE "S 1"."T 1" SET c2 = (c2 + 400), c3 = (c3 || '_update7'::text) WHERE ((("C 1" % 10) = 7))
RETURNING"C 1", c2, c3, c4, c5, c6, c7, c8
+ Remote SQL: UPDATE "S 1"."T 1" SET c2 = (c2 + 400), c3 = (c3 || '_update7') WHERE ((("C 1" % 10) = 7))
RETURNING"C 1", c2, c3, c4, c5, c6, c7, c8
(4 rows)
UPDATE ft2 SET c2 = c2 + 400, c3 = c3 || '_update7' WHERE c1 % 10 = 7 RETURNING *;
@@ -4512,11 +4559,11 @@ UPDATE ft2 SET c2 = c2 + 400, c3 = c3 || '_update7' WHERE c1 % 10 = 7 RETURNING
EXPLAIN (verbose, costs off)
UPDATE ft2 SET c2 = ft2.c2 + 500, c3 = ft2.c3 || '_update9', c7 = DEFAULT
FROM ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 9; -- can be pushed down
- QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Update on public.ft2
-> Foreign Update
- Remote SQL: UPDATE "S 1"."T 1" r1 SET c2 = (r1.c2 + 500), c3 = (r1.c3 || '_update9'::text), c7 = 'ft2
'::character(10)FROM "S 1"."T 1" r2 WHERE ((r1.c2 = r2."C 1")) AND (((r2."C 1" % 10) = 9))
+ Remote SQL: UPDATE "S 1"."T 1" r1 SET c2 = (r1.c2 + 500), c3 = (r1.c3 || '_update9'), c7 = 'ft2
'::character(10)FROM "S 1"."T 1" r2 WHERE ((r1.c2 = r2."C 1")) AND (((r2."C 1" % 10) = 9))
(3 rows)
UPDATE ft2 SET c2 = ft2.c2 + 500, c3 = ft2.c3 || '_update9', c7 = DEFAULT
@@ -8129,7 +8176,7 @@ select tableoid::regclass, * FROM locp;
update utrtest set a = 2 where b = 'foo' returning *;
ERROR: new row for relation "loct" violates check constraint "loct_a_check"
DETAIL: Failing row contains (2, foo).
-CONTEXT: remote SQL command: UPDATE public.loct SET a = 2 WHERE ((b = 'foo'::text)) RETURNING a, b
+CONTEXT: remote SQL command: UPDATE public.loct SET a = 2 WHERE ((b = 'foo')) RETURNING a, b
-- But the reverse is allowed
update utrtest set a = 1 where b = 'qux' returning *;
ERROR: cannot route tuples into foreign table to be updated "remp"
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 43c30d492d..e40112e41d 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1167,6 +1167,24 @@ SELECT sum(c2), array_agg(c8) FROM ft1 GROUP BY c8; -- ERROR
ANALYZE ft1; -- ERROR
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
+-- ===================================================================
+-- local type can be different from remote type in some cases,
+-- in particular if similarly-named operators do equivalent things
+-- ===================================================================
+ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE text;
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM ft1 WHERE c8 = 'foo' LIMIT 1;
+SELECT * FROM ft1 WHERE c8 = 'foo' LIMIT 1;
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM ft1 WHERE 'foo' = c8 LIMIT 1;
+SELECT * FROM ft1 WHERE 'foo' = c8 LIMIT 1;
+-- we declared c8 to be text locally, but it's still the same type on
+-- the remote which will balk if we try to do anything incompatible
+-- with that remote type
+SELECT * FROM ft1 WHERE c8 LIKE 'foo' LIMIT 1; -- ERROR
+SELECT * FROM ft1 WHERE c8::text LIKE 'foo' LIMIT 1; -- ERROR; cast not pushed down
+ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
+
-- ===================================================================
-- subtransaction
-- + local/remote error doesn't break cursor
On Thu Nov 11, 2021 at 3:36 PM EST, Tom Lane wrote: > I thought about this some more and realized exactly why I wanted to > restrict the change to cases where the other side is a plain foreign > Var: that way, if anything surprising happens, we can blame it > directly on the user having declared a local column with a different > type from the remote column. > > That being the case, I took a closer look at deparseVar and realized > that we can't simply check "IsA(node, Var)": some Vars in the > expression can belong to local tables. We need to verify that the Var > is one that will print as a remote column reference. Eminently reasonable all around! `git apply` insisted that the v8 patch didn't (apply, that is), but `patch -p1` liked it fine. I've put it through a few paces and it seems good; what needs to happen next?
"Dian M Fay" <dian.m.fay@gmail.com> writes:
> Eminently reasonable all around! `git apply` insisted that the v8 patch
> didn't (apply, that is), but `patch -p1` liked it fine. I've put it
> through a few paces and it seems good; what needs to happen next?
I don't see anything else to do except shove it out into the light
of day and see what happens. Hence, pushed.
As I remarked in the commit message:
>> One point that I (tgl) remain slightly uncomfortable with is that we
>> will ignore an implicit RelabelType when deciding if the non-Const input
>> is a plain Var. That makes it a little squishy to argue that the remote
>> should resolve the Const as being of the same type as its Var, because
>> then our Const is not the same type as our Var. However, if we don't do
>> that, then this hack won't work as desired if the user chooses to use
>> varchar rather than text to represent some remote column. That seems
>> useful, so do it like this for now. We might have to give up the
>> RelabelType-ignoring bit if any problems surface.
I think we can await complaints before doing more, but I wanted that
bit on record for anyone perusing the archives.
regards, tom lane