public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alexander Pyhalov <[email protected]>
To: Tom Lane <[email protected]>
Cc: Ranier Vilela <[email protected]>
Cc: Zhihong Yu <[email protected]>
Cc: Pgsql Hackers <[email protected]>
Subject: Re: Push down time-related SQLValue functions to foreign server
Date: Tue, 18 Jan 2022 11:52:25 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CALNJ-vSp-jndrT8S40oUbNcbopv-PTROaQak2rMW=Be3MfmVbQ@mail.gmail.com>
<CAEudQArwB-C1FuShbcCAGdDqszt8Ho80eFHFLVE+Ki8Q=HnL_g@mail.gmail.com>
<[email protected]>
<[email protected]>
Hi.
Tom Lane писал 2022-01-18 02:08:
> Alexander Pyhalov <[email protected]> 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
Attachments:
[text/x-diff] 0001-SQLValue-functions-pushdown.patch (15.5K, ../[email protected]/2-0001-SQLValue-functions-pushdown.patch)
download | inline diff:
From 9a67c52b57e0b50a3702598aa0b3e8af89569a9c Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov <[email protected]>
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
view thread (10+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Push down time-related SQLValue functions to foreign server
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox