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.
From 3b950aa43af686acdbbaa5aada82bfed725652d7 Mon Sep 17 00:00:00 2001
From: Dian M Fay <dian.m....@gmail.com>
Date: Sun, 7 Mar 2021 02:59:14 -0500
Subject: [PATCH] Suppress explicit casts of text constants in postgres_fdw
when the other side of a binary OpExpr is also text.
---
contrib/postgres_fdw/deparse.c | 27 +++++-
.../postgres_fdw/expected/postgres_fdw.out | 93 ++++++++++++-------
contrib/postgres_fdw/sql/postgres_fdw.sql | 9 ++
3 files changed, 90 insertions(+), 39 deletions(-)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 2b8f00abe7..e056c5f117 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -101,6 +101,7 @@ typedef struct deparse_expr_cxt
* a base
relation. */
StringInfo buf; /* output buffer to append to */
List **params_list; /* exprs that will become remote Params
*/
+ int const_showtype; /* override showtype in
deparseConst */
} deparse_expr_cxt;
#define REL_ALIAS_PREFIX "r"
@@ -1146,6 +1147,7 @@ deparseSelectStmtForRel(StringInfo buf, PlannerInfo
*root, RelOptInfo *rel,
context.foreignrel = rel;
context.scanrel = IS_UPPER_REL(rel) ? fpinfo->outerrel : rel;
context.params_list = params_list;
+ context.const_showtype = 0;
/* Construct SELECT clause */
deparseSelectSql(tlist, is_subquery, retrieved_attrs, &context);
@@ -1725,6 +1727,7 @@ deparseFromExprForRel(StringInfo buf, PlannerInfo *root,
RelOptInfo *foreignrel,
context.scanrel = foreignrel;
context.root = root;
context.params_list = params_list;
+ context.const_showtype = 0;
appendStringInfoChar(buf, '(');
appendConditions(fpinfo->joinclauses, &context);
@@ -2028,6 +2031,7 @@ deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
context.scanrel = foreignrel;
context.buf = buf;
context.params_list = params_list;
+ context.const_showtype = 0;
appendStringInfoString(buf, "UPDATE ");
deparseRelation(buf, rel);
@@ -2135,6 +2139,7 @@ deparseDirectDeleteSql(StringInfo buf, PlannerInfo *root,
context.scanrel = foreignrel;
context.buf = buf;
context.params_list = params_list;
+ context.const_showtype = 0;
appendStringInfoString(buf, "DELETE FROM ");
deparseRelation(buf, rel);
@@ -2554,7 +2559,7 @@ deparseExpr(Expr *node, deparse_expr_cxt *context)
deparseVar((Var *) node, context);
break;
case T_Const:
- deparseConst((Const *) node, context, 0);
+ deparseConst((Const *) node, context,
context->const_showtype);
break;
case T_Param:
deparseParam((Param *) node, context);
@@ -2927,6 +2932,9 @@ deparseOpExpr(OpExpr *node, deparse_expr_cxt *context)
StringInfo buf = context->buf;
HeapTuple tuple;
Form_pg_operator form;
+ Expr *left;
+ Expr *right;
+ Oid rightType;
char oprkind;
/* Retrieve information about the operator from system catalog. */
@@ -2943,11 +2951,21 @@ deparseOpExpr(OpExpr *node, deparse_expr_cxt *context)
/* Always parenthesize the expression. */
appendStringInfoChar(buf, '(');
+ right = llast(node->args);
+
/* Deparse left operand, if any. */
if (oprkind == 'b')
{
- deparseExpr(linitial(node->args), context);
+ left = linitial(node->args);
+
+ deparseExpr(left, context);
appendStringInfoChar(buf, ' ');
+ rightType = exprType((Node *) right);
+
+ if (rightType == TEXTOID && rightType == exprType((Node
*)left)) {
+ /* Suppress const casting for text values against text
columns. */
+ context->const_showtype = -1;
+ }
}
/* Deparse operator name. */
@@ -2955,11 +2973,14 @@ deparseOpExpr(OpExpr *node, deparse_expr_cxt *context)
/* Deparse right operand. */
appendStringInfoChar(buf, ' ');
- deparseExpr(llast(node->args), context);
+ deparseExpr(right, context);
appendStringInfoChar(buf, ')');
ReleaseSysCache(tuple);
+
+ /* Release cast suppression in case it was set. */
+ context->const_showtype = 0;
}
/*
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out
b/contrib/postgres_fdw/expected/postgres_fdw.out
index c7b83180a8..9c3256b5cd 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'::bpchar)) 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.*
@@ -4202,6 +4202,27 @@ ERROR: invalid input syntax for type integer: "foo"
CONTEXT: processing expression at position 2 in select list
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
-- ===================================================================
+-- conversion for const matching local type
+-- ===================================================================
+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)
+
+ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE text;
+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)
+
+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
+ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
+-- ===================================================================
-- subtransaction
-- + local/remote error doesn't break cursor
-- ===================================================================
@@ -4251,35 +4272,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
@@ -4381,22 +4402,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 *;
@@ -4509,11 +4530,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
@@ -8054,7 +8075,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 4c653f2473..dbaca0929e 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1166,6 +1166,15 @@ SELECT ftx.x1, ft2.c2, ftx FROM ft1
ftx(x1,x2,x3,x4,x5,x6,x7,x8), ft2
SELECT sum(c2), array_agg(c8) FROM ft1 GROUP BY c8; -- ERROR
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
+-- ===================================================================
+-- conversion for const matching local type
+-- ===================================================================
+SELECT * FROM ft1 WHERE c8 = 'foo' LIMIT 1;
+ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE text;
+SELECT * FROM ft1 WHERE c8 = 'foo' LIMIT 1;
+SELECT * FROM ft1 WHERE c8 LIKE 'foo' LIMIT 1; -- ERROR
+ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;
+
-- ===================================================================
-- subtransaction
-- + local/remote error doesn't break cursor
--
2.32.0
