postgres_fdw: Handle boolean comparison predicates
The comparison predicates IS [NOT] TRUE/FALSE/UNKNOWN were not
recognised by postgres_fdw, so they were not pushed down to the remote
server. The attached patch adds support for them.
I am adding this to the commitfest 2021-07.
Attachments:
0001-postgres_fdw-Handle-boolean-comparison-predicates.patchapplication/octet-stream; name=0001-postgres_fdw-Handle-boolean-comparison-predicates.patchDownload
From 6088fb84d26fe2140d2ac3d8001c9ae50a756394 Mon Sep 17 00:00:00 2001
From: Emre Hasegeli <emre@hasegeli.com>
Date: Mon, 31 May 2021 10:43:39 +0300
Subject: [PATCH] postgres_fdw: Handle boolean comparison predicates
The comparison predicates IS [NOT] TRUE/FALSE/UNKNOWN were not
recognised by postgres_fdw, so they were not pushed down to the remote
server. Handle them and deparse with more common syntax.
---
contrib/postgres_fdw/deparse.c | 67 +++++++++++++++++++
.../postgres_fdw/expected/postgres_fdw.out | 48 +++++++++++++
contrib/postgres_fdw/sql/postgres_fdw.sql | 6 ++
3 files changed, 121 insertions(+)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 31919fda8c..eae3f19276 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -151,20 +151,21 @@ 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 void deparseOperatorName(StringInfo buf, Form_pg_operator opform);
static void deparseDistinctExpr(DistinctExpr *node, deparse_expr_cxt *context);
static void deparseScalarArrayOpExpr(ScalarArrayOpExpr *node,
deparse_expr_cxt *context);
static void deparseRelabelType(RelabelType *node, deparse_expr_cxt *context);
static void deparseBoolExpr(BoolExpr *node, deparse_expr_cxt *context);
static void deparseNullTest(NullTest *node, deparse_expr_cxt *context);
+static void deparseBooleanTest(BooleanTest *node, deparse_expr_cxt *context);
static void deparseArrayExpr(ArrayExpr *node, deparse_expr_cxt *context);
static void printRemoteParam(int paramindex, Oid paramtype, int32 paramtypmod,
deparse_expr_cxt *context);
static void printRemotePlaceholder(Oid paramtype, int32 paramtypmod,
deparse_expr_cxt *context);
static void deparseSelectSql(List *tlist, bool is_subquery, List **retrieved_attrs,
deparse_expr_cxt *context);
static void deparseLockingClause(deparse_expr_cxt *context);
static void appendOrderByClause(List *pathkeys, bool has_final_sort,
deparse_expr_cxt *context);
@@ -636,20 +637,36 @@ foreign_expr_walker(Node *node,
*/
if (!foreign_expr_walker((Node *) nt->arg,
glob_cxt, &inner_cxt))
return false;
/* Output is always boolean and so noncollatable. */
collation = InvalidOid;
state = FDW_COLLATE_NONE;
}
break;
+ case T_BooleanTest:
+ {
+ BooleanTest *bt = (BooleanTest *) node;
+
+ /*
+ * Recurse to input subexpressions.
+ */
+ if (!foreign_expr_walker((Node *) bt->arg,
+ glob_cxt, &inner_cxt))
+ return false;
+
+ /* Output is always boolean and so noncollatable. */
+ collation = InvalidOid;
+ state = FDW_COLLATE_NONE;
+ }
+ break;
case T_ArrayExpr:
{
ArrayExpr *a = (ArrayExpr *) node;
/*
* Recurse to input subexpressions.
*/
if (!foreign_expr_walker((Node *) a->elements,
glob_cxt, &inner_cxt))
return false;
@@ -2449,20 +2466,23 @@ deparseExpr(Expr *node, deparse_expr_cxt *context)
break;
case T_RelabelType:
deparseRelabelType((RelabelType *) node, context);
break;
case T_BoolExpr:
deparseBoolExpr((BoolExpr *) node, context);
break;
case T_NullTest:
deparseNullTest((NullTest *) node, context);
break;
+ case T_BooleanTest:
+ deparseBooleanTest((BooleanTest *) node, context);
+ break;
case T_ArrayExpr:
deparseArrayExpr((ArrayExpr *) node, context);
break;
case T_Aggref:
deparseAggref((Aggref *) node, context);
break;
default:
elog(ERROR, "unsupported expression type for deparse: %d",
(int) nodeTag(node));
break;
@@ -3000,20 +3020,67 @@ deparseNullTest(NullTest *node, deparse_expr_cxt *context)
}
else
{
if (node->nulltesttype == IS_NULL)
appendStringInfoString(buf, " IS NOT DISTINCT FROM NULL)");
else
appendStringInfoString(buf, " IS DISTINCT FROM NULL)");
}
}
+/*
+ * Deparse IS [NOT] TRUE/FALSE/UNKNOWN expression.
+ */
+static void
+deparseBooleanTest(BooleanTest *node, deparse_expr_cxt *context)
+{
+ StringInfo buf = context->buf;
+
+ switch (node->booltesttype)
+ {
+ case IS_TRUE:
+ appendStringInfoChar(buf, '(');
+ deparseExpr(node->arg, context);
+ appendStringInfoString(buf, ")");
+ break;
+ case IS_NOT_TRUE:
+ appendStringInfoString(buf, "(NOT ");
+ deparseExpr(node->arg, context);
+ appendStringInfoString(buf, " OR ");
+ deparseExpr(node->arg, context);
+ appendStringInfoString(buf, " IS NULL)");
+ break;
+ case IS_FALSE:
+ appendStringInfoString(buf, "(NOT ");
+ deparseExpr(node->arg, context);
+ appendStringInfoString(buf, ")");
+ break;
+ case IS_NOT_FALSE:
+ appendStringInfoChar(buf, '(');
+ deparseExpr(node->arg, context);
+ appendStringInfoString(buf, " OR ");
+ deparseExpr(node->arg, context);
+ appendStringInfoString(buf, " IS NULL)");
+ break;
+ case IS_UNKNOWN:
+ appendStringInfoChar(buf, '(');
+ deparseExpr(node->arg, context);
+ appendStringInfoString(buf, " IS NULL)");
+ break;
+ case IS_NOT_UNKNOWN:
+ appendStringInfoChar(buf, '(');
+ deparseExpr(node->arg, context);
+ appendStringInfoString(buf, " IS NOT NULL)");
+ break;
+ }
+}
+
/*
* Deparse ARRAY[...] construct.
*/
static void
deparseArrayExpr(ArrayExpr *node, deparse_expr_cxt *context)
{
StringInfo buf = context->buf;
bool first = true;
ListCell *lc;
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 7df30010f2..1d618790b6 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -659,20 +659,68 @@ EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c1 IS NULL; -- Nu
(3 rows)
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c1 IS NOT NULL; -- NullTest
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 (("C 1" IS NOT NULL))
(3 rows)
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 = 100) IS TRUE; -- BooleanTest
+ 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 ((("C 1" = 100)))
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 = 100) IS NOT TRUE; -- BooleanTest
+ 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 ((NOT ("C 1" = 100) OR ("C 1" = 100) IS NULL))
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 = 100) IS FALSE; -- BooleanTest
+ 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 ((NOT ("C 1" = 100)))
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 = 100) IS NOT FALSE; -- BooleanTest
+ 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 ((("C 1" = 100) OR ("C 1" = 100) IS NULL))
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 = 100) IS UNKNOWN; -- BooleanTest
+ 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 ((("C 1" = 100) IS NULL))
+(3 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 = 100) IS NOT UNKNOWN; -- BooleanTest
+ 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 ((("C 1" = 100) IS NOT NULL))
+(3 rows)
+
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE round(abs(c1), 0) = 1; -- FuncExpr
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 ((round(abs("C 1"), 0) = 1::numeric))
(3 rows)
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c1 = -c1; -- OpExpr(l)
QUERY PLAN
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 78379bdea5..7d1887aa1d 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -321,20 +321,26 @@ DELETE FROM loct_empty;
ANALYZE ft_empty;
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft_empty ORDER BY c1;
-- ===================================================================
-- WHERE with remotely-executable conditions
-- ===================================================================
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE t1.c1 = 1; -- Var, OpExpr(b), Const
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE t1.c1 = 100 AND t1.c2 = 0; -- BoolExpr
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c1 IS NULL; -- NullTest
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c1 IS NOT NULL; -- NullTest
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 = 100) IS TRUE; -- BooleanTest
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 = 100) IS NOT TRUE; -- BooleanTest
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 = 100) IS FALSE; -- BooleanTest
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 = 100) IS NOT FALSE; -- BooleanTest
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 = 100) IS UNKNOWN; -- BooleanTest
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 = 100) IS NOT UNKNOWN; -- BooleanTest
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE round(abs(c1), 0) = 1; -- FuncExpr
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c1 = -c1; -- OpExpr(l)
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 IS NOT NULL) IS DISTINCT FROM (c1 IS NOT NULL); -- DistinctExpr
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c1 = ANY(ARRAY[c2, 1, c1 + 0]); -- ScalarArrayOpExpr
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c1 = (ARRAY[c1,c2,3])[1]; -- SubscriptingRef
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c6 = E'foo''s\\bar'; -- check special chars
EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE c8 = 'foo'; -- can't be sent to remote
-- parameterized remote path for foreign table
EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM "S 1"."T 1" a, ft2 b WHERE a."C 1" = 47 AND b.c1 = a.c2;
--
2.30.1 (Apple Git-130)
Hi Emre,
This looks like a good improvement.
Please add this patch to the commitfest so that it's not forgotten. It
will be considered as a new feature so will be considered for commit
after the next commitfest.
Mean time here are some comments.
+/*
+ * Deparse IS [NOT] TRUE/FALSE/UNKNOWN expression.
+ */
+static void
+deparseBooleanTest(BooleanTest *node, deparse_expr_cxt *context)
+{
+ StringInfo buf = context->buf;
+
+ switch (node->booltesttype)
+ {+ case IS_NOT_TRUE:
+ appendStringInfoString(buf, "(NOT ");
+ deparseExpr(node->arg, context);
+ appendStringInfoString(buf, " OR ");
+ deparseExpr(node->arg, context);
+ appendStringInfoString(buf, " IS NULL)");
+ break;+}
I don't understand why we need to complicate the expressions when
sending those to the foreign nodes. Why do we want to send (xyz IS
FALSE) (NOT (xyz) OR (xyz IS NULL)) and not as just (xyz IS FALSE).
The latter is much more readable and less error-prone. That true for
all the BooleanTest deparsing.
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 = 100) IS
TRUE; -- BooleanTest
Also test a boolean column?
On Mon, May 31, 2021 at 1:33 PM Emre Hasegeli <emre@hasegeli.com> wrote:
The comparison predicates IS [NOT] TRUE/FALSE/UNKNOWN were not
recognised by postgres_fdw, so they were not pushed down to the remote
server. The attached patch adds support for them.I am adding this to the commitfest 2021-07.
--
Best Wishes,
Ashutosh Bapat
Please add this patch to the commitfest so that it's not forgotten. It
will be considered as a new feature so will be considered for commit
after the next commitfest.
I did [1]https://commitfest.postgresql.org/33/3144/. You can add yourself as a reviewer.
I don't understand why we need to complicate the expressions when
sending those to the foreign nodes. Why do we want to send
(NOT xyz OR xyz IS NULL) and not as just (xyz IS FALSE).
The latter is much more readable and less error-prone. That true for
all the BooleanTest deparsing.
= true/false conditions are normalised. I thought similar behaviour
would be expected here.
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 = 100) IS
TRUE; -- BooleanTestAlso test a boolean column?
There isn't a boolean column on the test table currently.
Le lundi 31 mai 2021, 18:51:57 CEST Emre Hasegeli a écrit :
Please add this patch to the commitfest so that it's not forgotten. It
will be considered as a new feature so will be considered for commit
after the next commitfest.I did [1]. You can add yourself as a reviewer.
I don't understand why we need to complicate the expressions when
sending those to the foreign nodes. Why do we want to send
(NOT xyz OR xyz IS NULL) and not as just (xyz IS FALSE).
The latter is much more readable and less error-prone. That true for
all the BooleanTest deparsing.= true/false conditions are normalised. I thought similar behaviour
would be expected here.
I agree with Ashutosh, since IS NOT TRUE / FALSE is already a way of
normalizing it I don't really see what this brings.
+EXPLAIN (VERBOSE, COSTS OFF) SELECT * FROM ft1 t1 WHERE (c1 = 100) IS
TRUE; -- BooleanTestAlso test a boolean column?
There isn't a boolean column on the test table currently.
We should probably add one then.
--
Ronan Dunklau
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested
Hello
I tried to apply the patch to master branch and got a couple of errors, so I think the patch needs a rebase.
I also agree with Ashutosh that the "IS NOT TRUE" case can be simplified to just "IS FALSE". it's simpler to understand.
based on this, I think we should restructure the switch-case statement in deparseBooleanTest because some of the cases in there evaluate to the same result but handles differently.
For example, "IS TRUE" and "IS NOT FALSE" both evaluate to true, so can be handled in the same way
something like:
switch (node->booltesttype)
{
case IS_TRUE:
case IS_NOT_FALSE:
appendStringInfoChar(buf, '(');
deparseExpr(node->arg, context);
appendStringInfoString(buf, ")");
break;
case IS_FALSE:
case IS_NOT_TRUE:
appendStringInfoChar(buf, '(');
deparseExpr(node->arg, context);
appendStringInfoString(buf, " IS FALSE)");
break;
case IS_UNKNOWN:
appendStringInfoChar(buf, '(');
deparseExpr(node->arg, context);
appendStringInfoString(buf, " IS NULL)");
break;
case IS_NOT_UNKNOWN:
appendStringInfoChar(buf, '(');
deparseExpr(node->arg, context);
appendStringInfoString(buf, " IS NOT NULL)");
break;
}
just a thought
thanks!
-------------------------------
Cary Huang
HighGo Software Canada
www.highgo.ca
Cary Huang <cary.huang@highgo.ca> writes:
I also agree with Ashutosh that the "IS NOT TRUE" case can be simplified to just "IS FALSE". it's simpler to understand.
Uh ... surely that's just wrong?
regression=# select null is not true;
?column?
----------
t
(1 row)
regression=# select null is false;
?column?
----------
f
(1 row)
regards, tom lane
On 31 May 2021, at 18:51, Emre Hasegeli <emre@hasegeli.com> wrote:
Please add this patch to the commitfest so that it's not forgotten. It
will be considered as a new feature so will be considered for commit
after the next commitfest.I did [1].
The patch no longer applies to HEAD, can you please submit a rebased version?
--
Daniel Gustafsson https://vmware.com/
On 1 Sep 2021, at 13:15, Daniel Gustafsson <daniel@yesql.se> wrote:
On 31 May 2021, at 18:51, Emre Hasegeli <emre@hasegeli.com> wrote:
Please add this patch to the commitfest so that it's not forgotten. It
will be considered as a new feature so will be considered for commit
after the next commitfest.I did [1].
The patch no longer applies to HEAD, can you please submit a rebased version?
Since the commitfest is now ending, I'm marking this Returned with Feedback.
Please resubmit a rebased version for the next CF if you are still interested
in pursuing this patch.
--
Daniel Gustafsson https://vmware.com/