Hi.
Tom Lane писал 2022-01-18 02:08:
Alexander Pyhalov <a.pyha...@postgrespro.ru> writes:
Perhaps in a MACRO?
Changed this check to a macro, also fixed condition in
is_foreign_param() and added test for it.
Also fixed comment in prepare_query_params().
I took a quick look at this. I'm unconvinced that you need the
TIME_RELATED_SQLVALUE_FUNCTION macro, mainly because I think testing
that in is_foreign_param() is pointless. The only way we'll be seeing
a SQLValueFunction in is_foreign_param() is if we decided it was
shippable, so you really don't need two duplicate tests.
(In the same vein, I would not bother with including a switch in
deparseSQLValueFunction that knows about these opcodes explicitly.
Just use the typmod field; exprTypmod() does.)
Yes, sure, is_foreign_param() is called only when is_foreign_expr() is
true.
Simplified this part.
I also find it pretty bizarre that contain_unsafe_functions
isn't placed beside its one caller. Maybe that indicates that
is_foreign_expr is mis-located and should be in shippable.c.
More generally, it's annoying that you had to copy-and-paste
all of contain_mutable_functions to make this. That creates
a rather nasty maintenance hazard for future hackers, who will
need to keep these widely-separated functions in sync. Not
sure what to do about that though. Do we want to extend
contain_mutable_functions itself to cover this use-case?
I've moved logic to contain_mutable_functions_skip_sqlvalues(), it
uses the same subroutines as contain_mutable_functions().
Should we instead just add one more parameter to
contain_mutable_functions()?
I'm not sure that it's a good idea given that
contain_mutable_functions() seems to be an
external interface.
The test cases seem a bit overkill --- what is the point of the
two nigh-identical PREPARE tests, or the GROUP BY test? If
anything is broken about GROUP BY, surely it's not specific
to this patch.
I've removed PREPARE tests, but GROUP BY test checks
foreign_grouping_ok()/is_foreign_param() path,
so I think it's useful.
I'm not really convinced by the premise of 0002, particularly
this bit:
static bool
-contain_mutable_functions_checker(Oid func_id, void *context)
+contain_unsafe_functions_checker(Oid func_id, void *context)
{
- return (func_volatile(func_id) != PROVOLATILE_IMMUTABLE);
+ /* now() is stable, but we can ship it as it's replaced by parameter
*/
+ return !(func_volatile(func_id) == PROVOLATILE_IMMUTABLE || func_id
== F_NOW);
}
The point of the check_functions_in_node callback API is to verify
the mutability of functions that are embedded in various sorts of
expression nodes ... but they might not be in a plain FuncExpr node,
which is the only case you'll deparse correctly. It might be that
now() cannot legally appear in any of the other node types that
check_functions_in_node knows about, but I'm not quite convinced
of that, and even less convinced that that'll stay true as we add
more expression node types. Also, if we commit this, for sure
some poor soul will try to expand the logic to some other stable
function that *can* appear in those contexts, and that'll be broken.
The implementation of converting now() to CURRENT_TIMESTAMP
seems like an underdocumented kluge, too.
On the whole I'm a bit inclined to drop 0002; I'm not sure it's
worth the trouble.
OK. Let's drop it for now.
--
Best regards,
Alexander Pyhalov,
Postgres Professional
From 9a67c52b57e0b50a3702598aa0b3e8af89569a9c Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov <a.pyha...@postgrespro.ru>
Date: Thu, 29 Jul 2021 11:45:28 +0300
Subject: [PATCH] SQLValue functions pushdown
current_timestamp, localtimestamp and similar SQLValue functions
can be computed locally and sent to remote side as parameters values.
---
contrib/postgres_fdw/deparse.c | 83 ++++++++++++++++-
.../postgres_fdw/expected/postgres_fdw.out | 88 +++++++++++++++++++
contrib/postgres_fdw/postgres_fdw.c | 9 +-
contrib/postgres_fdw/postgres_fdw.h | 1 +
contrib/postgres_fdw/shippable.c | 3 +
contrib/postgres_fdw/sql/postgres_fdw.sql | 22 +++++
src/backend/optimizer/util/clauses.c | 27 +++++-
src/include/optimizer/optimizer.h | 1 +
8 files changed, 226 insertions(+), 8 deletions(-)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index bf12eac0288..a398e1b2174 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -109,6 +109,15 @@ typedef struct deparse_expr_cxt
appendStringInfo((buf), "%s%d.", REL_ALIAS_PREFIX, (varno))
#define SUBQUERY_REL_ALIAS_PREFIX "s"
#define SUBQUERY_COL_ALIAS_PREFIX "c"
+#define TIME_RELATED_SQLVALUE_FUNCTION(s) \
+ (s->op == SVFOP_CURRENT_TIMESTAMP || \
+ s->op == SVFOP_CURRENT_TIMESTAMP_N || \
+ s->op == SVFOP_CURRENT_TIME || \
+ s->op == SVFOP_CURRENT_TIME_N || \
+ s->op == SVFOP_LOCALTIMESTAMP || \
+ s->op == SVFOP_LOCALTIMESTAMP_N || \
+ s->op == SVFOP_LOCALTIME || \
+ s->op == SVFOP_LOCALTIME_N)
/*
* Functions to determine whether an expression can be evaluated safely on
@@ -158,6 +167,7 @@ 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 deparseSQLValueFunction(SQLValueFunction *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 deparseCaseExpr(CaseExpr *node, deparse_expr_cxt *context);
@@ -274,7 +284,7 @@ is_foreign_expr(PlannerInfo *root,
* be able to make this choice with more granularity. (We check this last
* because it requires a lot of expensive catalog lookups.)
*/
- if (contain_mutable_functions((Node *) expr))
+ if (contain_mutable_functions_skip_sqlvalues((Node *) expr))
return false;
/* OK to evaluate on the remote server */
@@ -619,6 +629,30 @@ foreign_expr_walker(Node *node,
state = FDW_COLLATE_UNSAFE;
}
break;
+ case T_SQLValueFunction:
+ {
+ SQLValueFunction *s = (SQLValueFunction *) node;
+
+ /*
+ * For now only time-related SQLValue functions are supported.
+ * We can push down localtimestamp and localtime as we compute
+ * them locally.
+ */
+ if (s->op != SVFOP_CURRENT_TIMESTAMP &&
+ s->op != SVFOP_CURRENT_TIMESTAMP_N &&
+ s->op != SVFOP_CURRENT_TIME &&
+ s->op != SVFOP_CURRENT_TIME_N &&
+ s->op != SVFOP_LOCALTIMESTAMP &&
+ s->op != SVFOP_LOCALTIMESTAMP_N &&
+ s->op != SVFOP_LOCALTIME &&
+ s->op != SVFOP_LOCALTIME_N)
+ return false;
+
+ /* Timestamp or time are not collatable */
+ collation = InvalidOid;
+ state = FDW_COLLATE_NONE;
+ }
+ break;
case T_BoolExpr:
{
BoolExpr *b = (BoolExpr *) node;
@@ -1032,6 +1066,12 @@ is_foreign_param(PlannerInfo *root,
case T_Param:
/* Params always have to be sent to the foreign server */
return true;
+ case T_SQLValueFunction:
+ /*
+ * We can get here only if is_foreign_expr(expr) returned
+ * true, so it's a supported SQLValueFunction.
+ */
+ return true;
default:
break;
}
@@ -2604,6 +2644,9 @@ deparseExpr(Expr *node, deparse_expr_cxt *context)
case T_RelabelType:
deparseRelabelType((RelabelType *) node, context);
break;
+ case T_SQLValueFunction:
+ deparseSQLValueFunction((SQLValueFunction *) node, context);
+ break;
case T_BoolExpr:
deparseBoolExpr((BoolExpr *) node, context);
break;
@@ -3192,6 +3235,44 @@ deparseRelabelType(RelabelType *node, deparse_expr_cxt *context)
node->resulttypmod));
}
+/*
+ * Deparse a SQLValueFunction node
+ */
+static void
+deparseSQLValueFunction(SQLValueFunction *node, deparse_expr_cxt *context)
+{
+ int32 typmod = node->typmod;
+
+ Assert(node->type != InvalidOid);
+
+ /* Treat like a Param */
+ if (context->params_list)
+ {
+ int pindex = 0;
+ ListCell *lc;
+
+ /* find its index in params_list */
+ foreach(lc, *context->params_list)
+ {
+ pindex++;
+ if (equal(node, (Node *) lfirst(lc)))
+ break;
+ }
+ if (lc == NULL)
+ {
+ /* not in list, so add it */
+ pindex++;
+ *context->params_list = lappend(*context->params_list, node);
+ }
+
+ printRemoteParam(pindex, node->type, typmod, context);
+ }
+ else
+ {
+ printRemotePlaceholder(node->type, typmod, context);
+ }
+}
+
/*
* Deparse a BoolExpr node.
*/
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 7d6f7d9e3df..29345f251d7 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -1073,6 +1073,94 @@ SELECT * FROM ft1 t1 WHERE t1.c1 === t1.c2 order by t1.c2 limit 1;
1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo
(1 row)
+-- Test SQLValue functions pushdown
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT c1,c2,c4 FROM ft2 WHERE c4 = current_timestamp;
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------
+ Foreign Scan on public.ft2
+ Output: c1, c2, c4
+ Remote SQL: SELECT "C 1", c2, c4 FROM "S 1"."T 1" WHERE ((c4 = $1::timestamp with time zone))
+(3 rows)
+
+SELECT c1,c2,c4 FROM ft2 WHERE c4 = current_timestamp;
+ c1 | c2 | c4
+----+----+----
+(0 rows)
+
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT c1,c2,c5 FROM ft2 WHERE c5 > localtimestamp - interval '1000 years' AND c1 > 990 ORDER BY c1;
+ QUERY PLAN
+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan on public.ft2
+ Output: c1, c2, c5
+ Remote SQL: SELECT "C 1", c2, c5 FROM "S 1"."T 1" WHERE (("C 1" > 990)) AND ((c5 > ($1::timestamp without time zone - '1000 years'::interval))) ORDER BY "C 1" ASC NULLS LAST
+(3 rows)
+
+SELECT c1,c2,c5 FROM ft2 WHERE c5 > localtimestamp - interval '1000 years' AND c1 > 990 ORDER BY c1;
+ c1 | c2 | c5
+------+----+--------------------------
+ 991 | 1 | Thu Apr 02 00:00:00 1970
+ 992 | 2 | Fri Apr 03 00:00:00 1970
+ 993 | 3 | Sat Apr 04 00:00:00 1970
+ 994 | 4 | Sun Apr 05 00:00:00 1970
+ 995 | 5 | Mon Apr 06 00:00:00 1970
+ 996 | 6 | Tue Apr 07 00:00:00 1970
+ 997 | 7 | Wed Apr 08 00:00:00 1970
+ 998 | 8 | Thu Apr 09 00:00:00 1970
+ 999 | 9 | Fri Apr 10 00:00:00 1970
+ 1000 | 0 | Thu Jan 01 00:00:00 1970
+(10 rows)
+
+-- not shippable due to timestamptz arithmetic
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT c1,c2,c4 FROM ft2 WHERE c4 > current_timestamp - interval '1000 years' AND c1 > 990 ORDER BY c1;
+ QUERY PLAN
+---------------------------------------------------------------------------------------------------------
+ Foreign Scan on public.ft2
+ Output: c1, c2, c4
+ Filter: (ft2.c4 > (CURRENT_TIMESTAMP - '@ 1000 years'::interval))
+ Remote SQL: SELECT "C 1", c2, c4 FROM "S 1"."T 1" WHERE (("C 1" > 990)) ORDER BY "C 1" ASC NULLS LAST
+(4 rows)
+
+SELECT c1,c2,c4 FROM ft2 WHERE c4 > current_timestamp - interval '1000 years' AND c1 > 990 ORDER BY c1;
+ c1 | c2 | c4
+------+----+------------------------------
+ 991 | 1 | Thu Apr 02 00:00:00 1970 PST
+ 992 | 2 | Fri Apr 03 00:00:00 1970 PST
+ 993 | 3 | Sat Apr 04 00:00:00 1970 PST
+ 994 | 4 | Sun Apr 05 00:00:00 1970 PST
+ 995 | 5 | Mon Apr 06 00:00:00 1970 PST
+ 996 | 6 | Tue Apr 07 00:00:00 1970 PST
+ 997 | 7 | Wed Apr 08 00:00:00 1970 PST
+ 998 | 8 | Thu Apr 09 00:00:00 1970 PST
+ 999 | 9 | Fri Apr 10 00:00:00 1970 PST
+ 1000 | 0 | Thu Jan 01 00:00:00 1970 PST
+(10 rows)
+
+-- direct modify
+EXPLAIN (VERBOSE, COSTS OFF)
+UPDATE ft2 SET c4 = current_timestamp WHERE c4 < current_timestamp;
+ QUERY PLAN
+--------------------------------------------------------------------------------------------------------------------------
+ Update on public.ft2
+ -> Foreign Update on public.ft2
+ Remote SQL: UPDATE "S 1"."T 1" SET c4 = $1::timestamp with time zone WHERE ((c4 < $1::timestamp with time zone))
+(3 rows)
+
+-- check that we don't try to push down parameter in group by
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT sum(c1), current_timestamp FROM ft2 WHERE c1 > 990 GROUP BY current_timestamp;
+ QUERY PLAN
+-------------------------------------------------------------------------
+ GroupAggregate
+ Output: sum(c1), (CURRENT_TIMESTAMP)
+ Group Key: CURRENT_TIMESTAMP
+ -> Foreign Scan on public.ft2
+ Output: CURRENT_TIMESTAMP, c1
+ Remote SQL: SELECT "C 1" FROM "S 1"."T 1" WHERE (("C 1" > 990))
+(6 rows)
+
-- Test CASE pushdown
EXPLAIN (VERBOSE, COSTS OFF)
SELECT c1,c2,c3 FROM ft2 WHERE CASE WHEN c1 > 990 THEN c1 END < 1000 ORDER BY c1;
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 09a3f5e23cb..6aa3a20f20c 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -4834,12 +4834,9 @@ prepare_query_params(PlanState *node,
}
/*
- * Prepare remote-parameter expressions for evaluation. (Note: in
- * practice, we expect that all these expressions will be just Params, so
- * we could possibly do something more efficient than using the full
- * expression-eval machinery for this. But probably there would be little
- * benefit, and it'd require postgres_fdw to know more than is desirable
- * about Param evaluation.)
+ * Prepare remote-parameter expressions for evaluation. (Note: we cannot
+ * expect that all these expressions will be just Params, so we should use
+ * the full expression-eval machinery for this).
*/
*param_exprs = ExecInitExprList(fdw_exprs, node);
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index 8ae79e97e44..0ab12e90e05 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -231,5 +231,6 @@ extern const char *get_jointype_name(JoinType jointype);
/* in shippable.c */
extern bool is_builtin(Oid objectId);
extern bool is_shippable(Oid objectId, Oid classId, PgFdwRelationInfo *fpinfo);
+extern bool contain_unsafe_functions(Node *clause);
#endif /* POSTGRES_FDW_H */
diff --git a/contrib/postgres_fdw/shippable.c b/contrib/postgres_fdw/shippable.c
index 8e759da00d5..7227a1be35b 100644
--- a/contrib/postgres_fdw/shippable.c
+++ b/contrib/postgres_fdw/shippable.c
@@ -25,9 +25,12 @@
#include "access/transam.h"
#include "catalog/dependency.h"
+#include "catalog/pg_proc.h"
+#include "nodes/nodeFuncs.h"
#include "postgres_fdw.h"
#include "utils/hsearch.h"
#include "utils/inval.h"
+#include "utils/lsyscache.h"
#include "utils/syscache.h"
/* Hash table for caching the results of shippability lookups */
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 9eb673e3693..69d15ede05e 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -413,6 +413,28 @@ EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1 t1 WHERE t1.c1 === t1.c2 order by t1.c2 limit 1;
SELECT * FROM ft1 t1 WHERE t1.c1 === t1.c2 order by t1.c2 limit 1;
+-- Test SQLValue functions pushdown
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT c1,c2,c4 FROM ft2 WHERE c4 = current_timestamp;
+SELECT c1,c2,c4 FROM ft2 WHERE c4 = current_timestamp;
+
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT c1,c2,c5 FROM ft2 WHERE c5 > localtimestamp - interval '1000 years' AND c1 > 990 ORDER BY c1;
+SELECT c1,c2,c5 FROM ft2 WHERE c5 > localtimestamp - interval '1000 years' AND c1 > 990 ORDER BY c1;
+
+-- not shippable due to timestamptz arithmetic
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT c1,c2,c4 FROM ft2 WHERE c4 > current_timestamp - interval '1000 years' AND c1 > 990 ORDER BY c1;
+SELECT c1,c2,c4 FROM ft2 WHERE c4 > current_timestamp - interval '1000 years' AND c1 > 990 ORDER BY c1;
+
+-- direct modify
+EXPLAIN (VERBOSE, COSTS OFF)
+UPDATE ft2 SET c4 = current_timestamp WHERE c4 < current_timestamp;
+
+-- check that we don't try to push down parameter in group by
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT sum(c1), current_timestamp FROM ft2 WHERE c1 > 990 GROUP BY current_timestamp;
+
-- Test CASE pushdown
EXPLAIN (VERBOSE, COSTS OFF)
SELECT c1,c2,c3 FROM ft2 WHERE CASE WHEN c1 > 990 THEN c1 END < 1000 ORDER BY c1;
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index a707dc9f26a..4975bfc62c3 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -91,6 +91,11 @@ typedef struct
List *safe_param_ids; /* PARAM_EXEC Param IDs to treat as safe */
} max_parallel_hazard_context;
+typedef struct
+{
+ bool skip_sqlvalues;
+} mutable_functions_context;
+
static bool contain_agg_clause_walker(Node *node, void *context);
static bool find_window_functions_walker(Node *node, WindowFuncLists *lists);
static bool contain_subplans_walker(Node *node, void *context);
@@ -366,6 +371,20 @@ contain_mutable_functions(Node *clause)
return contain_mutable_functions_walker(clause, NULL);
}
+/*
+ * contain_mutable_functions_skip_sqlvalues
+ * Special purpose version of contain_mutable_functions() for use in
+ * FDWs: ignore SQLValueFunction, but detect other mutable functions.
+ */
+bool
+contain_mutable_functions_skip_sqlvalues(Node *clause)
+{
+ mutable_functions_context context;
+
+ context.skip_sqlvalues = true;
+ return contain_mutable_functions_walker(clause, &context);
+}
+
static bool
contain_mutable_functions_checker(Oid func_id, void *context)
{
@@ -384,8 +403,14 @@ contain_mutable_functions_walker(Node *node, void *context)
if (IsA(node, SQLValueFunction))
{
+ bool skip_sqlvalues = false;
+
+ if (context && ((mutable_functions_context *) context)->skip_sqlvalues)
+ skip_sqlvalues = true;
+
/* all variants of SQLValueFunction are stable */
- return true;
+ if (!skip_sqlvalues)
+ return true;
}
if (IsA(node, NextValueExpr))
diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/optimizer.h
index 6b8ee0c69fa..bce1597d3c1 100644
--- a/src/include/optimizer/optimizer.h
+++ b/src/include/optimizer/optimizer.h
@@ -141,6 +141,7 @@ extern Expr *canonicalize_qual(Expr *qual, bool is_check);
/* in util/clauses.c: */
extern bool contain_mutable_functions(Node *clause);
+extern bool contain_mutable_functions_skip_sqlvalues(Node *clause);
extern bool contain_volatile_functions(Node *clause);
extern bool contain_volatile_functions_not_nextval(Node *clause);
--
2.25.1
