public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2 1/3] WIP: elog: Make ereport a bit easier to use and a bit faster.
23+ messages / 8 participants
[nested] [flat]
* [PATCH v2 1/3] WIP: elog: Make ereport a bit easier to use and a bit faster.
@ 2020-03-20 01:05 Andres Freund <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Andres Freund @ 2020-03-20 01:05 UTC (permalink / raw)
Author: Andres Freund
Discussion: https://postgr.es/m/[email protected]
---
src/include/executor/executor.h | 2 +-
src/include/parser/parse_coerce.h | 2 +-
src/include/parser/parse_node.h | 2 +-
src/include/parser/scanner.h | 2 +-
src/include/utils/elog.h | 64 ++++++++++++----------
src/backend/executor/execUtils.c | 8 +--
src/backend/parser/parse_coerce.c | 6 +--
src/backend/parser/parse_node.c | 8 +--
src/backend/parser/scan.l | 6 +--
src/backend/storage/ipc/dsm_impl.c | 8 +--
src/backend/tcop/postgres.c | 14 ++---
src/backend/utils/adt/jsonfuncs.c | 8 +--
src/backend/utils/error/elog.c | 86 +++++++++---------------------
src/pl/plpgsql/src/pl_scanner.c | 6 +--
src/pl/plpgsql/src/plpgsql.h | 2 +-
15 files changed, 98 insertions(+), 126 deletions(-)
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 94890512dc8..cd0e6439cde 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -546,7 +546,7 @@ exec_rt_fetch(Index rti, EState *estate)
extern Relation ExecGetRangeTableRelation(EState *estate, Index rti);
-extern int executor_errposition(EState *estate, int location);
+extern void executor_errposition(EState *estate, int location);
extern void RegisterExprContextCallback(ExprContext *econtext,
ExprContextCallbackFunction function,
diff --git a/src/include/parser/parse_coerce.h b/src/include/parser/parse_coerce.h
index 8686eaacbc9..a3295b3fa45 100644
--- a/src/include/parser/parse_coerce.h
+++ b/src/include/parser/parse_coerce.h
@@ -61,7 +61,7 @@ extern Node *coerce_to_specific_type_typmod(ParseState *pstate, Node *node,
Oid targetTypeId, int32 targetTypmod,
const char *constructName);
-extern int parser_coercion_errposition(ParseState *pstate,
+extern void parser_coercion_errposition(ParseState *pstate,
int coerce_location,
Node *input_expr);
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index d25819aa28b..b223b595423 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -307,7 +307,7 @@ typedef struct ParseCallbackState
extern ParseState *make_parsestate(ParseState *parentParseState);
extern void free_parsestate(ParseState *pstate);
-extern int parser_errposition(ParseState *pstate, int location);
+extern void parser_errposition(ParseState *pstate, int location);
extern void setup_parser_errposition_callback(ParseCallbackState *pcbstate,
ParseState *pstate, int location);
diff --git a/src/include/parser/scanner.h b/src/include/parser/scanner.h
index a27352afc14..02ae10a2250 100644
--- a/src/include/parser/scanner.h
+++ b/src/include/parser/scanner.h
@@ -140,7 +140,7 @@ extern core_yyscan_t scanner_init(const char *str,
extern void scanner_finish(core_yyscan_t yyscanner);
extern int core_yylex(core_YYSTYPE *lvalp, YYLTYPE *llocp,
core_yyscan_t yyscanner);
-extern int scanner_errposition(int location, core_yyscan_t yyscanner);
+extern void scanner_errposition(int location, core_yyscan_t yyscanner);
extern void setup_scanner_errposition_callback(ScannerCallbackState *scbstate,
core_yyscan_t yyscanner,
int location);
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 0a4ef029ceb..241457f35bd 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -118,59 +118,65 @@
*----------
*/
#ifdef HAVE__BUILTIN_CONSTANT_P
-#define ereport_domain(elevel, domain, rest) \
+#define ereport_domain(elevel, domain, ...) \
do { \
pg_prevent_errno_in_scope(); \
if (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \
- errfinish rest; \
+ { \
+ __VA_ARGS__; \
+ errfinish(); \
+ } \
if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \
pg_unreachable(); \
} while(0)
#else /* !HAVE__BUILTIN_CONSTANT_P */
-#define ereport_domain(elevel, domain, rest) \
+#define ereport_domain(elevel, domain, ...) \
do { \
const int elevel_ = (elevel); \
pg_prevent_errno_in_scope(); \
if (errstart(elevel_, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \
- errfinish rest; \
+ { \
+ __VA_ARGS__; \
+ errfinish(); \
+ } \
if (elevel_ >= ERROR) \
pg_unreachable(); \
} while(0)
#endif /* HAVE__BUILTIN_CONSTANT_P */
-#define ereport(elevel, rest) \
- ereport_domain(elevel, TEXTDOMAIN, rest)
+#define ereport(elevel, ...) \
+ ereport_domain(elevel, TEXTDOMAIN, __VA_ARGS__)
#define TEXTDOMAIN NULL
extern bool errstart(int elevel, const char *filename, int lineno,
const char *funcname, const char *domain);
-extern void errfinish(int dummy,...);
+extern void errfinish(void);
-extern int errcode(int sqlerrcode);
+extern void errcode(int sqlerrcode);
-extern int errcode_for_file_access(void);
-extern int errcode_for_socket_access(void);
+extern void errcode_for_file_access(void);
+extern void errcode_for_socket_access(void);
-extern int errmsg(const char *fmt,...) pg_attribute_printf(1, 2);
-extern int errmsg_internal(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void errmsg(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void errmsg_internal(const char *fmt,...) pg_attribute_printf(1, 2);
-extern int errmsg_plural(const char *fmt_singular, const char *fmt_plural,
+extern void errmsg_plural(const char *fmt_singular, const char *fmt_plural,
unsigned long n,...) pg_attribute_printf(1, 4) pg_attribute_printf(2, 4);
-extern int errdetail(const char *fmt,...) pg_attribute_printf(1, 2);
-extern int errdetail_internal(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void errdetail(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void errdetail_internal(const char *fmt,...) pg_attribute_printf(1, 2);
-extern int errdetail_log(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void errdetail_log(const char *fmt,...) pg_attribute_printf(1, 2);
-extern int errdetail_log_plural(const char *fmt_singular,
+extern void errdetail_log_plural(const char *fmt_singular,
const char *fmt_plural,
unsigned long n,...) pg_attribute_printf(1, 4) pg_attribute_printf(2, 4);
-extern int errdetail_plural(const char *fmt_singular, const char *fmt_plural,
+extern void errdetail_plural(const char *fmt_singular, const char *fmt_plural,
unsigned long n,...) pg_attribute_printf(1, 4) pg_attribute_printf(2, 4);
-extern int errhint(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void errhint(const char *fmt,...) pg_attribute_printf(1, 2);
/*
* errcontext() is typically called in error context callback functions, not
@@ -182,22 +188,22 @@ extern int errhint(const char *fmt,...) pg_attribute_printf(1, 2);
*/
#define errcontext set_errcontext_domain(TEXTDOMAIN), errcontext_msg
-extern int set_errcontext_domain(const char *domain);
+extern void set_errcontext_domain(const char *domain);
-extern int errcontext_msg(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void errcontext_msg(const char *fmt,...) pg_attribute_printf(1, 2);
-extern int errhidestmt(bool hide_stmt);
-extern int errhidecontext(bool hide_ctx);
+extern void errhidestmt(bool hide_stmt);
+extern void errhidecontext(bool hide_ctx);
-extern int errbacktrace(void);
+extern void errbacktrace(void);
-extern int errfunction(const char *funcname);
-extern int errposition(int cursorpos);
+extern void errfunction(const char *funcname);
+extern void errposition(int cursorpos);
-extern int internalerrposition(int cursorpos);
-extern int internalerrquery(const char *query);
+extern void internalerrposition(int cursorpos);
+extern void internalerrquery(const char *query);
-extern int err_generic_string(int field, const char *str);
+extern void err_generic_string(int field, const char *str);
extern int geterrcode(void);
extern int geterrposition(void);
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index cc5177cc2b9..6deef1c679a 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -832,21 +832,21 @@ UpdateChangedParamSet(PlanState *node, Bitmapset *newchg)
* normal non-error case: computing character indexes would be much more
* expensive than storing token offsets.)
*/
-int
+void
executor_errposition(EState *estate, int location)
{
int pos;
/* No-op if location was not provided */
if (location < 0)
- return 0;
+ return;
/* Can't do anything if source text is not available */
if (estate == NULL || estate->es_sourceText == NULL)
- return 0;
+ return;
/* Convert offset to character number */
pos = pg_mbstrlen_with_len(estate->es_sourceText, location) + 1;
/* And pass it to the ereport mechanism */
- return errposition(pos);
+ errposition(pos);
}
/*
diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index 645e4aa4ceb..91d4e99d345 100644
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -1246,15 +1246,15 @@ coerce_to_specific_type(ParseState *pstate, Node *node,
* XXX possibly this is more generally useful than coercion errors;
* if so, should rename and place with parser_errposition.
*/
-int
+void
parser_coercion_errposition(ParseState *pstate,
int coerce_location,
Node *input_expr)
{
if (coerce_location >= 0)
- return parser_errposition(pstate, coerce_location);
+ parser_errposition(pstate, coerce_location);
else
- return parser_errposition(pstate, exprLocation(input_expr));
+ parser_errposition(pstate, exprLocation(input_expr));
}
diff --git a/src/backend/parser/parse_node.c b/src/backend/parser/parse_node.c
index 6e98fe55fc4..9a2fd924b67 100644
--- a/src/backend/parser/parse_node.c
+++ b/src/backend/parser/parse_node.c
@@ -106,21 +106,21 @@ free_parsestate(ParseState *pstate)
* normal non-error case: computing character indexes would be much more
* expensive than storing token offsets.)
*/
-int
+void
parser_errposition(ParseState *pstate, int location)
{
int pos;
/* No-op if location was not provided */
if (location < 0)
- return 0;
+ return;
/* Can't do anything if source text is not available */
if (pstate == NULL || pstate->p_sourcetext == NULL)
- return 0;
+ return;
/* Convert offset to character number */
pos = pg_mbstrlen_with_len(pstate->p_sourcetext, location) + 1;
/* And pass it to the ereport mechanism */
- return errposition(pos);
+ errposition(pos);
}
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index b1ea0cb5384..50ba68abd4f 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -1076,18 +1076,18 @@ other .
* (essentially, scan.l, parser.c, and gram.y), since it requires the
* yyscanner struct to still be available.
*/
-int
+void
scanner_errposition(int location, core_yyscan_t yyscanner)
{
int pos;
if (location < 0)
- return 0; /* no-op if location is unknown */
+ return; /* no-op if location is unknown */
/* Convert byte offset to character number */
pos = pg_mbstrlen_with_len(yyextra->scanbuf, location) + 1;
/* And pass it to the ereport mechanism */
- return errposition(pos);
+ errposition(pos);
}
/*
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index 1972aecbedc..515686ca152 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -92,7 +92,7 @@ static bool dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
void **impl_private, void **mapped_address,
Size *mapped_size, int elevel);
#endif
-static int errcode_for_dynamic_shared_memory(void);
+static void errcode_for_dynamic_shared_memory(void);
const struct config_enum_entry dynamic_shared_memory_options[] = {
#ifdef USE_DSM_POSIX
@@ -1030,11 +1030,11 @@ dsm_impl_unpin_segment(dsm_handle handle, void **impl_private)
}
}
-static int
+static void
errcode_for_dynamic_shared_memory(void)
{
if (errno == EFBIG || errno == ENOMEM)
- return errcode(ERRCODE_OUT_OF_MEMORY);
+ errcode(ERRCODE_OUT_OF_MEMORY);
else
- return errcode_for_file_access();
+ errcode_for_file_access();
}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 00c77b66c74..cb8c23e4b76 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3720,15 +3720,15 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx,
/* spell the error message a bit differently depending on context */
if (IsUnderPostmaster)
ereport(FATAL,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("invalid command-line argument for server process: %s", argv[optind]),
- errhint("Try \"%s --help\" for more information.", progname)));
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid command-line argument for server process: %s", argv[optind]),
+ errhint("Try \"%s --help\" for more information.", progname));
else
ereport(FATAL,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("%s: invalid command-line argument: %s",
- progname, argv[optind]),
- errhint("Try \"%s --help\" for more information.", progname)));
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("%s: invalid command-line argument: %s",
+ progname, argv[optind]),
+ errhint("Try \"%s --help\" for more information.", progname));
}
/*
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 4b5007e0d6f..321ab9a0db9 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -329,7 +329,7 @@ typedef struct JsObject
hash_destroy((jso)->val.json_hash); \
} while (0)
-static int report_json_context(JsonLexContext *lex);
+static void report_json_context(JsonLexContext *lex);
/* semantic action functions for json_object_keys */
static void okeys_object_field_start(void *state, char *fname, bool isnull);
@@ -631,7 +631,7 @@ json_ereport_error(JsonParseErrorType error, JsonLexContext *lex)
* The return value isn't meaningful, but we make it non-void so that this
* can be invoked inside ereport().
*/
-static int
+static void
report_json_context(JsonLexContext *lex)
{
const char *context_start;
@@ -689,8 +689,8 @@ report_json_context(JsonLexContext *lex)
prefix = (context_start > line_start) ? "..." : "";
suffix = (lex->token_type != JSON_TOKEN_END && context_end - lex->input < lex->input_length && *context_end != '\n' && *context_end != '\r') ? "..." : "";
- return errcontext("JSON data, line %d: %s%s%s",
- line_number, prefix, ctxt, suffix);
+ errcontext("JSON data, line %d: %s%s%s",
+ line_number, prefix, ctxt, suffix);
}
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 62eef7b71f4..dc1fda03a8f 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -438,7 +438,7 @@ matches_backtrace_functions(const char *funcname)
* See elog.h for the error level definitions.
*/
void
-errfinish(int dummy,...)
+errfinish(void)
{
ErrorData *edata = &errordata[errordata_stack_depth];
int elevel;
@@ -605,7 +605,7 @@ errfinish(int dummy,...)
*
* The code is expected to be represented as per MAKE_SQLSTATE().
*/
-int
+void
errcode(int sqlerrcode)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -614,8 +614,6 @@ errcode(int sqlerrcode)
CHECK_STACK_DEPTH();
edata->sqlerrcode = sqlerrcode;
-
- return 0; /* return value does not matter */
}
@@ -628,7 +626,7 @@ errcode(int sqlerrcode)
* NOTE: the primary error message string should generally include %m
* when this is used.
*/
-int
+void
errcode_for_file_access(void)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -686,8 +684,6 @@ errcode_for_file_access(void)
edata->sqlerrcode = ERRCODE_INTERNAL_ERROR;
break;
}
-
- return 0; /* return value does not matter */
}
/*
@@ -699,7 +695,7 @@ errcode_for_file_access(void)
* NOTE: the primary error message string should generally include %m
* when this is used.
*/
-int
+void
errcode_for_socket_access(void)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -722,8 +718,6 @@ errcode_for_socket_access(void)
edata->sqlerrcode = ERRCODE_INTERNAL_ERROR;
break;
}
-
- return 0; /* return value does not matter */
}
@@ -819,7 +813,7 @@ errcode_for_socket_access(void)
* Note: no newline is needed at the end of the fmt string, since
* ereport will provide one for the output methods that need it.
*/
-int
+void
errmsg(const char *fmt,...)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -834,14 +828,13 @@ errmsg(const char *fmt,...)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
/*
* Add a backtrace to the containing ereport() call. This is intended to be
* added temporarily during debugging.
*/
-int
+void
errbacktrace(void)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -855,8 +848,6 @@ errbacktrace(void)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
-
- return 0;
}
/*
@@ -906,7 +897,7 @@ set_backtrace(ErrorData *edata, int num_skip)
* the message because the translation would fail and result in infinite
* error recursion.
*/
-int
+void
errmsg_internal(const char *fmt,...)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -921,7 +912,6 @@ errmsg_internal(const char *fmt,...)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
@@ -929,7 +919,7 @@ errmsg_internal(const char *fmt,...)
* errmsg_plural --- add a primary error message text to the current error,
* with support for pluralization of the message text
*/
-int
+void
errmsg_plural(const char *fmt_singular, const char *fmt_plural,
unsigned long n,...)
{
@@ -945,14 +935,13 @@ errmsg_plural(const char *fmt_singular, const char *fmt_plural,
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
/*
* errdetail --- add a detail error message text to the current error
*/
-int
+void
errdetail(const char *fmt,...)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -966,7 +955,6 @@ errdetail(const char *fmt,...)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
@@ -979,7 +967,7 @@ errdetail(const char *fmt,...)
* messages that seem not worth translating for one reason or another
* (typically, that they don't seem to be useful to average users).
*/
-int
+void
errdetail_internal(const char *fmt,...)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -993,14 +981,13 @@ errdetail_internal(const char *fmt,...)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
/*
* errdetail_log --- add a detail_log error message text to the current error
*/
-int
+void
errdetail_log(const char *fmt,...)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1014,14 +1001,13 @@ errdetail_log(const char *fmt,...)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
/*
* errdetail_log_plural --- add a detail_log error message text to the current error
* with support for pluralization of the message text
*/
-int
+void
errdetail_log_plural(const char *fmt_singular, const char *fmt_plural,
unsigned long n,...)
{
@@ -1036,7 +1022,6 @@ errdetail_log_plural(const char *fmt_singular, const char *fmt_plural,
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
@@ -1044,7 +1029,7 @@ errdetail_log_plural(const char *fmt_singular, const char *fmt_plural,
* errdetail_plural --- add a detail error message text to the current error,
* with support for pluralization of the message text
*/
-int
+void
errdetail_plural(const char *fmt_singular, const char *fmt_plural,
unsigned long n,...)
{
@@ -1059,14 +1044,13 @@ errdetail_plural(const char *fmt_singular, const char *fmt_plural,
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
/*
* errhint --- add a hint error message text to the current error
*/
-int
+void
errhint(const char *fmt,...)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1080,7 +1064,6 @@ errhint(const char *fmt,...)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
@@ -1091,7 +1074,7 @@ errhint(const char *fmt,...)
* context information. We assume earlier calls represent more-closely-nested
* states.
*/
-int
+void
errcontext_msg(const char *fmt,...)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1105,7 +1088,6 @@ errcontext_msg(const char *fmt,...)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
/*
@@ -1127,7 +1109,7 @@ errcontext_msg(const char *fmt,...)
* TEXTDOMAIN value that the errstart call did, so order does not matter
* so long as errstart initializes context_domain along with domain.
*/
-int
+void
set_errcontext_domain(const char *domain)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1137,8 +1119,6 @@ set_errcontext_domain(const char *domain)
/* the default text domain is the backend's */
edata->context_domain = domain ? domain : PG_TEXTDOMAIN("postgres");
-
- return 0; /* return value does not matter */
}
@@ -1147,7 +1127,7 @@ set_errcontext_domain(const char *domain)
*
* This should be called if the message text already includes the statement.
*/
-int
+void
errhidestmt(bool hide_stmt)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1156,8 +1136,6 @@ errhidestmt(bool hide_stmt)
CHECK_STACK_DEPTH();
edata->hide_stmt = hide_stmt;
-
- return 0; /* return value does not matter */
}
/*
@@ -1166,7 +1144,7 @@ errhidestmt(bool hide_stmt)
* This should only be used for verbose debugging messages where the repeated
* inclusion of context would bloat the log volume too much.
*/
-int
+void
errhidecontext(bool hide_ctx)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1175,8 +1153,6 @@ errhidecontext(bool hide_ctx)
CHECK_STACK_DEPTH();
edata->hide_ctx = hide_ctx;
-
- return 0; /* return value does not matter */
}
@@ -1187,7 +1163,7 @@ errhidecontext(bool hide_ctx)
* name appear in messages sent to old-protocol clients. Note that the
* passed string is expected to be a non-freeable constant string.
*/
-int
+void
errfunction(const char *funcname)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1197,14 +1173,12 @@ errfunction(const char *funcname)
edata->funcname = funcname;
edata->show_funcname = true;
-
- return 0; /* return value does not matter */
}
/*
* errposition --- add cursor position to the current error
*/
-int
+void
errposition(int cursorpos)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1213,14 +1187,12 @@ errposition(int cursorpos)
CHECK_STACK_DEPTH();
edata->cursorpos = cursorpos;
-
- return 0; /* return value does not matter */
}
/*
* internalerrposition --- add internal cursor position to the current error
*/
-int
+void
internalerrposition(int cursorpos)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1229,8 +1201,6 @@ internalerrposition(int cursorpos)
CHECK_STACK_DEPTH();
edata->internalpos = cursorpos;
-
- return 0; /* return value does not matter */
}
/*
@@ -1240,7 +1210,7 @@ internalerrposition(int cursorpos)
* is intended for use in error callback subroutines that are editorializing
* on the layout of the error report.
*/
-int
+void
internalerrquery(const char *query)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1256,8 +1226,6 @@ internalerrquery(const char *query)
if (query)
edata->internalquery = MemoryContextStrdup(edata->assoc_context, query);
-
- return 0; /* return value does not matter */
}
/*
@@ -1270,7 +1238,7 @@ internalerrquery(const char *query)
* Most potential callers should not use this directly, but instead prefer
* higher-level abstractions, such as errtablecol() (see relcache.c).
*/
-int
+void
err_generic_string(int field, const char *str)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1299,8 +1267,6 @@ err_generic_string(int field, const char *str)
elog(ERROR, "unsupported ErrorData field id: %d", field);
break;
}
-
- return 0; /* return value does not matter */
}
/*
@@ -1463,7 +1429,7 @@ elog_finish(int elevel, const char *fmt,...)
/*
* And let errfinish() finish up.
*/
- errfinish(0);
+ errfinish();
}
@@ -1749,7 +1715,7 @@ ThrowErrorData(ErrorData *edata)
recursion_depth--;
/* Process the error. */
- errfinish(0);
+ errfinish();
}
/*
@@ -1863,7 +1829,7 @@ pg_re_throw(void)
*/
error_context_stack = NULL;
- errfinish(0);
+ errfinish();
}
/* Doesn't return ... */
diff --git a/src/pl/plpgsql/src/pl_scanner.c b/src/pl/plpgsql/src/pl_scanner.c
index 9cea2e42ac4..55b82b8a74c 100644
--- a/src/pl/plpgsql/src/pl_scanner.c
+++ b/src/pl/plpgsql/src/pl_scanner.c
@@ -468,20 +468,20 @@ plpgsql_peek2(int *tok1_p, int *tok2_p, int *tok1_loc, int *tok2_loc)
* parsing of a plpgsql function, since it requires the scanorig string
* to still be available.
*/
-int
+void
plpgsql_scanner_errposition(int location)
{
int pos;
if (location < 0 || scanorig == NULL)
- return 0; /* no-op if location is unknown */
+ return; /* no-op if location is unknown */
/* Convert byte offset to character number */
pos = pg_mbstrlen_with_len(scanorig, location) + 1;
/* And pass it to the ereport mechanism */
(void) internalerrposition(pos);
/* Also pass the function body string */
- return internalerrquery(scanorig);
+ internalerrquery(scanorig);
}
/*
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 69df3306fda..325f4f7fd45 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -1319,7 +1319,7 @@ extern void plpgsql_append_source_text(StringInfo buf,
extern int plpgsql_peek(void);
extern void plpgsql_peek2(int *tok1_p, int *tok2_p, int *tok1_loc,
int *tok2_loc);
-extern int plpgsql_scanner_errposition(int location);
+extern void plpgsql_scanner_errposition(int location);
extern void plpgsql_yyerror(const char *message) pg_attribute_noreturn();
extern int plpgsql_location_to_lineno(int location);
extern int plpgsql_latest_lineno(void);
--
2.25.0.114.g5b0ca878e0
--nyesmgwoodjgg6to
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v2-0002-WIP-reimplement-elog-using-ereport.patch"
^ permalink raw reply [nested|flat] 23+ messages in thread
* [PATCH v9 2/4] WIP: Check for volatile defaults
@ 2020-12-02 05:11 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Justin Pryzby @ 2020-12-02 05:11 UTC (permalink / raw)
We want to check if any column *uses* a volatile default value, but after
parsing and rewriting, that information appears to be lost about which column
values are defaults and which were specified. insertedcols doesn't appear to
be useful for this. So add a field to track if a TargetEntry is planned with
column default.
---
src/backend/executor/nodeModifyTable.c | 64 ++++++++++++++++++++++++--
src/backend/nodes/copyfuncs.c | 1 +
src/backend/nodes/equalfuncs.c | 1 +
src/backend/nodes/makefuncs.c | 1 +
src/backend/nodes/outfuncs.c | 1 +
src/backend/nodes/readfuncs.c | 1 +
src/backend/optimizer/util/tlist.c | 1 +
src/backend/rewrite/rewriteHandler.c | 3 ++
src/include/nodes/primnodes.h | 2 +
9 files changed, 71 insertions(+), 4 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index a53cbeeb2c..2059428e2a 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -50,6 +50,7 @@
#include "foreign/fdwapi.h"
#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
+#include "optimizer/optimizer.h"
#include "rewrite/rewriteHandler.h"
#include "storage/bufmgr.h"
#include "storage/lmgr.h"
@@ -2258,6 +2259,61 @@ ExecModifyTable(PlanState *pstate)
return NULL;
}
+/*
+ * Determine if a table has volatile column defaults which are used by a given
+ * planned statement (if the column is not specified or specified as DEFAULT).
+ * This works only for INSERT.
+ */
+static bool
+has_volatile_defaults(ResultRelInfo *resultRelInfo, ModifyTable *node)
+{
+ TupleDesc tupDesc = RelationGetDescr(resultRelInfo->ri_RelationDesc);
+ Plan *plan;
+
+ Assert(list_length(node->plans) == 1);
+ plan = linitial(node->plans);
+
+ for (int attnum = 1; attnum <= tupDesc->natts; attnum++)
+ {
+ Form_pg_attribute att = TupleDescAttr(tupDesc, attnum - 1);
+ Expr *defexpr;
+ TargetEntry *tle;
+
+ /* We don't need to check dropped/generated attributes */
+ if (att->attisdropped || att->attgenerated)
+ continue;
+
+ tle = list_nth(plan->targetlist, attnum - 1);
+ Assert(tle != NULL);
+ Assert(tle->resno == attnum);
+
+ /*
+ * If the column was specified with a non-default value, then don't
+ * check the volatility of its default
+ */
+ if (!tle->isdefault)
+ continue;
+
+ /* Check the column's default value if one exists */
+ defexpr = (Expr *) build_column_default(resultRelInfo->ri_RelationDesc, attnum);
+ if (defexpr == NULL)
+ continue;
+
+ /* Run the expression through planner */
+ // defexpr = expression_planner(defexpr);
+ // (void) ExecInitExpr(defexpr, NULL);
+ expression_planner(defexpr);
+
+ if (contain_volatile_functions_not_nextval((Node *) defexpr))
+ {
+ elog(DEBUG1, "found volatile att %d", attnum);
+ return true;
+ }
+ }
+
+ return false;
+}
+
/* ----------------------------------------------------------------
* ExecInitModifyTable
* ----------------------------------------------------------------
@@ -2352,10 +2408,10 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
* are any statement level insert triggers.
*/
mtstate->miinfo = NULL;
- else if (mtstate->rootResultRelInfo->ri_FdwRoutine != NULL
- /* || cstate->volatile_defexprs */ )
- // XXX contain_volatile_functions_not_nextval((Node *) defexpr);
- /* Can't support multi-inserts to foreign tables or if there are any */
+ else if (mtstate->rootResultRelInfo->ri_FdwRoutine != NULL ||
+ has_volatile_defaults(mtstate->rootResultRelInfo, node))
+ /* Can't support multi-inserts to foreign tables or if there are any
+ * volatile default expressions in the table. */
mtstate->miinfo = NULL;
else
{
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index ba3ccc712c..2008e8e5d6 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2178,6 +2178,7 @@ _copyTargetEntry(const TargetEntry *from)
COPY_SCALAR_FIELD(resorigtbl);
COPY_SCALAR_FIELD(resorigcol);
COPY_SCALAR_FIELD(resjunk);
+ COPY_SCALAR_FIELD(isdefault);
return newnode;
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index a2ef853dc2..aa3cdf3729 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -770,6 +770,7 @@ _equalTargetEntry(const TargetEntry *a, const TargetEntry *b)
COMPARE_SCALAR_FIELD(resorigtbl);
COMPARE_SCALAR_FIELD(resorigcol);
COMPARE_SCALAR_FIELD(resjunk);
+ COMPARE_SCALAR_FIELD(isdefault);
return true;
}
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index 01c110cd2f..aeeba7032f 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -254,6 +254,7 @@ makeTargetEntry(Expr *expr,
tle->ressortgroupref = 0;
tle->resorigtbl = InvalidOid;
tle->resorigcol = 0;
+ tle->isdefault = false;
tle->resjunk = resjunk;
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 8392be6d44..924ffda1c6 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1661,6 +1661,7 @@ _outTargetEntry(StringInfo str, const TargetEntry *node)
WRITE_OID_FIELD(resorigtbl);
WRITE_INT_FIELD(resorigcol);
WRITE_BOOL_FIELD(resjunk);
+ WRITE_BOOL_FIELD(isdefault);
}
static void
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index d2c8d58070..642b6c63c5 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1275,6 +1275,7 @@ _readTargetEntry(void)
READ_OID_FIELD(resorigtbl);
READ_INT_FIELD(resorigcol);
READ_BOOL_FIELD(resjunk);
+ READ_BOOL_FIELD(isdefault);
READ_DONE();
}
diff --git a/src/backend/optimizer/util/tlist.c b/src/backend/optimizer/util/tlist.c
index 89853a0630..7ef1517027 100644
--- a/src/backend/optimizer/util/tlist.c
+++ b/src/backend/optimizer/util/tlist.c
@@ -349,6 +349,7 @@ apply_tlist_labeling(List *dest_tlist, List *src_tlist)
dest_tle->resorigtbl = src_tle->resorigtbl;
dest_tle->resorigcol = src_tle->resorigcol;
dest_tle->resjunk = src_tle->resjunk;
+ dest_tle->isdefault = src_tle->isdefault;
}
}
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 0c7508a0d8..7ab13e51e5 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -986,10 +986,13 @@ rewriteTargetListIU(List *targetList,
}
if (new_expr)
+ {
new_tle = makeTargetEntry((Expr *) new_expr,
attrno,
pstrdup(NameStr(att_tup->attname)),
false);
+ new_tle->isdefault = true;
+ }
}
/*
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index d4ce037088..888bd36a07 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1437,6 +1437,8 @@ typedef struct TargetEntry
AttrNumber resorigcol; /* column's number in source table */
bool resjunk; /* set to true to eliminate the attribute from
* final target list */
+ bool isdefault; /* true if using the column default, either
+ * by "DEFAULT" or omission of the column */
} TargetEntry;
--
2.17.0
--3yNHWXBV/QO9xKNm
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v9-0003-COPY-flush-multi-insert-buffer-based-on-accumulat.patch"
^ permalink raw reply [nested|flat] 23+ messages in thread
* Push down time-related SQLValue functions to foreign server
@ 2021-08-19 09:51 Alexander Pyhalov <[email protected]>
0 siblings, 3 replies; 23+ messages in thread
From: Alexander Pyhalov @ 2021-08-19 09:51 UTC (permalink / raw)
To: pgsql-hackers
Hi.
The attached patches allow pushing down
current_timestamp/localtimestamp/current_time/localtime and now() to
remote PostgreSQL server as locally computed parameters.
The idea is based on oracle_fdw behavior.
Examples.
\d test
Foreign table "public.test"
Column | Type | Collation | Nullable | Default |
FDW options
--------+--------------------------+-----------+----------+---------+-------------------
i | integer | | | |
(column_name 'i')
t | timestamp with time zone | | | |
(column_name 't')
Server: loopback
FDW options: (schema_name 'data', table_name 'test')
Prior the patch:
explain verbose select * from test where t=current_timestamp;
QUERY PLAN
---------------------------------------------------------------------
Foreign Scan on public.test (cost=100.00..188.12 rows=11 width=12)
Output: i, t
Filter: (test.t = CURRENT_TIMESTAMP)
Remote SQL: SELECT i, t FROM data.test
explain verbose update test set t=current_timestamp where t<now();
QUERY PLAN
----------------------------------------------------------------------------
Update on public.test (cost=100.00..154.47 rows=0 width=0)
Remote SQL: UPDATE data.test SET t = $2 WHERE ctid = $1
-> Foreign Scan on public.test (cost=100.00..154.47 rows=414
width=50)
Output: CURRENT_TIMESTAMP, ctid, test.*
Filter: (test.t < now())
Remote SQL: SELECT i, t, ctid FROM data.test FOR UPDATE
After patch:
explain verbose select * from test where t=current_timestamp;
QUERY PLAN
-------------------------------------------------------------------------------------
Foreign Scan on public.test (cost=100.00..144.35 rows=11 width=12)
Output: i, t
Remote SQL: SELECT i, t FROM data.test WHERE ((t = $1::timestamp with
time zone))
explain verbose update test set t=current_timestamp where t<now();
QUERY PLAN
----------------------------------------------------------------------------------------------------------------------
Update on public.test (cost=100.00..137.93 rows=0 width=0)
-> Foreign Update on public.test (cost=100.00..137.93 rows=414
width=50)
Remote SQL: UPDATE data.test SET t = $1::timestamp with time
zone WHERE ((t < $1::timestamp with time zone))
--
Best regards,
Alexander Pyhalov,
Postgres Professional
Attachments:
[text/x-diff] 0001-SQLValue-functions-pushdown.patch (15.4K, ../../[email protected]/2-0001-SQLValue-functions-pushdown.patch)
download | inline diff:
From 0846ba1d3a5f15bbea449b39741f08558fdb2d49 Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov <[email protected]>
Date: Thu, 29 Jul 2021 11:45:28 +0300
Subject: [PATCH 1/2] 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 | 100 +++++++++++++++-
.../postgres_fdw/expected/postgres_fdw.out | 108 ++++++++++++++++++
contrib/postgres_fdw/postgres_fdw.h | 1 +
contrib/postgres_fdw/shippable.c | 68 +++++++++++
contrib/postgres_fdw/sql/postgres_fdw.sql | 29 +++++
5 files changed, 305 insertions(+), 1 deletion(-)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index d98bd666818..01748835fc8 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -157,6 +157,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);
@@ -273,7 +274,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_unsafe_functions((Node *) expr))
return false;
/* OK to evaluate on the remote server */
@@ -618,6 +619,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;
@@ -1031,6 +1056,21 @@ is_foreign_param(PlannerInfo *root,
case T_Param:
/* Params always have to be sent to the foreign server */
return true;
+ case T_SQLValueFunction:
+ {
+ SQLValueFunction *s = (SQLValueFunction *) expr;
+
+ 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 true;
+ break;
+ }
default:
break;
}
@@ -2603,6 +2643,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;
@@ -3092,6 +3135,61 @@ deparseRelabelType(RelabelType *node, deparse_expr_cxt *context)
node->resulttypmod));
}
+/*
+ * Deparse a SQLValueFunction node
+ */
+static void
+deparseSQLValueFunction(SQLValueFunction *node, deparse_expr_cxt *context)
+{
+ int32 typmod = -1;
+
+ switch (node->op)
+ {
+ case SVFOP_LOCALTIME:
+ case SVFOP_CURRENT_TIME:
+ case SVFOP_LOCALTIMESTAMP:
+ case SVFOP_CURRENT_TIMESTAMP:
+ break;
+ case SVFOP_LOCALTIME_N:
+ case SVFOP_CURRENT_TIME_N:
+ case SVFOP_CURRENT_TIMESTAMP_N:
+ case SVFOP_LOCALTIMESTAMP_N:
+ typmod = node->typmod;
+ break;
+ default:
+ elog(ERROR, "unsupported SQLValueFunction op for deparse: %d",
+ node->op);
+ }
+ 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 e3ee30f1aaf..105324e2a00 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -1067,6 +1067,114 @@ 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)
+
+-- parametrized queries
+PREPARE st1(int) AS SELECT c3 FROM ft2 WHERE c1 = $1 AND c5 > localtimestamp - interval '1000 years';
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st1(1);
+ QUERY PLAN
+--------------------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan on public.ft2
+ Output: c3
+ Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE (("C 1" = 1)) AND ((c5 > ($1::timestamp without time zone - '1000 years'::interval)))
+(3 rows)
+
+EXECUTE st1(1);
+ c3
+-------
+ 00001
+(1 row)
+
+DEALLOCATE st1;
+PREPARE st1(int) AS SELECT c3 FROM ft2 WHERE c5 > localtimestamp - interval '1000 years' AND c1 = $1;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st1(1);
+ QUERY PLAN
+--------------------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan on public.ft2
+ Output: c3
+ Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE (("C 1" = 1)) AND ((c5 > ($1::timestamp without time zone - '1000 years'::interval)))
+(3 rows)
+
+EXECUTE st1(1);
+ c3
+-------
+ 00001
+(1 row)
+
+DEALLOCATE st1;
-- 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.h b/contrib/postgres_fdw/postgres_fdw.h
index ca83306af99..4ab199e4ef4 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -229,5 +229,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 b27f82e0155..56d47d0b2f3 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 */
@@ -209,3 +212,68 @@ is_shippable(Oid objectId, Oid classId, PgFdwRelationInfo *fpinfo)
return entry->shippable;
}
+
+static bool
+contain_mutable_functions_checker(Oid func_id, void *context)
+{
+ return (func_volatile(func_id) != PROVOLATILE_IMMUTABLE);
+}
+
+static bool
+contain_unsafe_functions_walker(Node *node, void *context)
+{
+ if (node == NULL)
+ return false;
+ /* Check for mutable functions in node itself */
+ if (check_functions_in_node(node, contain_mutable_functions_checker,
+ context))
+ return true;
+
+ /*
+ * Unlike contain_mutable_functions_walker, don't treat SQLValueFunction
+ * as unsafe - foreign_expr_walker() classifies them
+ */
+
+ if (IsA(node, NextValueExpr))
+ {
+ /* NextValueExpr is volatile */
+ return true;
+ }
+
+ /*
+ * It should be safe to treat MinMaxExpr as immutable, because it will
+ * depend on a non-cross-type btree comparison function, and those should
+ * always be immutable. Treating XmlExpr as immutable is more dubious,
+ * and treating CoerceToDomain as immutable is outright dangerous. But we
+ * have done so historically, and changing this would probably cause more
+ * problems than it would fix. In practice, if you have a non-immutable
+ * domain constraint you are in for pain anyhow.
+ */
+
+ /* Recurse to check arguments */
+ if (IsA(node, Query))
+ {
+ /* Recurse into subselects */
+ return query_tree_walker((Query *) node,
+ contain_unsafe_functions_walker,
+ context, 0);
+ }
+ return expression_tree_walker(node, contain_unsafe_functions_walker,
+ context);
+}
+
+/*
+ * contain_unsafe_functions
+ * Recursively search for unsafe mutable functions within a clause.
+ *
+ * Returns true if any mutable function (or operator implemented by a
+ * mutable function), which is not known to be safe, is found.
+ *
+ * We will recursively look into Query nodes (i.e., SubLink sub-selects)
+ * but not into SubPlans. See comments for contain_volatile_functions().
+ */
+bool
+contain_unsafe_functions(Node *clause)
+{
+ return contain_unsafe_functions_walker(clause, NULL);
+}
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 30b5175da5b..167f966553d 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -408,6 +408,35 @@ 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;
+
+-- parametrized queries
+PREPARE st1(int) AS SELECT c3 FROM ft2 WHERE c1 = $1 AND c5 > localtimestamp - interval '1000 years';
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st1(1);
+EXECUTE st1(1);
+DEALLOCATE st1;
+
+PREPARE st1(int) AS SELECT c3 FROM ft2 WHERE c5 > localtimestamp - interval '1000 years' AND c1 = $1;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st1(1);
+EXECUTE st1(1);
+DEALLOCATE st1;
+
-- 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;
--
2.25.1
[text/x-diff] 0002-now-pushdown.patch (6.6K, ../../[email protected]/3-0002-now-pushdown.patch)
download | inline diff:
From 0820c6b002f5a1df0158c4b507d44462f4993c34 Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov <[email protected]>
Date: Mon, 16 Aug 2021 18:09:09 +0300
Subject: [PATCH 2/2] now() pushdown
Treat now() the same way as current_timestamp and send it to remote
server as parameter value.
---
contrib/postgres_fdw/deparse.c | 34 +++++++++++++++----
.../postgres_fdw/expected/postgres_fdw.out | 15 ++++++++
contrib/postgres_fdw/shippable.c | 12 ++++---
contrib/postgres_fdw/sql/postgres_fdw.sql | 5 +++
4 files changed, 55 insertions(+), 11 deletions(-)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 01748835fc8..9f0be6cb884 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -52,6 +52,7 @@
#include "parser/parsetree.h"
#include "postgres_fdw.h"
#include "utils/builtins.h"
+#include "utils/fmgroids.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
#include "utils/syscache.h"
@@ -269,10 +270,9 @@ is_foreign_expr(PlannerInfo *root,
/*
* An expression which includes any mutable functions can't be sent over
- * because its result is not stable. For example, sending now() remote
- * side could cause confusion from clock offsets. Future versions might
- * be able to make this choice with more granularity. (We check this last
- * because it requires a lot of expensive catalog lookups.)
+ * because its result is not stable. Future versions might be able to
+ * make this choice with more granularity. (We check this last because it
+ * requires a lot of expensive catalog lookups.)
*/
if (contain_unsafe_functions((Node *) expr))
return false;
@@ -625,8 +625,8 @@ foreign_expr_walker(Node *node,
/*
* For now only time-related SQLValue functions are supported.
- * We can push down localtimestamp and localtime as we
- * compute them locally.
+ * We can push down localtimestamp and localtime as we compute
+ * them locally.
*/
if ((s->op != SVFOP_CURRENT_TIMESTAMP) &&
(s->op != SVFOP_CURRENT_TIMESTAMP_N) &&
@@ -1056,6 +1056,14 @@ is_foreign_param(PlannerInfo *root,
case T_Param:
/* Params always have to be sent to the foreign server */
return true;
+ case T_FuncExpr:
+ {
+ FuncExpr *fe = (FuncExpr *) expr;
+
+ if (fe->funcid == F_NOW)
+ return true;
+ break;
+ }
case T_SQLValueFunction:
{
SQLValueFunction *s = (SQLValueFunction *) expr;
@@ -2963,6 +2971,20 @@ deparseFuncExpr(FuncExpr *node, deparse_expr_cxt *context)
return;
}
+ if (node->funcid == F_NOW)
+ {
+ SQLValueFunction *svf = makeNode(SQLValueFunction);
+
+ svf->op = SVFOP_CURRENT_TIMESTAMP;
+ svf->type = TIMESTAMPTZOID;
+ svf->typmod = -1;
+ svf->location = -1;
+
+ deparseSQLValueFunction(svf, context);
+
+ return;
+ }
+
/* Check if need to print VARIADIC (cf. ruleutils.c) */
use_variadic = node->funcvariadic;
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 105324e2a00..f12455a7c74 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -1082,6 +1082,21 @@ SELECT c1,c2,c4 FROM ft2 WHERE c4 = current_timestamp;
----+----+----
(0 rows)
+-- the same with now()
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT c1,c2,c4 FROM ft2 WHERE c4 = now();
+ 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 = now();
+ 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
diff --git a/contrib/postgres_fdw/shippable.c b/contrib/postgres_fdw/shippable.c
index 56d47d0b2f3..d8163d67d46 100644
--- a/contrib/postgres_fdw/shippable.c
+++ b/contrib/postgres_fdw/shippable.c
@@ -28,6 +28,7 @@
#include "catalog/pg_proc.h"
#include "nodes/nodeFuncs.h"
#include "postgres_fdw.h"
+#include "utils/fmgroids.h"
#include "utils/hsearch.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
@@ -214,9 +215,10 @@ is_shippable(Oid objectId, Oid classId, PgFdwRelationInfo *fpinfo)
}
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);
}
static bool
@@ -224,8 +226,8 @@ contain_unsafe_functions_walker(Node *node, void *context)
{
if (node == NULL)
return false;
- /* Check for mutable functions in node itself */
- if (check_functions_in_node(node, contain_mutable_functions_checker,
+ /* Check for unsafe functions in node itself */
+ if (check_functions_in_node(node, contain_unsafe_functions_checker,
context))
return true;
@@ -266,7 +268,7 @@ contain_unsafe_functions_walker(Node *node, void *context)
* contain_unsafe_functions
* Recursively search for unsafe mutable functions within a clause.
*
- * Returns true if any mutable function (or operator implemented by a
+ * Returns true if any unsafe mutable function (or operator implemented by a
* mutable function), which is not known to be safe, is found.
*
* We will recursively look into Query nodes (i.e., SubLink sub-selects)
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 167f966553d..55046f25c50 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -413,6 +413,11 @@ EXPLAIN (VERBOSE, COSTS OFF)
SELECT c1,c2,c4 FROM ft2 WHERE c4 = current_timestamp;
SELECT c1,c2,c4 FROM ft2 WHERE c4 = current_timestamp;
+-- the same with now()
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT c1,c2,c4 FROM ft2 WHERE c4 = now();
+SELECT c1,c2,c4 FROM ft2 WHERE c4 = now();
+
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;
--
2.25.1
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2021-08-19 10:22 Zhihong Yu <[email protected]>
parent: Alexander Pyhalov <[email protected]>
2 siblings, 1 reply; 23+ messages in thread
From: Zhihong Yu @ 2021-08-19 10:22 UTC (permalink / raw)
To: Alexander Pyhalov <[email protected]>; +Cc: pgsql-hackers
On Thu, Aug 19, 2021 at 2:52 AM Alexander Pyhalov <[email protected]>
wrote:
> Hi.
>
> The attached patches allow pushing down
> current_timestamp/localtimestamp/current_time/localtime and now() to
> remote PostgreSQL server as locally computed parameters.
> The idea is based on oracle_fdw behavior.
>
> Examples.
>
> \d test
> Foreign table "public.test"
> Column | Type | Collation | Nullable | Default |
> FDW options
>
> --------+--------------------------+-----------+----------+---------+-------------------
> i | integer | | | |
> (column_name 'i')
> t | timestamp with time zone | | | |
> (column_name 't')
> Server: loopback
> FDW options: (schema_name 'data', table_name 'test')
>
> Prior the patch:
>
> explain verbose select * from test where t=current_timestamp;
> QUERY PLAN
> ---------------------------------------------------------------------
> Foreign Scan on public.test (cost=100.00..188.12 rows=11 width=12)
> Output: i, t
> Filter: (test.t = CURRENT_TIMESTAMP)
> Remote SQL: SELECT i, t FROM data.test
>
> explain verbose update test set t=current_timestamp where t<now();
> QUERY PLAN
>
> ----------------------------------------------------------------------------
> Update on public.test (cost=100.00..154.47 rows=0 width=0)
> Remote SQL: UPDATE data.test SET t = $2 WHERE ctid = $1
> -> Foreign Scan on public.test (cost=100.00..154.47 rows=414
> width=50)
> Output: CURRENT_TIMESTAMP, ctid, test.*
> Filter: (test.t < now())
> Remote SQL: SELECT i, t, ctid FROM data.test FOR UPDATE
>
>
> After patch:
> explain verbose select * from test where t=current_timestamp;
> QUERY PLAN
>
> -------------------------------------------------------------------------------------
> Foreign Scan on public.test (cost=100.00..144.35 rows=11 width=12)
> Output: i, t
> Remote SQL: SELECT i, t FROM data.test WHERE ((t = $1::timestamp with
> time zone))
>
> explain verbose update test set t=current_timestamp where t<now();
> QUERY PLAN
>
> ----------------------------------------------------------------------------------------------------------------------
> Update on public.test (cost=100.00..137.93 rows=0 width=0)
> -> Foreign Update on public.test (cost=100.00..137.93 rows=414
> width=50)
> Remote SQL: UPDATE data.test SET t = $1::timestamp with time
> zone WHERE ((t < $1::timestamp with time zone))
>
> --
> Best regards,
> Alexander Pyhalov,
> Postgres Professional
Hi,
For 0002 patch:
+ /* 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);
Did you mean to say 'now() is unstable' ?
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2021-08-19 10:24 Alexander Pyhalov <[email protected]>
parent: Zhihong Yu <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Alexander Pyhalov @ 2021-08-19 10:24 UTC (permalink / raw)
To: Zhihong Yu <[email protected]>; +Cc: pgsql-hackers
Zhihong Yu писал 2021-08-19 13:22:
> Hi,
> For 0002 patch:
>
> + /* 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);
>
> Did you mean to say 'now() is unstable' ?
No, it's stable, not immutable, so we need additional check.
--
Best regards,
Alexander Pyhalov,
Postgres Professional
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2021-08-19 10:55 Zhihong Yu <[email protected]>
parent: Alexander Pyhalov <[email protected]>
2 siblings, 1 reply; 23+ messages in thread
From: Zhihong Yu @ 2021-08-19 10:55 UTC (permalink / raw)
To: Alexander Pyhalov <[email protected]>; +Cc: pgsql-hackers
On Thu, Aug 19, 2021 at 2:52 AM Alexander Pyhalov <[email protected]>
wrote:
> Hi.
>
> The attached patches allow pushing down
> current_timestamp/localtimestamp/current_time/localtime and now() to
> remote PostgreSQL server as locally computed parameters.
> The idea is based on oracle_fdw behavior.
>
> Examples.
>
> \d test
> Foreign table "public.test"
> Column | Type | Collation | Nullable | Default |
> FDW options
>
> --------+--------------------------+-----------+----------+---------+-------------------
> i | integer | | | |
> (column_name 'i')
> t | timestamp with time zone | | | |
> (column_name 't')
> Server: loopback
> FDW options: (schema_name 'data', table_name 'test')
>
> Prior the patch:
>
> explain verbose select * from test where t=current_timestamp;
> QUERY PLAN
> ---------------------------------------------------------------------
> Foreign Scan on public.test (cost=100.00..188.12 rows=11 width=12)
> Output: i, t
> Filter: (test.t = CURRENT_TIMESTAMP)
> Remote SQL: SELECT i, t FROM data.test
>
> explain verbose update test set t=current_timestamp where t<now();
> QUERY PLAN
>
> ----------------------------------------------------------------------------
> Update on public.test (cost=100.00..154.47 rows=0 width=0)
> Remote SQL: UPDATE data.test SET t = $2 WHERE ctid = $1
> -> Foreign Scan on public.test (cost=100.00..154.47 rows=414
> width=50)
> Output: CURRENT_TIMESTAMP, ctid, test.*
> Filter: (test.t < now())
> Remote SQL: SELECT i, t, ctid FROM data.test FOR UPDATE
>
>
> After patch:
> explain verbose select * from test where t=current_timestamp;
> QUERY PLAN
>
> -------------------------------------------------------------------------------------
> Foreign Scan on public.test (cost=100.00..144.35 rows=11 width=12)
> Output: i, t
> Remote SQL: SELECT i, t FROM data.test WHERE ((t = $1::timestamp with
> time zone))
>
> explain verbose update test set t=current_timestamp where t<now();
> QUERY PLAN
>
> ----------------------------------------------------------------------------------------------------------------------
> Update on public.test (cost=100.00..137.93 rows=0 width=0)
> -> Foreign Update on public.test (cost=100.00..137.93 rows=414
> width=50)
> Remote SQL: UPDATE data.test SET t = $1::timestamp with time
> zone WHERE ((t < $1::timestamp with time zone))
>
> --
> Best regards,
> Alexander Pyhalov,
> Postgres Professional
Hi,
For 0001 patch:
+ if ((s->op != SVFOP_CURRENT_TIMESTAMP) &&
+ (s->op != SVFOP_CURRENT_TIMESTAMP_N) &&
+ (s->op != SVFOP_CURRENT_TIME) &&
...
The above check appears more than once. If extracted into a helper method,
it would help reduce duplicate and make the code more readable.
Cheers
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2021-08-19 11:01 Ranier Vilela <[email protected]>
parent: Zhihong Yu <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Ranier Vilela @ 2021-08-19 11:01 UTC (permalink / raw)
To: Zhihong Yu <[email protected]>; +Cc: Alexander Pyhalov <[email protected]>; pgsql-hackers
Em qui., 19 de ago. de 2021 às 07:50, Zhihong Yu <[email protected]>
escreveu:
>
>
> On Thu, Aug 19, 2021 at 2:52 AM Alexander Pyhalov <
> [email protected]> wrote:
>
>> Hi.
>>
>> The attached patches allow pushing down
>> current_timestamp/localtimestamp/current_time/localtime and now() to
>> remote PostgreSQL server as locally computed parameters.
>> The idea is based on oracle_fdw behavior.
>>
>> Examples.
>>
>> \d test
>> Foreign table "public.test"
>> Column | Type | Collation | Nullable | Default |
>> FDW options
>>
>> --------+--------------------------+-----------+----------+---------+-------------------
>> i | integer | | | |
>> (column_name 'i')
>> t | timestamp with time zone | | | |
>> (column_name 't')
>> Server: loopback
>> FDW options: (schema_name 'data', table_name 'test')
>>
>> Prior the patch:
>>
>> explain verbose select * from test where t=current_timestamp;
>> QUERY PLAN
>> ---------------------------------------------------------------------
>> Foreign Scan on public.test (cost=100.00..188.12 rows=11 width=12)
>> Output: i, t
>> Filter: (test.t = CURRENT_TIMESTAMP)
>> Remote SQL: SELECT i, t FROM data.test
>>
>> explain verbose update test set t=current_timestamp where t<now();
>> QUERY PLAN
>>
>> ----------------------------------------------------------------------------
>> Update on public.test (cost=100.00..154.47 rows=0 width=0)
>> Remote SQL: UPDATE data.test SET t = $2 WHERE ctid = $1
>> -> Foreign Scan on public.test (cost=100.00..154.47 rows=414
>> width=50)
>> Output: CURRENT_TIMESTAMP, ctid, test.*
>> Filter: (test.t < now())
>> Remote SQL: SELECT i, t, ctid FROM data.test FOR UPDATE
>>
>>
>> After patch:
>> explain verbose select * from test where t=current_timestamp;
>> QUERY PLAN
>>
>> -------------------------------------------------------------------------------------
>> Foreign Scan on public.test (cost=100.00..144.35 rows=11 width=12)
>> Output: i, t
>> Remote SQL: SELECT i, t FROM data.test WHERE ((t = $1::timestamp with
>> time zone))
>>
>> explain verbose update test set t=current_timestamp where t<now();
>> QUERY PLAN
>>
>> ----------------------------------------------------------------------------------------------------------------------
>> Update on public.test (cost=100.00..137.93 rows=0 width=0)
>> -> Foreign Update on public.test (cost=100.00..137.93 rows=414
>> width=50)
>> Remote SQL: UPDATE data.test SET t = $1::timestamp with time
>> zone WHERE ((t < $1::timestamp with time zone))
>>
>> --
>> Best regards,
>> Alexander Pyhalov,
>> Postgres Professional
>
> Hi,
> For 0001 patch:
>
> + if ((s->op != SVFOP_CURRENT_TIMESTAMP) &&
> + (s->op != SVFOP_CURRENT_TIMESTAMP_N) &&
> + (s->op != SVFOP_CURRENT_TIME) &&
> ...
>
> The above check appears more than once. If extracted into a helper method,
> it would help reduce duplicate and make the code more readable.
>
Perhaps in a MACRO?
regards,
Ranier Vilela
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2021-08-19 14:01 Ashutosh Bapat <[email protected]>
parent: Alexander Pyhalov <[email protected]>
2 siblings, 1 reply; 23+ messages in thread
From: Ashutosh Bapat @ 2021-08-19 14:01 UTC (permalink / raw)
To: Alexander Pyhalov <[email protected]>; +Cc: pgsql-hackers
I spent some time looking at this patch.
Generally it looks like a good idea. These stable functions will be
evaluated at the execution time and replaced with constants. I am not
sure whether the nodes saved in the param_list may not get the same
treatment. Have you verified that?
Also the new node types being added to the param list is something
other than Param. So it conflicts with the comment below in
prepare_query_params()?
/*
* 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.)
*/
If we are already adding non-params to this list, then the comment is outdated?
On Thu, Aug 19, 2021 at 3:22 PM Alexander Pyhalov
<[email protected]> wrote:
>
> Hi.
>
> The attached patches allow pushing down
> current_timestamp/localtimestamp/current_time/localtime and now() to
> remote PostgreSQL server as locally computed parameters.
> The idea is based on oracle_fdw behavior.
>
> Examples.
>
> \d test
> Foreign table "public.test"
> Column | Type | Collation | Nullable | Default |
> FDW options
> --------+--------------------------+-----------+----------+---------+-------------------
> i | integer | | | |
> (column_name 'i')
> t | timestamp with time zone | | | |
> (column_name 't')
> Server: loopback
> FDW options: (schema_name 'data', table_name 'test')
>
> Prior the patch:
>
> explain verbose select * from test where t=current_timestamp;
> QUERY PLAN
> ---------------------------------------------------------------------
> Foreign Scan on public.test (cost=100.00..188.12 rows=11 width=12)
> Output: i, t
> Filter: (test.t = CURRENT_TIMESTAMP)
> Remote SQL: SELECT i, t FROM data.test
>
> explain verbose update test set t=current_timestamp where t<now();
> QUERY PLAN
> ----------------------------------------------------------------------------
> Update on public.test (cost=100.00..154.47 rows=0 width=0)
> Remote SQL: UPDATE data.test SET t = $2 WHERE ctid = $1
> -> Foreign Scan on public.test (cost=100.00..154.47 rows=414
> width=50)
> Output: CURRENT_TIMESTAMP, ctid, test.*
> Filter: (test.t < now())
> Remote SQL: SELECT i, t, ctid FROM data.test FOR UPDATE
>
>
> After patch:
> explain verbose select * from test where t=current_timestamp;
> QUERY PLAN
> -------------------------------------------------------------------------------------
> Foreign Scan on public.test (cost=100.00..144.35 rows=11 width=12)
> Output: i, t
> Remote SQL: SELECT i, t FROM data.test WHERE ((t = $1::timestamp with
> time zone))
>
> explain verbose update test set t=current_timestamp where t<now();
> QUERY PLAN
> ----------------------------------------------------------------------------------------------------------------------
> Update on public.test (cost=100.00..137.93 rows=0 width=0)
> -> Foreign Update on public.test (cost=100.00..137.93 rows=414
> width=50)
> Remote SQL: UPDATE data.test SET t = $1::timestamp with time
> zone WHERE ((t < $1::timestamp with time zone))
>
> --
> Best regards,
> Alexander Pyhalov,
> Postgres Professional
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2021-08-20 07:13 Alexander Pyhalov <[email protected]>
parent: Ranier Vilela <[email protected]>
0 siblings, 3 replies; 23+ messages in thread
From: Alexander Pyhalov @ 2021-08-20 07:13 UTC (permalink / raw)
To: Ranier Vilela <[email protected]>; +Cc: Zhihong Yu <[email protected]>; pgsql-hackers
Hi.
Ranier Vilela писал 2021-08-19 14:01:
> Em qui., 19 de ago. de 2021 às 07:50, Zhihong Yu <[email protected]>
>> Hi,
>> For 0001 patch:
>>
>> + if ((s->op != SVFOP_CURRENT_TIMESTAMP) &&
>> + (s->op != SVFOP_CURRENT_TIMESTAMP_N) &&
>> + (s->op != SVFOP_CURRENT_TIME) &&
>> ...
>>
>> The above check appears more than once. If extracted into a helper
>> method, it would help reduce duplicate and make the code more
>> readable.
>
> 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().
--
Best regards,
Alexander Pyhalov,
Postgres Professional
Attachments:
[text/x-diff] 0001-SQLValue-functions-pushdown.patch (17.2K, ../../[email protected]/2-0001-SQLValue-functions-pushdown.patch)
download | inline diff:
From 2cfd3e42cad07ed552a1eb23b06040b0f74a7f2f Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov <[email protected]>
Date: Thu, 29 Jul 2021 11:45:28 +0300
Subject: [PATCH 1/2] 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 | 95 +++++++++++++-
.../postgres_fdw/expected/postgres_fdw.out | 121 ++++++++++++++++++
contrib/postgres_fdw/postgres_fdw.c | 9 +-
contrib/postgres_fdw/postgres_fdw.h | 1 +
contrib/postgres_fdw/shippable.c | 68 ++++++++++
contrib/postgres_fdw/sql/postgres_fdw.sql | 33 +++++
6 files changed, 320 insertions(+), 7 deletions(-)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index d98bd666818..6c99acd0c82 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
@@ -157,6 +166,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);
@@ -273,7 +283,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_unsafe_functions((Node *) expr))
return false;
/* OK to evaluate on the remote server */
@@ -618,6 +628,23 @@ 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 (!TIME_RELATED_SQLVALUE_FUNCTION(s))
+ return false;
+
+ /* Timestamp or time are not collatable */
+ collation = InvalidOid;
+ state = FDW_COLLATE_NONE;
+ }
+ break;
case T_BoolExpr:
{
BoolExpr *b = (BoolExpr *) node;
@@ -1031,6 +1058,14 @@ is_foreign_param(PlannerInfo *root,
case T_Param:
/* Params always have to be sent to the foreign server */
return true;
+ case T_SQLValueFunction:
+ {
+ SQLValueFunction *s = (SQLValueFunction *) expr;
+
+ if (TIME_RELATED_SQLVALUE_FUNCTION(s))
+ return true;
+ break;
+ }
default:
break;
}
@@ -2603,6 +2638,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;
@@ -3092,6 +3130,61 @@ deparseRelabelType(RelabelType *node, deparse_expr_cxt *context)
node->resulttypmod));
}
+/*
+ * Deparse a SQLValueFunction node
+ */
+static void
+deparseSQLValueFunction(SQLValueFunction *node, deparse_expr_cxt *context)
+{
+ int32 typmod = -1;
+
+ switch (node->op)
+ {
+ case SVFOP_LOCALTIME:
+ case SVFOP_CURRENT_TIME:
+ case SVFOP_LOCALTIMESTAMP:
+ case SVFOP_CURRENT_TIMESTAMP:
+ break;
+ case SVFOP_LOCALTIME_N:
+ case SVFOP_CURRENT_TIME_N:
+ case SVFOP_CURRENT_TIMESTAMP_N:
+ case SVFOP_LOCALTIMESTAMP_N:
+ typmod = node->typmod;
+ break;
+ default:
+ elog(ERROR, "unsupported SQLValueFunction op for deparse: %d",
+ node->op);
+ }
+ 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 e3ee30f1aaf..64367ae986d 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -1067,6 +1067,127 @@ 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)
+
+-- parametrized queries
+PREPARE st1(int) AS SELECT c3 FROM ft2 WHERE c1 = $1 AND c5 > localtimestamp - interval '1000 years';
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st1(1);
+ QUERY PLAN
+--------------------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan on public.ft2
+ Output: c3
+ Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE (("C 1" = 1)) AND ((c5 > ($1::timestamp without time zone - '1000 years'::interval)))
+(3 rows)
+
+EXECUTE st1(1);
+ c3
+-------
+ 00001
+(1 row)
+
+DEALLOCATE st1;
+PREPARE st1(int) AS SELECT c3 FROM ft2 WHERE c5 > localtimestamp - interval '1000 years' AND c1 = $1;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st1(1);
+ QUERY PLAN
+--------------------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan on public.ft2
+ Output: c3
+ Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE (("C 1" = 1)) AND ((c5 > ($1::timestamp without time zone - '1000 years'::interval)))
+(3 rows)
+
+EXECUTE st1(1);
+ c3
+-------
+ 00001
+(1 row)
+
+DEALLOCATE st1;
+-- 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 9d443baf02a..9ad6807513c 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -4833,12 +4833,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 ca83306af99..4ab199e4ef4 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -229,5 +229,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 b27f82e0155..56d47d0b2f3 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 */
@@ -209,3 +212,68 @@ is_shippable(Oid objectId, Oid classId, PgFdwRelationInfo *fpinfo)
return entry->shippable;
}
+
+static bool
+contain_mutable_functions_checker(Oid func_id, void *context)
+{
+ return (func_volatile(func_id) != PROVOLATILE_IMMUTABLE);
+}
+
+static bool
+contain_unsafe_functions_walker(Node *node, void *context)
+{
+ if (node == NULL)
+ return false;
+ /* Check for mutable functions in node itself */
+ if (check_functions_in_node(node, contain_mutable_functions_checker,
+ context))
+ return true;
+
+ /*
+ * Unlike contain_mutable_functions_walker, don't treat SQLValueFunction
+ * as unsafe - foreign_expr_walker() classifies them
+ */
+
+ if (IsA(node, NextValueExpr))
+ {
+ /* NextValueExpr is volatile */
+ return true;
+ }
+
+ /*
+ * It should be safe to treat MinMaxExpr as immutable, because it will
+ * depend on a non-cross-type btree comparison function, and those should
+ * always be immutable. Treating XmlExpr as immutable is more dubious,
+ * and treating CoerceToDomain as immutable is outright dangerous. But we
+ * have done so historically, and changing this would probably cause more
+ * problems than it would fix. In practice, if you have a non-immutable
+ * domain constraint you are in for pain anyhow.
+ */
+
+ /* Recurse to check arguments */
+ if (IsA(node, Query))
+ {
+ /* Recurse into subselects */
+ return query_tree_walker((Query *) node,
+ contain_unsafe_functions_walker,
+ context, 0);
+ }
+ return expression_tree_walker(node, contain_unsafe_functions_walker,
+ context);
+}
+
+/*
+ * contain_unsafe_functions
+ * Recursively search for unsafe mutable functions within a clause.
+ *
+ * Returns true if any mutable function (or operator implemented by a
+ * mutable function), which is not known to be safe, is found.
+ *
+ * We will recursively look into Query nodes (i.e., SubLink sub-selects)
+ * but not into SubPlans. See comments for contain_volatile_functions().
+ */
+bool
+contain_unsafe_functions(Node *clause)
+{
+ return contain_unsafe_functions_walker(clause, NULL);
+}
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 30b5175da5b..e6c3ac75a53 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -408,6 +408,39 @@ 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;
+
+-- parametrized queries
+PREPARE st1(int) AS SELECT c3 FROM ft2 WHERE c1 = $1 AND c5 > localtimestamp - interval '1000 years';
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st1(1);
+EXECUTE st1(1);
+DEALLOCATE st1;
+
+PREPARE st1(int) AS SELECT c3 FROM ft2 WHERE c5 > localtimestamp - interval '1000 years' AND c1 = $1;
+EXPLAIN (VERBOSE, COSTS OFF) EXECUTE st1(1);
+EXECUTE st1(1);
+DEALLOCATE st1;
+
+-- 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;
--
2.25.1
[text/x-diff] 0002-now-pushdown.patch (6.5K, ../../[email protected]/3-0002-now-pushdown.patch)
download | inline diff:
From c00862932cccf3243264cfed6e98f653d29efc3d Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov <[email protected]>
Date: Mon, 16 Aug 2021 18:09:09 +0300
Subject: [PATCH 2/2] now() pushdown
Treat now() the same way as current_timestamp and send it to remote
server as parameter value.
---
contrib/postgres_fdw/deparse.c | 34 +++++++++++++++----
.../postgres_fdw/expected/postgres_fdw.out | 15 ++++++++
contrib/postgres_fdw/shippable.c | 12 ++++---
contrib/postgres_fdw/sql/postgres_fdw.sql | 5 +++
4 files changed, 55 insertions(+), 11 deletions(-)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 6c99acd0c82..bd9bea670be 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -52,6 +52,7 @@
#include "parser/parsetree.h"
#include "postgres_fdw.h"
#include "utils/builtins.h"
+#include "utils/fmgroids.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
#include "utils/syscache.h"
@@ -278,10 +279,9 @@ is_foreign_expr(PlannerInfo *root,
/*
* An expression which includes any mutable functions can't be sent over
- * because its result is not stable. For example, sending now() remote
- * side could cause confusion from clock offsets. Future versions might
- * be able to make this choice with more granularity. (We check this last
- * because it requires a lot of expensive catalog lookups.)
+ * because its result is not stable. Future versions might be able to
+ * make this choice with more granularity. (We check this last because it
+ * requires a lot of expensive catalog lookups.)
*/
if (contain_unsafe_functions((Node *) expr))
return false;
@@ -634,8 +634,8 @@ foreign_expr_walker(Node *node,
/*
* For now only time-related SQLValue functions are supported.
- * We can push down localtimestamp and localtime as we
- * compute them locally.
+ * We can push down localtimestamp and localtime as we compute
+ * them locally.
*/
if (!TIME_RELATED_SQLVALUE_FUNCTION(s))
return false;
@@ -1058,6 +1058,14 @@ is_foreign_param(PlannerInfo *root,
case T_Param:
/* Params always have to be sent to the foreign server */
return true;
+ case T_FuncExpr:
+ {
+ FuncExpr *fe = (FuncExpr *) expr;
+
+ if (fe->funcid == F_NOW)
+ return true;
+ break;
+ }
case T_SQLValueFunction:
{
SQLValueFunction *s = (SQLValueFunction *) expr;
@@ -2958,6 +2966,20 @@ deparseFuncExpr(FuncExpr *node, deparse_expr_cxt *context)
return;
}
+ if (node->funcid == F_NOW)
+ {
+ SQLValueFunction *svf = makeNode(SQLValueFunction);
+
+ svf->op = SVFOP_CURRENT_TIMESTAMP;
+ svf->type = TIMESTAMPTZOID;
+ svf->typmod = -1;
+ svf->location = -1;
+
+ deparseSQLValueFunction(svf, context);
+
+ return;
+ }
+
/* Check if need to print VARIADIC (cf. ruleutils.c) */
use_variadic = node->funcvariadic;
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 64367ae986d..bc9b3c996da 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -1082,6 +1082,21 @@ SELECT c1,c2,c4 FROM ft2 WHERE c4 = current_timestamp;
----+----+----
(0 rows)
+-- the same with now()
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT c1,c2,c4 FROM ft2 WHERE c4 = now();
+ 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 = now();
+ 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
diff --git a/contrib/postgres_fdw/shippable.c b/contrib/postgres_fdw/shippable.c
index 56d47d0b2f3..d8163d67d46 100644
--- a/contrib/postgres_fdw/shippable.c
+++ b/contrib/postgres_fdw/shippable.c
@@ -28,6 +28,7 @@
#include "catalog/pg_proc.h"
#include "nodes/nodeFuncs.h"
#include "postgres_fdw.h"
+#include "utils/fmgroids.h"
#include "utils/hsearch.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
@@ -214,9 +215,10 @@ is_shippable(Oid objectId, Oid classId, PgFdwRelationInfo *fpinfo)
}
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);
}
static bool
@@ -224,8 +226,8 @@ contain_unsafe_functions_walker(Node *node, void *context)
{
if (node == NULL)
return false;
- /* Check for mutable functions in node itself */
- if (check_functions_in_node(node, contain_mutable_functions_checker,
+ /* Check for unsafe functions in node itself */
+ if (check_functions_in_node(node, contain_unsafe_functions_checker,
context))
return true;
@@ -266,7 +268,7 @@ contain_unsafe_functions_walker(Node *node, void *context)
* contain_unsafe_functions
* Recursively search for unsafe mutable functions within a clause.
*
- * Returns true if any mutable function (or operator implemented by a
+ * Returns true if any unsafe mutable function (or operator implemented by a
* mutable function), which is not known to be safe, is found.
*
* We will recursively look into Query nodes (i.e., SubLink sub-selects)
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index e6c3ac75a53..4a6a52a355b 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -413,6 +413,11 @@ EXPLAIN (VERBOSE, COSTS OFF)
SELECT c1,c2,c4 FROM ft2 WHERE c4 = current_timestamp;
SELECT c1,c2,c4 FROM ft2 WHERE c4 = current_timestamp;
+-- the same with now()
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT c1,c2,c4 FROM ft2 WHERE c4 = now();
+SELECT c1,c2,c4 FROM ft2 WHERE c4 = now();
+
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;
--
2.25.1
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2021-08-20 07:36 Alexander Pyhalov <[email protected]>
parent: Ashutosh Bapat <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Alexander Pyhalov @ 2021-08-20 07:36 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: pgsql-hackers
Hi.
Ashutosh Bapat писал 2021-08-19 17:01:
> I spent some time looking at this patch.
>
> Generally it looks like a good idea. These stable functions will be
> evaluated at the execution time and replaced with constants. I am not
> sure whether the nodes saved in the param_list may not get the same
> treatment. Have you verified that?
I'm not sure I understand you. All parameters are treated in the same
way.
They are evaluated in process_query_params(), real params and
parameters, corresponding to our SQLValue functions.
If we look at execution of something like
explain verbose select * from test1 t1 where i in (select i from test1
t2 where t2.t< now() and t1.i=t2.i) ;
QUERY PLAN
-------------------------------------------------------------------------------------------------------------------
Foreign Scan on public.test1 t1 (cost=100.00..243310.11 rows=930
width=20)
Output: t1.i, t1.t, t1.l
Filter: (SubPlan 1)
Remote SQL: SELECT i, t, l FROM data.test1
SubPlan 1
-> Foreign Scan on public.test1 t2 (cost=100.00..161.29 rows=5
width=4)
Output: t2.i
Remote SQL: SELECT i FROM data.test1 WHERE (($1::integer =
i)) AND ((t < $2::timestamp with time zone)
we can see two parameters evaluated in process_query_params(), one - of
T_Param type (with value of current t1.i) and one of T_SQLValueFunction
type (with value of current_timestamp).
>
> Also the new node types being added to the param list is something
> other than Param. So it conflicts with the comment below in
> prepare_query_params()?
> /*
> * 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.)
> */
> If we are already adding non-params to this list, then the comment is
> outdated?
Fixed comment in the new version of the patches.
--
Best regards,
Alexander Pyhalov,
Postgres Professional
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2021-08-20 11:19 Ranier Vilela <[email protected]>
parent: Alexander Pyhalov <[email protected]>
2 siblings, 1 reply; 23+ messages in thread
From: Ranier Vilela @ 2021-08-20 11:19 UTC (permalink / raw)
To: Alexander Pyhalov <[email protected]>; +Cc: Zhihong Yu <[email protected]>; pgsql-hackers
Em sex., 20 de ago. de 2021 às 04:13, Alexander Pyhalov <
[email protected]> escreveu:
> Hi.
>
> Ranier Vilela писал 2021-08-19 14:01:
> > Em qui., 19 de ago. de 2021 às 07:50, Zhihong Yu <[email protected]>
> >> Hi,
> >> For 0001 patch:
> >>
> >> + if ((s->op != SVFOP_CURRENT_TIMESTAMP) &&
> >> + (s->op != SVFOP_CURRENT_TIMESTAMP_N) &&
> >> + (s->op != SVFOP_CURRENT_TIME) &&
> >> ...
> >>
> >> The above check appears more than once. If extracted into a helper
> >> method, it would help reduce duplicate and make the code more
> >> readable.
> >
> > 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().
>
Thanks.
Another question:
For 0002 patch:
+ if (node->funcid == F_NOW)
+ {
+ SQLValueFunction *svf = makeNode(SQLValueFunction);
+
+ svf->op = SVFOP_CURRENT_TIMESTAMP;
+ svf->type = TIMESTAMPTZOID;
+ svf->typmod = -1;
+ svf->location = -1;
+
+ deparseSQLValueFunction(svf, context);
+
+ return;
+ }
It seems to me that the svf->xpr field ( SQLValueFunction *svf ) is not
initialized somewhere even by deparseSQLValueFunction.
If it's not really used, it should be initialized to NULL, ok?
regards,
Ranier Vilela
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2021-08-20 12:18 Alexander Pyhalov <[email protected]>
parent: Ranier Vilela <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Alexander Pyhalov @ 2021-08-20 12:18 UTC (permalink / raw)
To: Ranier Vilela <[email protected]>; +Cc: Zhihong Yu <[email protected]>; pgsql-hackers
Ranier Vilela писал 2021-08-20 14:19:
> Another question:
> For 0002 patch:
>
> + if (node->funcid == F_NOW)
> + {
> + SQLValueFunction *svf = makeNode(SQLValueFunction);
> +
> + svf->op = SVFOP_CURRENT_TIMESTAMP;
> + svf->type = TIMESTAMPTZOID;
> + svf->typmod = -1;
> + svf->location = -1;
> +
> + deparseSQLValueFunction(svf, context);
> +
> + return;
> + }
> It seems to me that the svf->xpr field ( SQLValueFunction *svf ) is
> not initialized somewhere even by deparseSQLValueFunction.
> If it's not really used, it should be initialized to NULL, ok?
>
xpr field just carries node type, which will be initialized by
makeNode().
--
Best regards,
Alexander Pyhalov,
Postgres Professional
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2021-08-20 13:28 Zhihong Yu <[email protected]>
parent: Alexander Pyhalov <[email protected]>
2 siblings, 0 replies; 23+ messages in thread
From: Zhihong Yu @ 2021-08-20 13:28 UTC (permalink / raw)
To: Alexander Pyhalov <[email protected]>; +Cc: Ranier Vilela <[email protected]>; pgsql-hackers
On Fri, Aug 20, 2021 at 12:13 AM Alexander Pyhalov <[email protected]>
wrote:
> Hi.
>
> Ranier Vilela писал 2021-08-19 14:01:
> > Em qui., 19 de ago. de 2021 às 07:50, Zhihong Yu <[email protected]>
> >> Hi,
> >> For 0001 patch:
> >>
> >> + if ((s->op != SVFOP_CURRENT_TIMESTAMP) &&
> >> + (s->op != SVFOP_CURRENT_TIMESTAMP_N) &&
> >> + (s->op != SVFOP_CURRENT_TIME) &&
> >> ...
> >>
> >> The above check appears more than once. If extracted into a helper
> >> method, it would help reduce duplicate and make the code more
> >> readable.
> >
> > 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().
>
>
> --
> Best regards,
> Alexander Pyhalov,
> Postgres Professional
Hi,
The patches are good by me.
Thanks
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2021-08-20 13:59 Ranier Vilela <[email protected]>
parent: Alexander Pyhalov <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Ranier Vilela @ 2021-08-20 13:59 UTC (permalink / raw)
To: Alexander Pyhalov <[email protected]>; +Cc: Zhihong Yu <[email protected]>; pgsql-hackers
Em sex., 20 de ago. de 2021 às 09:18, Alexander Pyhalov <
[email protected]> escreveu:
> Ranier Vilela писал 2021-08-20 14:19:
>
> > Another question:
> > For 0002 patch:
> >
> > + if (node->funcid == F_NOW)
> > + {
> > + SQLValueFunction *svf = makeNode(SQLValueFunction);
> > +
> > + svf->op = SVFOP_CURRENT_TIMESTAMP;
> > + svf->type = TIMESTAMPTZOID;
> > + svf->typmod = -1;
> > + svf->location = -1;
> > +
> > + deparseSQLValueFunction(svf, context);
> > +
> > + return;
> > + }
> > It seems to me that the svf->xpr field ( SQLValueFunction *svf ) is
> > not initialized somewhere even by deparseSQLValueFunction.
> > If it's not really used, it should be initialized to NULL, ok?
> >
>
> xpr field just carries node type, which will be initialized by
> makeNode().
>
Great, I missed it.
regards,
Ranier Vilela
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2022-01-17 23:08 Tom Lane <[email protected]>
parent: Alexander Pyhalov <[email protected]>
2 siblings, 1 reply; 23+ messages in thread
From: Tom Lane @ 2022-01-17 23:08 UTC (permalink / raw)
To: Alexander Pyhalov <[email protected]>; +Cc: Ranier Vilela <[email protected]>; Zhihong Yu <[email protected]>; pgsql-hackers
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.)
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?
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'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.
regards, tom lane
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2022-01-18 08:52 Alexander Pyhalov <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Alexander Pyhalov @ 2022-01-18 08:52 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Ranier Vilela <[email protected]>; Zhihong Yu <[email protected]>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2022-01-18 16:53 Tom Lane <[email protected]>
parent: Alexander Pyhalov <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Tom Lane @ 2022-01-18 16:53 UTC (permalink / raw)
To: Alexander Pyhalov <[email protected]>; +Cc: Ranier Vilela <[email protected]>; Zhihong Yu <[email protected]>; pgsql-hackers
Alexander Pyhalov <[email protected]> writes:
> [ updated patch ]
Thanks for updating the patch. (BTW, please attach version numbers
to new patch versions, to avoid confusion.)
However, before we proceed any further with this patch, I think we
really ought to stop and think about the question I raised last
night: why are we building a one-off feature for SQLValueFunction?
Wouldn't the same parameter-substitution mechanism work for *any*
stable expression that doesn't contain remote Vars? That would
subsume the now() case as well as plenty of others.
So far the only counterexample I've been able to come up with is
that shipping values of reg* types might not be too safe, because
the remote side might not have the same objects. For example
consider these two potential quals:
WHERE remote_oid_column = CURRENT_ROLE::regrole
WHERE remote_text_column = CURRENT_ROLE::text
Say we're running as user 'joe' and that role doesn't exist on the
remote server. Then executing the first WHERE locally is fine, but
shipping it to the remote would cause a failure because the remote's
regrolein() will fail to convert the parameter value. But the second
case is quite non-problematic, because what will be sent over is just
some uninterpreted text.
In point of fact, this hazard doesn't have anything to do with stable
or not-stable subexpressions --- for example,
WHERE remote_oid_column = 'joe'::regrole
is just as unsafe, even though the value under consideration is a
*constant*. Maybe there is something in postgres_fdw that would stop
it from shipping this qual, but I don't recall seeing it, so I wonder
if there's a pre-existing bug here.
So it seems like we need a check to prevent generating remote Params
that are of "unsafe" types, but this is a type issue not an expression
issue --- as long as an expression is stable and does not yield an
unsafe-to-ship data type, why can't we treat it as a Param?
regards, tom lane
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2022-01-18 19:02 Alexander Pyhalov <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Alexander Pyhalov @ 2022-01-18 19:02 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Ranier Vilela <[email protected]>; Zhihong Yu <[email protected]>; pgsql-hackers
Tom Lane писал 2022-01-18 19:53:
> Alexander Pyhalov <[email protected]> writes:
>> [ updated patch ]
>
> Thanks for updating the patch. (BTW, please attach version numbers
> to new patch versions, to avoid confusion.)
>
> However, before we proceed any further with this patch, I think we
> really ought to stop and think about the question I raised last
> night: why are we building a one-off feature for SQLValueFunction?
> Wouldn't the same parameter-substitution mechanism work for *any*
> stable expression that doesn't contain remote Vars? That would
> subsume the now() case as well as plenty of others.
>
Hi.
I think, I can extend it to allow any stable function (not just
immutable/sqlvalues) in is_foreign_expr(), but not so sure about
"expression".
Perhaps, at top of deparseExpr() we can check that expression doesn't
contain vars, params, but contains stable function, and deparse it as
param.
This means we'll translate something like
explain select * from t where d > now() - '1 day'::interval;
to
select * from t where d > $1;
The question is how will we reliably determine its typmod (given that I
read in exprTypmod() comment
"returns the type-specific modifier of the expression's result type, if
it can be determined. In many cases, it can't".
What do we save if we don't ship whole expression as param, but only
stable functions? Allowing them seems to be more straightforward.
--
Best regards,
Alexander Pyhalov,
Postgres Professional
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2022-01-18 19:11 Tom Lane <[email protected]>
parent: Alexander Pyhalov <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Tom Lane @ 2022-01-18 19:11 UTC (permalink / raw)
To: Alexander Pyhalov <[email protected]>; +Cc: Ranier Vilela <[email protected]>; Zhihong Yu <[email protected]>; pgsql-hackers
Alexander Pyhalov <[email protected]> writes:
> Tom Lane писал 2022-01-18 19:53:
>> However, before we proceed any further with this patch, I think we
>> really ought to stop and think about the question I raised last
>> night: why are we building a one-off feature for SQLValueFunction?
>> Wouldn't the same parameter-substitution mechanism work for *any*
>> stable expression that doesn't contain remote Vars? That would
>> subsume the now() case as well as plenty of others.
> This means we'll translate something like
> explain select * from t where d > now() - '1 day'::interval;
> to
> select * from t where d > $1;
Right.
> The question is how will we reliably determine its typmod (given that I
> read in exprTypmod() comment
> "returns the type-specific modifier of the expression's result type, if
> it can be determined. In many cases, it can't".
I don't think we need to. If exprTypmod() says the typmod is -1,
then that's what it is.
> What do we save if we don't ship whole expression as param, but only
> stable functions? Allowing them seems to be more straightforward.
How so? Right off the bat, you get rid of the need for your own
version of contain_mutable_function. ISTM this approach can probably
be implemented in a patch that's noticeably smaller than what you
have now. It'll likely be touching entirely different places in
postgres_fdw, of course.
regards, tom lane
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2022-01-18 20:01 Tom Lane <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Tom Lane @ 2022-01-18 20:01 UTC (permalink / raw)
To: Alexander Pyhalov <[email protected]>; +Cc: Ranier Vilela <[email protected]>; Zhihong Yu <[email protected]>; pgsql-hackers
I wrote:
> Alexander Pyhalov <[email protected]> writes:
>> This means we'll translate something like
>> explain select * from t where d > now() - '1 day'::interval;
>> to
>> select * from t where d > $1;
> Right.
After thinking about that a bit more, I see that this will result
in a major redefinition of what is "shippable". Right now, we do not
consider the above WHERE clause to be shippable, not only because of
now() but because the timestamptz-minus-interval operator is dependent
on the timezone setting, which might be different at the remote.
But if we evaluate that operator locally and send its result as a
parameter, the objection vanishes. In fact, I don't think we even
need to require the subexpression to contain only built-in functions.
Its result still has to be of a built-in type, but that's a much
weaker restriction.
So this is going to require significant restructuring of both
is_foreign_expr and deparse.c, which I realize may be more than
you bargained for. But if you want to tackle it, I think what
we want to do is divide potentially-shippable expressions into
three sorts of components:
1. Remote Vars, which obviously ship as-is.
2. Locally-evaluatable subexpressions, which must contain no
remote Vars, must be stable or immutable, and must have a result
type that we consider safe to ship. If such a subexpression
is just a Const, we ship it as a constant, otherwise we evaluate
the value at runtime and ship a parameter.
3. Superstructure, which consists of all expression nodes having
at least one remote Var below them. These nodes have to be
shippable according to the existing definition, so that we know
that the remote server will evaluate them just as we would.
If we keep the existing division of labor between is_foreign_expr
and deparse.c, I fear that there's going to be a lot of duplicated
code as well as duplicative planning effort. I wonder if it'd be
wise to combine those operations into a single expression-scanning
process that determines whether an expression is safe to ship and
produces the translated string immediately if it is.
regards, tom lane
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2022-01-20 13:42 Alexander Pyhalov <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Alexander Pyhalov @ 2022-01-20 13:42 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Ranier Vilela <[email protected]>; Zhihong Yu <[email protected]>; pgsql-hackers
Tom Lane писал 2022-01-18 23:01:
> I wrote:
>> Alexander Pyhalov <[email protected]> writes:
>>> This means we'll translate something like
>>> explain select * from t where d > now() - '1 day'::interval;
>>> to
>>> select * from t where d > $1;
>
>> Right.
>
> After thinking about that a bit more, I see that this will result
> in a major redefinition of what is "shippable". Right now, we do not
> consider the above WHERE clause to be shippable, not only because of
> now() but because the timestamptz-minus-interval operator is dependent
> on the timezone setting, which might be different at the remote.
> But if we evaluate that operator locally and send its result as a
> parameter, the objection vanishes. In fact, I don't think we even
> need to require the subexpression to contain only built-in functions.
> Its result still has to be of a built-in type, but that's a much
> weaker restriction.
>
Hi.
So far I have the following prototype. It seems to be working, but I
think it can be enhanced.
At least, some sort of caching seems to be necessary for
is_stable_expr().
1) Now expression can be either 'stable shippable' or 'shippable
according to old rules'. We check if it's 'stable shippable' in
foreign_expr_walker(),
is_foreign_param() and deparseExpr(). All such exprs are replaced by
params while deparsing.
2) contain_mutable_functions() now is calculated only for current node,
if node is not considered 'stable shippable'.
Is it step in the right direction or do I miss something?
--
Best regards,
Alexander Pyhalov,
Postgres Professional
Attachments:
[text/x-diff] v04-0001-Push-down-stable-expressions.patch (50.1K, ../../[email protected]/2-v04-0001-Push-down-stable-expressions.patch)
download | inline diff:
From 60e6e0bf98326cb557c70a365797026e9925b7a3 Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov <[email protected]>
Date: Thu, 29 Jul 2021 11:45:28 +0300
Subject: [PATCH] Push down stable expressions
Stable expressions can be computed locally and sent to remote side as parameters values.
---
contrib/postgres_fdw/deparse.c | 1162 +++++++++--------
.../postgres_fdw/expected/postgres_fdw.out | 87 ++
contrib/postgres_fdw/postgres_fdw.c | 9 +-
contrib/postgres_fdw/postgres_fdw.h | 1 +
contrib/postgres_fdw/shippable.c | 74 ++
contrib/postgres_fdw/sql/postgres_fdw.sql | 22 +
6 files changed, 811 insertions(+), 544 deletions(-)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index bf12eac0288..33a79026574 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -158,6 +158,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 deparseStableExpr(Expr *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);
@@ -267,20 +268,17 @@ is_foreign_expr(PlannerInfo *root,
if (loc_cxt.state == FDW_COLLATE_UNSAFE)
return false;
- /*
- * An expression which includes any mutable functions can't be sent over
- * because its result is not stable. For example, sending now() remote
- * side could cause confusion from clock offsets. Future versions might
- * 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))
- return false;
/* OK to evaluate on the remote server */
return true;
}
+static bool
+contain_mutable_functions_checker(Oid func_id, void *context)
+{
+ return (func_volatile(func_id) != PROVOLATILE_IMMUTABLE);
+}
+
/*
* Check if expression is safe to execute remotely, and return true if so.
*
@@ -321,616 +319,650 @@ foreign_expr_walker(Node *node,
inner_cxt.collation = InvalidOid;
inner_cxt.state = FDW_COLLATE_NONE;
- switch (nodeTag(node))
+ if (is_stable_expr(node))
+ {
+ collation = exprCollation(node);
+ if (collation == InvalidOid || collation == DEFAULT_COLLATION_OID)
+ state = FDW_COLLATE_NONE;
+ else
+ state = FDW_COLLATE_UNSAFE;
+ }
+ else
{
- case T_Var:
- {
- Var *var = (Var *) node;
- /*
- * If the Var is from the foreign table, we consider its
- * collation (if any) safe to use. If it is from another
- * table, we treat its collation the same way as we would a
- * Param's collation, ie it's not safe for it to have a
- * non-default collation.
- */
- if (bms_is_member(var->varno, glob_cxt->relids) &&
- var->varlevelsup == 0)
+ /*
+ * An expression which includes any mutable functions can't be sent
+ * over because its result is not stable.
+ */
+ if (check_functions_in_node(node, contain_mutable_functions_checker, NULL))
+ return false;
+
+ switch (nodeTag(node))
+ {
+ case T_Var:
{
- /* Var belongs to foreign table */
+ Var *var = (Var *) node;
/*
- * System columns other than ctid should not be sent to
- * the remote, since we don't make any effort to ensure
- * that local and remote values match (tableoid, in
- * particular, almost certainly doesn't match).
+ * If the Var is from the foreign table, we consider its
+ * collation (if any) safe to use. If it is from another
+ * table, we treat its collation the same way as we would
+ * a Param's collation, ie it's not safe for it to have a
+ * non-default collation.
*/
- if (var->varattno < 0 &&
- var->varattno != SelfItemPointerAttributeNumber)
- return false;
-
- /* Else check the collation */
- collation = var->varcollid;
- state = OidIsValid(collation) ? FDW_COLLATE_SAFE : FDW_COLLATE_NONE;
- }
- else
- {
- /* Var belongs to some other table */
- collation = var->varcollid;
- if (collation == InvalidOid ||
- collation == DEFAULT_COLLATION_OID)
+ if (bms_is_member(var->varno, glob_cxt->relids) &&
+ var->varlevelsup == 0)
{
+ /* Var belongs to foreign table */
+
/*
- * It's noncollatable, or it's safe to combine with a
- * collatable foreign Var, so set state to NONE.
+ * System columns other than ctid should not be sent
+ * to the remote, since we don't make any effort to
+ * ensure that local and remote values match
+ * (tableoid, in particular, almost certainly doesn't
+ * match).
*/
- state = FDW_COLLATE_NONE;
+ if (var->varattno < 0 &&
+ var->varattno != SelfItemPointerAttributeNumber)
+ return false;
+
+ /* Else check the collation */
+ collation = var->varcollid;
+ state = OidIsValid(collation) ? FDW_COLLATE_SAFE : FDW_COLLATE_NONE;
}
else
{
- /*
- * Do not fail right away, since the Var might appear
- * in a collation-insensitive context.
- */
- state = FDW_COLLATE_UNSAFE;
+ /* Var belongs to some other table */
+ collation = var->varcollid;
+ if (collation == InvalidOid ||
+ collation == DEFAULT_COLLATION_OID)
+ {
+ /*
+ * It's noncollatable, or it's safe to combine
+ * with a collatable foreign Var, so set state to
+ * NONE.
+ */
+ state = FDW_COLLATE_NONE;
+ }
+ else
+ {
+ /*
+ * Do not fail right away, since the Var might
+ * appear in a collation-insensitive context.
+ */
+ state = FDW_COLLATE_UNSAFE;
+ }
}
}
- }
- break;
- case T_Const:
- {
- Const *c = (Const *) node;
+ break;
+ case T_Const:
+ {
+ Const *c = (Const *) node;
- /*
- * If the constant has nondefault collation, either it's of a
- * non-builtin type, or it reflects folding of a CollateExpr.
- * It's unsafe to send to the remote unless it's used in a
- * non-collation-sensitive context.
- */
- collation = c->constcollid;
- if (collation == InvalidOid ||
- collation == DEFAULT_COLLATION_OID)
- state = FDW_COLLATE_NONE;
- else
- state = FDW_COLLATE_UNSAFE;
- }
- break;
- case T_Param:
- {
- Param *p = (Param *) node;
+ /*
+ * If the constant has nondefault collation, either it's
+ * of a non-builtin type, or it reflects folding of a
+ * CollateExpr. It's unsafe to send to the remote unless
+ * it's used in a non-collation-sensitive context.
+ */
+ collation = c->constcollid;
+ if (collation == InvalidOid ||
+ collation == DEFAULT_COLLATION_OID)
+ state = FDW_COLLATE_NONE;
+ else
+ state = FDW_COLLATE_UNSAFE;
+ }
+ break;
+ case T_Param:
+ {
+ Param *p = (Param *) node;
- /*
- * If it's a MULTIEXPR Param, punt. We can't tell from here
- * whether the referenced sublink/subplan contains any remote
- * Vars; if it does, handling that is too complicated to
- * consider supporting at present. Fortunately, MULTIEXPR
- * Params are not reduced to plain PARAM_EXEC until the end of
- * planning, so we can easily detect this case. (Normal
- * PARAM_EXEC Params are safe to ship because their values
- * come from somewhere else in the plan tree; but a MULTIEXPR
- * references a sub-select elsewhere in the same targetlist,
- * so we'd be on the hook to evaluate it somehow if we wanted
- * to handle such cases as direct foreign updates.)
- */
- if (p->paramkind == PARAM_MULTIEXPR)
- return false;
+ /*
+ * If it's a MULTIEXPR Param, punt. We can't tell from
+ * here whether the referenced sublink/subplan contains
+ * any remote Vars; if it does, handling that is too
+ * complicated to consider supporting at present.
+ * Fortunately, MULTIEXPR Params are not reduced to plain
+ * PARAM_EXEC until the end of planning, so we can easily
+ * detect this case. (Normal PARAM_EXEC Params are safe
+ * to ship because their values come from somewhere else
+ * in the plan tree; but a MULTIEXPR references a
+ * sub-select elsewhere in the same targetlist, so we'd be
+ * on the hook to evaluate it somehow if we wanted to
+ * handle such cases as direct foreign updates.)
+ */
+ if (p->paramkind == PARAM_MULTIEXPR)
+ return false;
- /*
- * Collation rule is same as for Consts and non-foreign Vars.
- */
- collation = p->paramcollid;
- if (collation == InvalidOid ||
- collation == DEFAULT_COLLATION_OID)
- state = FDW_COLLATE_NONE;
- else
- state = FDW_COLLATE_UNSAFE;
- }
- break;
- case T_SubscriptingRef:
- {
- SubscriptingRef *sr = (SubscriptingRef *) node;
+ /*
+ * Collation rule is same as for Consts and non-foreign
+ * Vars.
+ */
+ collation = p->paramcollid;
+ if (collation == InvalidOid ||
+ collation == DEFAULT_COLLATION_OID)
+ state = FDW_COLLATE_NONE;
+ else
+ state = FDW_COLLATE_UNSAFE;
+ }
+ break;
+ case T_SubscriptingRef:
+ {
+ SubscriptingRef *sr = (SubscriptingRef *) node;
- /* Assignment should not be in restrictions. */
- if (sr->refassgnexpr != NULL)
- return false;
+ /* Assignment should not be in restrictions. */
+ if (sr->refassgnexpr != NULL)
+ return false;
- /*
- * Recurse into the remaining subexpressions. The container
- * subscripts will not affect collation of the SubscriptingRef
- * result, so do those first and reset inner_cxt afterwards.
- */
- if (!foreign_expr_walker((Node *) sr->refupperindexpr,
- glob_cxt, &inner_cxt, case_arg_cxt))
- return false;
- inner_cxt.collation = InvalidOid;
- inner_cxt.state = FDW_COLLATE_NONE;
- if (!foreign_expr_walker((Node *) sr->reflowerindexpr,
- glob_cxt, &inner_cxt, case_arg_cxt))
- return false;
- inner_cxt.collation = InvalidOid;
- inner_cxt.state = FDW_COLLATE_NONE;
- if (!foreign_expr_walker((Node *) sr->refexpr,
- glob_cxt, &inner_cxt, case_arg_cxt))
- return false;
+ /*
+ * Recurse into the remaining subexpressions. The
+ * container subscripts will not affect collation of the
+ * SubscriptingRef result, so do those first and reset
+ * inner_cxt afterwards.
+ */
+ if (!foreign_expr_walker((Node *) sr->refupperindexpr,
+ glob_cxt, &inner_cxt, case_arg_cxt))
+ return false;
+ inner_cxt.collation = InvalidOid;
+ inner_cxt.state = FDW_COLLATE_NONE;
+ if (!foreign_expr_walker((Node *) sr->reflowerindexpr,
+ glob_cxt, &inner_cxt, case_arg_cxt))
+ return false;
+ inner_cxt.collation = InvalidOid;
+ inner_cxt.state = FDW_COLLATE_NONE;
+ if (!foreign_expr_walker((Node *) sr->refexpr,
+ glob_cxt, &inner_cxt, case_arg_cxt))
+ return false;
- /*
- * Container subscripting typically yields same collation as
- * refexpr's, but in case it doesn't, use same logic as for
- * function nodes.
- */
- collation = sr->refcollid;
- if (collation == InvalidOid)
- state = FDW_COLLATE_NONE;
- else if (inner_cxt.state == FDW_COLLATE_SAFE &&
- collation == inner_cxt.collation)
- state = FDW_COLLATE_SAFE;
- else if (collation == DEFAULT_COLLATION_OID)
- state = FDW_COLLATE_NONE;
- else
- state = FDW_COLLATE_UNSAFE;
- }
- break;
- case T_FuncExpr:
- {
- FuncExpr *fe = (FuncExpr *) node;
+ /*
+ * Container subscripting typically yields same collation
+ * as refexpr's, but in case it doesn't, use same logic as
+ * for function nodes.
+ */
+ collation = sr->refcollid;
+ if (collation == InvalidOid)
+ state = FDW_COLLATE_NONE;
+ else if (inner_cxt.state == FDW_COLLATE_SAFE &&
+ collation == inner_cxt.collation)
+ state = FDW_COLLATE_SAFE;
+ else if (collation == DEFAULT_COLLATION_OID)
+ state = FDW_COLLATE_NONE;
+ else
+ state = FDW_COLLATE_UNSAFE;
+ }
+ break;
+ case T_FuncExpr:
+ {
+ FuncExpr *fe = (FuncExpr *) node;
- /*
- * If function used by the expression is not shippable, it
- * can't be sent to remote because it might have incompatible
- * semantics on remote side.
- */
- if (!is_shippable(fe->funcid, ProcedureRelationId, fpinfo))
- return false;
+ /*
+ * If function used by the expression is not shippable, it
+ * can't be sent to remote because it might have
+ * incompatible semantics on remote side.
+ */
+ if (!is_shippable(fe->funcid, ProcedureRelationId, fpinfo))
+ return false;
- /*
- * Recurse to input subexpressions.
- */
- if (!foreign_expr_walker((Node *) fe->args,
- glob_cxt, &inner_cxt, case_arg_cxt))
- return false;
+ /*
+ * Recurse to input subexpressions.
+ */
+ if (!foreign_expr_walker((Node *) fe->args,
+ glob_cxt, &inner_cxt, case_arg_cxt))
+ return false;
- /*
- * If function's input collation is not derived from a foreign
- * Var, it can't be sent to remote.
- */
- if (fe->inputcollid == InvalidOid)
- /* OK, inputs are all noncollatable */ ;
- else if (inner_cxt.state != FDW_COLLATE_SAFE ||
- fe->inputcollid != inner_cxt.collation)
- return false;
+ /*
+ * If function's input collation is not derived from a
+ * foreign Var, it can't be sent to remote.
+ */
+ if (fe->inputcollid == InvalidOid)
+ /* OK, inputs are all noncollatable */ ;
+ else if (inner_cxt.state != FDW_COLLATE_SAFE ||
+ fe->inputcollid != inner_cxt.collation)
+ return false;
- /*
- * Detect whether node is introducing a collation not derived
- * from a foreign Var. (If so, we just mark it unsafe for now
- * rather than immediately returning false, since the parent
- * node might not care.)
- */
- collation = fe->funccollid;
- if (collation == InvalidOid)
- state = FDW_COLLATE_NONE;
- else if (inner_cxt.state == FDW_COLLATE_SAFE &&
- collation == inner_cxt.collation)
- state = FDW_COLLATE_SAFE;
- else if (collation == DEFAULT_COLLATION_OID)
- state = FDW_COLLATE_NONE;
- else
- state = FDW_COLLATE_UNSAFE;
- }
- break;
- case T_OpExpr:
- case T_DistinctExpr: /* struct-equivalent to OpExpr */
- {
- OpExpr *oe = (OpExpr *) node;
+ /*
+ * Detect whether node is introducing a collation not
+ * derived from a foreign Var. (If so, we just mark it
+ * unsafe for now rather than immediately returning false,
+ * since the parent node might not care.)
+ */
+ collation = fe->funccollid;
+ if (collation == InvalidOid)
+ state = FDW_COLLATE_NONE;
+ else if (inner_cxt.state == FDW_COLLATE_SAFE &&
+ collation == inner_cxt.collation)
+ state = FDW_COLLATE_SAFE;
+ else if (collation == DEFAULT_COLLATION_OID)
+ state = FDW_COLLATE_NONE;
+ else
+ state = FDW_COLLATE_UNSAFE;
+ }
+ break;
+ case T_OpExpr:
+ case T_DistinctExpr: /* struct-equivalent to OpExpr */
+ {
+ OpExpr *oe = (OpExpr *) node;
- /*
- * Similarly, only shippable operators can be sent to remote.
- * (If the operator is shippable, we assume its underlying
- * function is too.)
- */
- if (!is_shippable(oe->opno, OperatorRelationId, fpinfo))
- return false;
+ /*
+ * Similarly, only shippable operators can be sent to
+ * remote. (If the operator is shippable, we assume its
+ * underlying function is too.)
+ */
+ if (!is_shippable(oe->opno, OperatorRelationId, fpinfo))
+ return false;
- /*
- * Recurse to input subexpressions.
- */
- if (!foreign_expr_walker((Node *) oe->args,
- glob_cxt, &inner_cxt, case_arg_cxt))
- return false;
+ /*
+ * Recurse to input subexpressions.
+ */
+ if (!foreign_expr_walker((Node *) oe->args,
+ glob_cxt, &inner_cxt, case_arg_cxt))
+ return false;
- /*
- * If operator's input collation is not derived from a foreign
- * Var, it can't be sent to remote.
- */
- if (oe->inputcollid == InvalidOid)
- /* OK, inputs are all noncollatable */ ;
- else if (inner_cxt.state != FDW_COLLATE_SAFE ||
- oe->inputcollid != inner_cxt.collation)
- return false;
-
- /* Result-collation handling is same as for functions */
- collation = oe->opcollid;
- if (collation == InvalidOid)
- state = FDW_COLLATE_NONE;
- else if (inner_cxt.state == FDW_COLLATE_SAFE &&
- collation == inner_cxt.collation)
- state = FDW_COLLATE_SAFE;
- else if (collation == DEFAULT_COLLATION_OID)
- state = FDW_COLLATE_NONE;
- else
- state = FDW_COLLATE_UNSAFE;
- }
- break;
- case T_ScalarArrayOpExpr:
- {
- ScalarArrayOpExpr *oe = (ScalarArrayOpExpr *) node;
+ /*
+ * If operator's input collation is not derived from a
+ * foreign Var, it can't be sent to remote.
+ */
+ if (oe->inputcollid == InvalidOid)
+ /* OK, inputs are all noncollatable */ ;
+ else if (inner_cxt.state != FDW_COLLATE_SAFE ||
+ oe->inputcollid != inner_cxt.collation)
+ return false;
- /*
- * Again, only shippable operators can be sent to remote.
- */
- if (!is_shippable(oe->opno, OperatorRelationId, fpinfo))
- return false;
+ /* Result-collation handling is same as for functions */
+ collation = oe->opcollid;
+ if (collation == InvalidOid)
+ state = FDW_COLLATE_NONE;
+ else if (inner_cxt.state == FDW_COLLATE_SAFE &&
+ collation == inner_cxt.collation)
+ state = FDW_COLLATE_SAFE;
+ else if (collation == DEFAULT_COLLATION_OID)
+ state = FDW_COLLATE_NONE;
+ else
+ state = FDW_COLLATE_UNSAFE;
+ }
+ break;
+ case T_ScalarArrayOpExpr:
+ {
+ ScalarArrayOpExpr *oe = (ScalarArrayOpExpr *) node;
- /*
- * Recurse to input subexpressions.
- */
- if (!foreign_expr_walker((Node *) oe->args,
- glob_cxt, &inner_cxt, case_arg_cxt))
- return false;
+ /*
+ * Again, only shippable operators can be sent to remote.
+ */
+ if (!is_shippable(oe->opno, OperatorRelationId, fpinfo))
+ return false;
- /*
- * If operator's input collation is not derived from a foreign
- * Var, it can't be sent to remote.
- */
- if (oe->inputcollid == InvalidOid)
- /* OK, inputs are all noncollatable */ ;
- else if (inner_cxt.state != FDW_COLLATE_SAFE ||
- oe->inputcollid != inner_cxt.collation)
- return false;
-
- /* Output is always boolean and so noncollatable. */
- collation = InvalidOid;
- state = FDW_COLLATE_NONE;
- }
- break;
- case T_RelabelType:
- {
- RelabelType *r = (RelabelType *) node;
+ /*
+ * Recurse to input subexpressions.
+ */
+ if (!foreign_expr_walker((Node *) oe->args,
+ glob_cxt, &inner_cxt, case_arg_cxt))
+ return false;
- /*
- * Recurse to input subexpression.
- */
- if (!foreign_expr_walker((Node *) r->arg,
- glob_cxt, &inner_cxt, case_arg_cxt))
- return false;
+ /*
+ * If operator's input collation is not derived from a
+ * foreign Var, it can't be sent to remote.
+ */
+ if (oe->inputcollid == InvalidOid)
+ /* OK, inputs are all noncollatable */ ;
+ else if (inner_cxt.state != FDW_COLLATE_SAFE ||
+ oe->inputcollid != inner_cxt.collation)
+ return false;
- /*
- * RelabelType must not introduce a collation not derived from
- * an input foreign Var (same logic as for a real function).
- */
- collation = r->resultcollid;
- if (collation == InvalidOid)
- state = FDW_COLLATE_NONE;
- else if (inner_cxt.state == FDW_COLLATE_SAFE &&
- collation == inner_cxt.collation)
- state = FDW_COLLATE_SAFE;
- else if (collation == DEFAULT_COLLATION_OID)
+ /* Output is always boolean and so noncollatable. */
+ collation = InvalidOid;
state = FDW_COLLATE_NONE;
- else
- state = FDW_COLLATE_UNSAFE;
- }
- break;
- case T_BoolExpr:
- {
- BoolExpr *b = (BoolExpr *) node;
-
- /*
- * Recurse to input subexpressions.
- */
- if (!foreign_expr_walker((Node *) b->args,
- glob_cxt, &inner_cxt, case_arg_cxt))
- return false;
+ }
+ break;
+ case T_RelabelType:
+ {
+ RelabelType *r = (RelabelType *) node;
- /* Output is always boolean and so noncollatable. */
- collation = InvalidOid;
- state = FDW_COLLATE_NONE;
- }
- break;
- case T_NullTest:
- {
- NullTest *nt = (NullTest *) node;
+ /*
+ * Recurse to input subexpression.
+ */
+ if (!foreign_expr_walker((Node *) r->arg,
+ glob_cxt, &inner_cxt, case_arg_cxt))
+ return false;
- /*
- * Recurse to input subexpressions.
- */
- if (!foreign_expr_walker((Node *) nt->arg,
- glob_cxt, &inner_cxt, case_arg_cxt))
- return false;
+ /*
+ * RelabelType must not introduce a collation not derived
+ * from an input foreign Var (same logic as for a real
+ * function).
+ */
+ collation = r->resultcollid;
+ if (collation == InvalidOid)
+ state = FDW_COLLATE_NONE;
+ else if (inner_cxt.state == FDW_COLLATE_SAFE &&
+ collation == inner_cxt.collation)
+ state = FDW_COLLATE_SAFE;
+ else if (collation == DEFAULT_COLLATION_OID)
+ state = FDW_COLLATE_NONE;
+ else
+ state = FDW_COLLATE_UNSAFE;
+ }
+ break;
+ case T_BoolExpr:
+ {
+ BoolExpr *b = (BoolExpr *) node;
- /* Output is always boolean and so noncollatable. */
- collation = InvalidOid;
- state = FDW_COLLATE_NONE;
- }
- break;
- case T_CaseExpr:
- {
- CaseExpr *ce = (CaseExpr *) node;
- foreign_loc_cxt arg_cxt;
- foreign_loc_cxt tmp_cxt;
- ListCell *lc;
+ /*
+ * Recurse to input subexpressions.
+ */
+ if (!foreign_expr_walker((Node *) b->args,
+ glob_cxt, &inner_cxt, case_arg_cxt))
+ return false;
- /*
- * Recurse to CASE's arg expression, if any. Its collation
- * has to be saved aside for use while examining CaseTestExprs
- * within the WHEN expressions.
- */
- arg_cxt.collation = InvalidOid;
- arg_cxt.state = FDW_COLLATE_NONE;
- if (ce->arg)
+ /* Output is always boolean and so noncollatable. */
+ collation = InvalidOid;
+ state = FDW_COLLATE_NONE;
+ }
+ break;
+ case T_NullTest:
{
- if (!foreign_expr_walker((Node *) ce->arg,
- glob_cxt, &arg_cxt, case_arg_cxt))
+ NullTest *nt = (NullTest *) node;
+
+ /*
+ * Recurse to input subexpressions.
+ */
+ if (!foreign_expr_walker((Node *) nt->arg,
+ glob_cxt, &inner_cxt, case_arg_cxt))
return false;
- }
- /* Examine the CaseWhen subexpressions. */
- foreach(lc, ce->args)
+ /* Output is always boolean and so noncollatable. */
+ collation = InvalidOid;
+ state = FDW_COLLATE_NONE;
+ }
+ break;
+ case T_CaseExpr:
{
- CaseWhen *cw = lfirst_node(CaseWhen, lc);
+ CaseExpr *ce = (CaseExpr *) node;
+ foreign_loc_cxt arg_cxt;
+ foreign_loc_cxt tmp_cxt;
+ ListCell *lc;
+ /*
+ * Recurse to CASE's arg expression, if any. Its
+ * collation has to be saved aside for use while examining
+ * CaseTestExprs within the WHEN expressions.
+ */
+ arg_cxt.collation = InvalidOid;
+ arg_cxt.state = FDW_COLLATE_NONE;
if (ce->arg)
{
+ if (!foreign_expr_walker((Node *) ce->arg,
+ glob_cxt, &arg_cxt, case_arg_cxt))
+ return false;
+ }
+
+ /* Examine the CaseWhen subexpressions. */
+ foreach(lc, ce->args)
+ {
+ CaseWhen *cw = lfirst_node(CaseWhen, lc);
+
+ if (ce->arg)
+ {
+ /*
+ * In a CASE-with-arg, the parser should have
+ * produced WHEN clauses of the form "CaseTestExpr
+ * = RHS", possibly with an implicit coercion
+ * inserted above the CaseTestExpr. However in an
+ * expression that's been through the optimizer,
+ * the WHEN clause could be almost anything (since
+ * the equality operator could have been expanded
+ * into an inline function). In such cases forbid
+ * pushdown, because deparseCaseExpr can't handle
+ * it.
+ */
+ Node *whenExpr = (Node *) cw->expr;
+ List *opArgs;
+
+ if (!IsA(whenExpr, OpExpr))
+ return false;
+
+ opArgs = ((OpExpr *) whenExpr)->args;
+ if (list_length(opArgs) != 2 ||
+ !IsA(strip_implicit_coercions(linitial(opArgs)),
+ CaseTestExpr))
+ return false;
+ }
+
/*
- * In a CASE-with-arg, the parser should have produced
- * WHEN clauses of the form "CaseTestExpr = RHS",
- * possibly with an implicit coercion inserted above
- * the CaseTestExpr. However in an expression that's
- * been through the optimizer, the WHEN clause could
- * be almost anything (since the equality operator
- * could have been expanded into an inline function).
- * In such cases forbid pushdown, because
- * deparseCaseExpr can't handle it.
+ * Recurse to WHEN expression, passing down the arg
+ * info. Its collation doesn't affect the result
+ * (really, it should be boolean and thus not have a
+ * collation).
*/
- Node *whenExpr = (Node *) cw->expr;
- List *opArgs;
-
- if (!IsA(whenExpr, OpExpr))
+ tmp_cxt.collation = InvalidOid;
+ tmp_cxt.state = FDW_COLLATE_NONE;
+ if (!foreign_expr_walker((Node *) cw->expr,
+ glob_cxt, &tmp_cxt, &arg_cxt))
return false;
- opArgs = ((OpExpr *) whenExpr)->args;
- if (list_length(opArgs) != 2 ||
- !IsA(strip_implicit_coercions(linitial(opArgs)),
- CaseTestExpr))
+ /* Recurse to THEN expression. */
+ if (!foreign_expr_walker((Node *) cw->result,
+ glob_cxt, &inner_cxt, case_arg_cxt))
return false;
}
- /*
- * Recurse to WHEN expression, passing down the arg info.
- * Its collation doesn't affect the result (really, it
- * should be boolean and thus not have a collation).
- */
- tmp_cxt.collation = InvalidOid;
- tmp_cxt.state = FDW_COLLATE_NONE;
- if (!foreign_expr_walker((Node *) cw->expr,
- glob_cxt, &tmp_cxt, &arg_cxt))
- return false;
-
- /* Recurse to THEN expression. */
- if (!foreign_expr_walker((Node *) cw->result,
+ /* Recurse to ELSE expression. */
+ if (!foreign_expr_walker((Node *) ce->defresult,
glob_cxt, &inner_cxt, case_arg_cxt))
return false;
+
+ /*
+ * Detect whether node is introducing a collation not
+ * derived from a foreign Var. (If so, we just mark it
+ * unsafe for now rather than immediately returning false,
+ * since the parent node might not care.) This is the
+ * same as for function nodes, except that the input
+ * collation is derived from only the THEN and ELSE
+ * subexpressions.
+ */
+ collation = ce->casecollid;
+ if (collation == InvalidOid)
+ state = FDW_COLLATE_NONE;
+ else if (inner_cxt.state == FDW_COLLATE_SAFE &&
+ collation == inner_cxt.collation)
+ state = FDW_COLLATE_SAFE;
+ else if (collation == DEFAULT_COLLATION_OID)
+ state = FDW_COLLATE_NONE;
+ else
+ state = FDW_COLLATE_UNSAFE;
}
+ break;
+ case T_CaseTestExpr:
+ {
+ CaseTestExpr *c = (CaseTestExpr *) node;
- /* Recurse to ELSE expression. */
- if (!foreign_expr_walker((Node *) ce->defresult,
- glob_cxt, &inner_cxt, case_arg_cxt))
- return false;
+ /* Punt if we seem not to be inside a CASE arg WHEN. */
+ if (!case_arg_cxt)
+ return false;
- /*
- * Detect whether node is introducing a collation not derived
- * from a foreign Var. (If so, we just mark it unsafe for now
- * rather than immediately returning false, since the parent
- * node might not care.) This is the same as for function
- * nodes, except that the input collation is derived from only
- * the THEN and ELSE subexpressions.
- */
- collation = ce->casecollid;
- if (collation == InvalidOid)
- state = FDW_COLLATE_NONE;
- else if (inner_cxt.state == FDW_COLLATE_SAFE &&
- collation == inner_cxt.collation)
- state = FDW_COLLATE_SAFE;
- else if (collation == DEFAULT_COLLATION_OID)
- state = FDW_COLLATE_NONE;
- else
- state = FDW_COLLATE_UNSAFE;
- }
- break;
- case T_CaseTestExpr:
- {
- CaseTestExpr *c = (CaseTestExpr *) node;
+ /*
+ * Otherwise, any nondefault collation attached to the
+ * CaseTestExpr node must be derived from foreign Var(s)
+ * in the CASE arg.
+ */
+ collation = c->collation;
+ if (collation == InvalidOid)
+ state = FDW_COLLATE_NONE;
+ else if (case_arg_cxt->state == FDW_COLLATE_SAFE &&
+ collation == case_arg_cxt->collation)
+ state = FDW_COLLATE_SAFE;
+ else if (collation == DEFAULT_COLLATION_OID)
+ state = FDW_COLLATE_NONE;
+ else
+ state = FDW_COLLATE_UNSAFE;
+ }
+ break;
+ case T_ArrayExpr:
+ {
+ ArrayExpr *a = (ArrayExpr *) node;
- /* Punt if we seem not to be inside a CASE arg WHEN. */
- if (!case_arg_cxt)
- return false;
+ /*
+ * Recurse to input subexpressions.
+ */
+ if (!foreign_expr_walker((Node *) a->elements,
+ glob_cxt, &inner_cxt, case_arg_cxt))
+ return false;
- /*
- * Otherwise, any nondefault collation attached to the
- * CaseTestExpr node must be derived from foreign Var(s) in
- * the CASE arg.
- */
- collation = c->collation;
- if (collation == InvalidOid)
- state = FDW_COLLATE_NONE;
- else if (case_arg_cxt->state == FDW_COLLATE_SAFE &&
- collation == case_arg_cxt->collation)
- state = FDW_COLLATE_SAFE;
- else if (collation == DEFAULT_COLLATION_OID)
- state = FDW_COLLATE_NONE;
- else
- state = FDW_COLLATE_UNSAFE;
- }
- break;
- case T_ArrayExpr:
- {
- ArrayExpr *a = (ArrayExpr *) node;
+ /*
+ * ArrayExpr must not introduce a collation not derived
+ * from an input foreign Var (same logic as for a
+ * function).
+ */
+ collation = a->array_collid;
+ if (collation == InvalidOid)
+ state = FDW_COLLATE_NONE;
+ else if (inner_cxt.state == FDW_COLLATE_SAFE &&
+ collation == inner_cxt.collation)
+ state = FDW_COLLATE_SAFE;
+ else if (collation == DEFAULT_COLLATION_OID)
+ state = FDW_COLLATE_NONE;
+ else
+ state = FDW_COLLATE_UNSAFE;
+ }
+ break;
+ case T_List:
+ {
+ List *l = (List *) node;
+ ListCell *lc;
- /*
- * Recurse to input subexpressions.
- */
- if (!foreign_expr_walker((Node *) a->elements,
- glob_cxt, &inner_cxt, case_arg_cxt))
- return false;
+ /*
+ * Recurse to component subexpressions.
+ */
+ foreach(lc, l)
+ {
+ if (!foreign_expr_walker((Node *) lfirst(lc),
+ glob_cxt, &inner_cxt, case_arg_cxt))
+ return false;
+ }
- /*
- * ArrayExpr must not introduce a collation not derived from
- * an input foreign Var (same logic as for a function).
- */
- collation = a->array_collid;
- if (collation == InvalidOid)
- state = FDW_COLLATE_NONE;
- else if (inner_cxt.state == FDW_COLLATE_SAFE &&
- collation == inner_cxt.collation)
- state = FDW_COLLATE_SAFE;
- else if (collation == DEFAULT_COLLATION_OID)
- state = FDW_COLLATE_NONE;
- else
- state = FDW_COLLATE_UNSAFE;
- }
- break;
- case T_List:
- {
- List *l = (List *) node;
- ListCell *lc;
+ /*
+ * When processing a list, collation state just bubbles up
+ * from the list elements.
+ */
+ collation = inner_cxt.collation;
+ state = inner_cxt.state;
- /*
- * Recurse to component subexpressions.
- */
- foreach(lc, l)
+ /* Don't apply exprType() to the list. */
+ check_type = false;
+ }
+ break;
+ case T_Aggref:
{
- if (!foreign_expr_walker((Node *) lfirst(lc),
- glob_cxt, &inner_cxt, case_arg_cxt))
+ Aggref *agg = (Aggref *) node;
+ ListCell *lc;
+
+ /* Not safe to pushdown when not in grouping context */
+ if (!IS_UPPER_REL(glob_cxt->foreignrel))
return false;
- }
- /*
- * When processing a list, collation state just bubbles up
- * from the list elements.
- */
- collation = inner_cxt.collation;
- state = inner_cxt.state;
+ /* Only non-split aggregates are pushable. */
+ if (agg->aggsplit != AGGSPLIT_SIMPLE)
+ return false;
- /* Don't apply exprType() to the list. */
- check_type = false;
- }
- break;
- case T_Aggref:
- {
- Aggref *agg = (Aggref *) node;
- ListCell *lc;
+ /* As usual, it must be shippable. */
+ if (!is_shippable(agg->aggfnoid, ProcedureRelationId, fpinfo))
+ return false;
- /* Not safe to pushdown when not in grouping context */
- if (!IS_UPPER_REL(glob_cxt->foreignrel))
- return false;
+ /*
+ * Recurse to input args. aggdirectargs, aggorder and
+ * aggdistinct are all present in args, so no need to
+ * check their shippability explicitly.
+ */
+ foreach(lc, agg->args)
+ {
+ Node *n = (Node *) lfirst(lc);
- /* Only non-split aggregates are pushable. */
- if (agg->aggsplit != AGGSPLIT_SIMPLE)
- return false;
+ /* If TargetEntry, extract the expression from it */
+ if (IsA(n, TargetEntry))
+ {
+ TargetEntry *tle = (TargetEntry *) n;
- /* As usual, it must be shippable. */
- if (!is_shippable(agg->aggfnoid, ProcedureRelationId, fpinfo))
- return false;
+ n = (Node *) tle->expr;
+ }
- /*
- * Recurse to input args. aggdirectargs, aggorder and
- * aggdistinct are all present in args, so no need to check
- * their shippability explicitly.
- */
- foreach(lc, agg->args)
- {
- Node *n = (Node *) lfirst(lc);
+ if (!foreign_expr_walker(n,
+ glob_cxt, &inner_cxt, case_arg_cxt))
+ return false;
+ }
- /* If TargetEntry, extract the expression from it */
- if (IsA(n, TargetEntry))
+ /*
+ * For aggorder elements, check whether the sort operator,
+ * if specified, is shippable or not.
+ */
+ if (agg->aggorder)
{
- TargetEntry *tle = (TargetEntry *) n;
-
- n = (Node *) tle->expr;
+ ListCell *lc;
+
+ foreach(lc, agg->aggorder)
+ {
+ SortGroupClause *srt = (SortGroupClause *) lfirst(lc);
+ Oid sortcoltype;
+ TypeCacheEntry *typentry;
+ TargetEntry *tle;
+
+ tle = get_sortgroupref_tle(srt->tleSortGroupRef,
+ agg->args);
+ sortcoltype = exprType((Node *) tle->expr);
+ typentry = lookup_type_cache(sortcoltype,
+ TYPECACHE_LT_OPR | TYPECACHE_GT_OPR);
+
+ /*
+ * Check shippability of non-default sort
+ * operator.
+ */
+ if (srt->sortop != typentry->lt_opr &&
+ srt->sortop != typentry->gt_opr &&
+ !is_shippable(srt->sortop, OperatorRelationId,
+ fpinfo))
+ return false;
+ }
}
- if (!foreign_expr_walker(n,
+ /* Check aggregate filter */
+ if (!foreign_expr_walker((Node *) agg->aggfilter,
glob_cxt, &inner_cxt, case_arg_cxt))
return false;
- }
- /*
- * For aggorder elements, check whether the sort operator, if
- * specified, is shippable or not.
- */
- if (agg->aggorder)
- {
- ListCell *lc;
+ /*
+ * If aggregate's input collation is not derived from a
+ * foreign Var, it can't be sent to remote.
+ */
+ if (agg->inputcollid == InvalidOid)
+ /* OK, inputs are all noncollatable */ ;
+ else if (inner_cxt.state != FDW_COLLATE_SAFE ||
+ agg->inputcollid != inner_cxt.collation)
+ return false;
- foreach(lc, agg->aggorder)
- {
- SortGroupClause *srt = (SortGroupClause *) lfirst(lc);
- Oid sortcoltype;
- TypeCacheEntry *typentry;
- TargetEntry *tle;
-
- tle = get_sortgroupref_tle(srt->tleSortGroupRef,
- agg->args);
- sortcoltype = exprType((Node *) tle->expr);
- typentry = lookup_type_cache(sortcoltype,
- TYPECACHE_LT_OPR | TYPECACHE_GT_OPR);
- /* Check shippability of non-default sort operator. */
- if (srt->sortop != typentry->lt_opr &&
- srt->sortop != typentry->gt_opr &&
- !is_shippable(srt->sortop, OperatorRelationId,
- fpinfo))
- return false;
- }
+ /*
+ * Detect whether node is introducing a collation not
+ * derived from a foreign Var. (If so, we just mark it
+ * unsafe for now rather than immediately returning false,
+ * since the parent node might not care.)
+ */
+ collation = agg->aggcollid;
+ if (collation == InvalidOid)
+ state = FDW_COLLATE_NONE;
+ else if (inner_cxt.state == FDW_COLLATE_SAFE &&
+ collation == inner_cxt.collation)
+ state = FDW_COLLATE_SAFE;
+ else if (collation == DEFAULT_COLLATION_OID)
+ state = FDW_COLLATE_NONE;
+ else
+ state = FDW_COLLATE_UNSAFE;
}
-
- /* Check aggregate filter */
- if (!foreign_expr_walker((Node *) agg->aggfilter,
- glob_cxt, &inner_cxt, case_arg_cxt))
- return false;
-
- /*
- * If aggregate's input collation is not derived from a
- * foreign Var, it can't be sent to remote.
- */
- if (agg->inputcollid == InvalidOid)
- /* OK, inputs are all noncollatable */ ;
- else if (inner_cxt.state != FDW_COLLATE_SAFE ||
- agg->inputcollid != inner_cxt.collation)
- return false;
+ break;
+ default:
/*
- * Detect whether node is introducing a collation not derived
- * from a foreign Var. (If so, we just mark it unsafe for now
- * rather than immediately returning false, since the parent
- * node might not care.)
+ * If it's anything else, assume it's unsafe. This list can
+ * be expanded later, but don't forget to add deparse support
+ * below.
*/
- collation = agg->aggcollid;
- if (collation == InvalidOid)
- state = FDW_COLLATE_NONE;
- else if (inner_cxt.state == FDW_COLLATE_SAFE &&
- collation == inner_cxt.collation)
- state = FDW_COLLATE_SAFE;
- else if (collation == DEFAULT_COLLATION_OID)
- state = FDW_COLLATE_NONE;
- else
- state = FDW_COLLATE_UNSAFE;
- }
- break;
- default:
-
- /*
- * If it's anything else, assume it's unsafe. This list can be
- * expanded later, but don't forget to add deparse support below.
- */
- return false;
+ return false;
+ }
}
/*
@@ -1009,6 +1041,15 @@ is_foreign_param(PlannerInfo *root,
if (expr == NULL)
return false;
+ if (is_stable_expr((Node *) expr))
+ {
+ /*
+ * We can get here only if is_foreign_expr(expr) returned true, so
+ * it's a supported stable expression.
+ */
+ return true;
+ }
+
switch (nodeTag(expr))
{
case T_Var:
@@ -2575,6 +2616,12 @@ deparseExpr(Expr *node, deparse_expr_cxt *context)
if (node == NULL)
return;
+ if (is_stable_expr((Node *) node))
+ {
+ deparseStableExpr(node, context);
+ return;
+ }
+
switch (nodeTag(node))
{
case T_Var:
@@ -3192,6 +3239,45 @@ deparseRelabelType(RelabelType *node, deparse_expr_cxt *context)
node->resulttypmod));
}
+/*
+ * Deparse a stable expression as param
+ */
+static void
+deparseStableExpr(Expr *node, deparse_expr_cxt *context)
+{
+ int32 type = exprType((Node *) node);
+ int32 typmod = exprTypmod((Node *) node);
+
+ Assert(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, type, typmod, context);
+ }
+ else
+ {
+ printRemotePlaceholder(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..5defc2d3758 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -1073,6 +1073,93 @@ 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 stable expressions 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)) 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)
+
+-- also shippable as whole expression is executed locally
+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
+ Remote SQL: SELECT "C 1", c2, c4 FROM "S 1"."T 1" WHERE (("C 1" > 990)) AND ((c4 > $1::timestamp with time zone)) ORDER BY "C 1" ASC NULLS LAST
+(3 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..acb36394fc6 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 is_stable_expr(Node *node);
#endif /* POSTGRES_FDW_H */
diff --git a/contrib/postgres_fdw/shippable.c b/contrib/postgres_fdw/shippable.c
index 8e759da00d5..5173d750035 100644
--- a/contrib/postgres_fdw/shippable.c
+++ b/contrib/postgres_fdw/shippable.c
@@ -25,9 +25,14 @@
#include "access/transam.h"
#include "catalog/dependency.h"
+#include "catalog/pg_proc.h"
+#include "nodes/nodeFuncs.h"
+#include "optimizer/clauses.h"
+#include "optimizer/optimizer.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 */
@@ -52,6 +57,7 @@ typedef struct
bool shippable;
} ShippableCacheEntry;
+static bool contain_params_walker(Node *node, bool *context);
/*
* Flush cache entries when pg_foreign_server is updated.
@@ -209,3 +215,71 @@ is_shippable(Oid objectId, Oid classId, PgFdwRelationInfo *fpinfo)
return entry->shippable;
}
+
+/*
+ * contain_params
+ * Does this node has params?
+ */
+static bool
+contain_params(Node *node)
+{
+ bool contains = false;
+
+ contain_params_walker(node, (void *) &contains);
+
+ return contains;
+}
+
+static bool
+contain_params_walker(Node *node, bool *context)
+{
+ if (node == NULL)
+ return false;
+ if (IsA(node, Param))
+ {
+ *context = true;
+
+ return false;
+ }
+ return expression_tree_walker(node, contain_params_walker,
+ (void *) context);
+}
+
+/*
+ * Check if expression is stable
+ */
+bool
+is_stable_expr(Node *node)
+{
+ if (node == NULL)
+ return false;
+
+ /* No need to turn on 'ship stable expression' machinery in these cases */
+ if (IsA(node, Const) || IsA(node, List))
+ return false;
+
+ /* Expression shouldn't reference any table */
+ if (contain_var_clause(node))
+ return false;
+
+ if (contain_volatile_functions(node))
+ return false;
+
+ if (contain_subplans(node))
+ return false;
+
+ if (contain_params(node))
+ return false;
+
+ if (contain_agg_clause(node))
+ return false;
+
+ if (contain_mutable_functions(node))
+ {
+ /* These are not volatile functions, so they are stable */
+ if (exprType(node) != InvalidOid)
+ return true;
+ }
+
+ return false;
+}
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 9eb673e3693..de084bc4ece 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 stable expressions 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;
+
+-- also shippable as whole expression is executed locally
+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;
--
2.25.1
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2022-01-21 19:57 Tom Lane <[email protected]>
parent: Alexander Pyhalov <[email protected]>
0 siblings, 1 reply; 23+ messages in thread
From: Tom Lane @ 2022-01-21 19:57 UTC (permalink / raw)
To: Alexander Pyhalov <[email protected]>; +Cc: Ranier Vilela <[email protected]>; Zhihong Yu <[email protected]>; pgsql-hackers
Alexander Pyhalov <[email protected]> writes:
> So far I have the following prototype. It seems to be working, but I
> think it can be enhanced.
> At least, some sort of caching seems to be necessary for
> is_stable_expr().
Yeah, from a performance standpoint this seems pretty horrid ---
it's probably exponential in the size of the expressions considered
because of the repeated recursions.
The approach I had in mind was to extend the responsibility of
foreign_expr_walker to make it deduce the classification of
an expression in a bottom-up fashion. Also, as I noted before,
we don't want to re-deduce that in a later deparse pass, so
maybe we should just go ahead and deparse during foreign_expr_walker.
Not sure about that part. It sounds expensive; but recording the
classification results in a way we could re-use later doesn't seem
too cheap either.
BTW, the patch is just about unreadable as-is. It would have been
better to not have re-indented the bulk of foreign_expr_walker,
leaving that for some later pgindent pass. But that may be moot,
because I don't think we can tolerate the double-recursion approach
you took here.
regards, tom lane
^ permalink raw reply [nested|flat] 23+ messages in thread
* Re: Push down time-related SQLValue functions to foreign server
@ 2022-08-02 20:59 Jacob Champion <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 0 replies; 23+ messages in thread
From: Jacob Champion @ 2022-08-02 20:59 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Alexander Pyhalov <[email protected]>; +Cc: Ranier Vilela <[email protected]>; Zhihong Yu <[email protected]>; pgsql-hackers
This entry has been waiting on author input for a while (our current
threshold is roughly two weeks), so I've marked it Returned with
Feedback.
Once you think the patchset is ready for review again, you (or any
interested party) can resurrect the patch entry by visiting
https://commitfest.postgresql.org/38/3289/
and changing the status to "Needs Review", and then changing the
status again to "Move to next CF". (Don't forget the second step;
hopefully we will have streamlined this in the near future!)
Thanks,
--Jacob
^ permalink raw reply [nested|flat] 23+ messages in thread
end of thread, other threads:[~2022-08-02 20:59 UTC | newest]
Thread overview: 23+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-20 01:05 [PATCH v2 1/3] WIP: elog: Make ereport a bit easier to use and a bit faster. Andres Freund <[email protected]>
2020-12-02 05:11 [PATCH v9 2/4] WIP: Check for volatile defaults Justin Pryzby <[email protected]>
2021-08-19 09:51 Push down time-related SQLValue functions to foreign server Alexander Pyhalov <[email protected]>
2021-08-19 10:22 ` Re: Push down time-related SQLValue functions to foreign server Zhihong Yu <[email protected]>
2021-08-19 10:24 ` Re: Push down time-related SQLValue functions to foreign server Alexander Pyhalov <[email protected]>
2021-08-19 10:55 ` Re: Push down time-related SQLValue functions to foreign server Zhihong Yu <[email protected]>
2021-08-19 11:01 ` Re: Push down time-related SQLValue functions to foreign server Ranier Vilela <[email protected]>
2021-08-20 07:13 ` Re: Push down time-related SQLValue functions to foreign server Alexander Pyhalov <[email protected]>
2021-08-20 11:19 ` Re: Push down time-related SQLValue functions to foreign server Ranier Vilela <[email protected]>
2021-08-20 12:18 ` Re: Push down time-related SQLValue functions to foreign server Alexander Pyhalov <[email protected]>
2021-08-20 13:59 ` Re: Push down time-related SQLValue functions to foreign server Ranier Vilela <[email protected]>
2021-08-20 13:28 ` Re: Push down time-related SQLValue functions to foreign server Zhihong Yu <[email protected]>
2022-01-17 23:08 ` Re: Push down time-related SQLValue functions to foreign server Tom Lane <[email protected]>
2022-01-18 08:52 ` Re: Push down time-related SQLValue functions to foreign server Alexander Pyhalov <[email protected]>
2022-01-18 16:53 ` Re: Push down time-related SQLValue functions to foreign server Tom Lane <[email protected]>
2022-01-18 19:02 ` Re: Push down time-related SQLValue functions to foreign server Alexander Pyhalov <[email protected]>
2022-01-18 19:11 ` Re: Push down time-related SQLValue functions to foreign server Tom Lane <[email protected]>
2022-01-18 20:01 ` Re: Push down time-related SQLValue functions to foreign server Tom Lane <[email protected]>
2022-01-20 13:42 ` Re: Push down time-related SQLValue functions to foreign server Alexander Pyhalov <[email protected]>
2022-01-21 19:57 ` Re: Push down time-related SQLValue functions to foreign server Tom Lane <[email protected]>
2022-08-02 20:59 ` Re: Push down time-related SQLValue functions to foreign server Jacob Champion <[email protected]>
2021-08-19 14:01 ` Re: Push down time-related SQLValue functions to foreign server Ashutosh Bapat <[email protected]>
2021-08-20 07:36 ` Re: Push down time-related SQLValue functions to foreign server Alexander Pyhalov <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox