From 424820c90427bd950e6de8225fa8669b77acd603 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 2 Aug 2019 15:50:01 +0200 Subject: [PATCH 1/5] An experimental commit to try to push down aggregate functions What this does: - it allows for Row Expressions node push downs (T_RowExpr) in a very hacky way. Chances are that to do it properly it needs to continue with a recursive exploration of the tree from that node. This was preventing the whole aggregate function ST_AsMVt to be pushed down to the foreign server. - it provides a crappy implementation of deparseRowExpr. It makes the planner happy to ship some SQL but it is not 100% valid as column names are lost when received in the foreign end. --- contrib/postgres_fdw/deparse.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index e7b3cf35eca94..c64da2c9b081a 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -182,6 +182,7 @@ static void appendGroupByClause(List *tlist, deparse_expr_cxt *context); static void appendAggOrderBy(List *orderList, List *targetList, deparse_expr_cxt *context); static void appendFunctionName(Oid funcid, deparse_expr_cxt *context); +static void deparseRowExpr(RowExpr *node, deparse_expr_cxt *context); static Node *deparseSortGroupClause(Index ref, List *tlist, bool force_colno, deparse_expr_cxt *context); @@ -775,7 +776,14 @@ foreign_expr_walker(Node *node, state = FDW_COLLATE_UNSAFE; } break; - default: + case T_RowExpr: + /* + * rtorre: this is a bold move, let's consider it true. Trying to + * cover the st_asmvt(ROW(st_asmvtgeom(...)) case. I guess the + * proper solution is to examine the row expression carefully. + */ + return true; + default: /* * If it's anything else, assume it's unsafe. This list can be @@ -2354,6 +2362,9 @@ deparseExpr(Expr *node, deparse_expr_cxt *context) case T_Aggref: deparseAggref((Aggref *) node, context); break; + case T_RowExpr: + deparseRowExpr((RowExpr *) node, context); + break; default: elog(ERROR, "unsupported expression type for deparse: %d", (int) nodeTag(node)); @@ -2361,6 +2372,25 @@ deparseExpr(Expr *node, deparse_expr_cxt *context) } } +static void +deparseRowExpr(RowExpr *node, deparse_expr_cxt *context) +{ + StringInfo buf = context->buf; + bool first; + ListCell *lc; + + appendStringInfoString(buf, "ROW("); + first = true; + foreach(lc, node->args) + { + if (!first) + appendStringInfo(buf, ", "); + deparseExpr((Expr *) lfirst(lc), context); + first = false; + } + appendStringInfoChar(buf, ')'); +} + /* * Deparse given Var node into context->buf. * From 83b3ef30d328c089d0ceecd05404d3689dfc5b47 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Wed, 11 Sep 2019 12:02:00 +0200 Subject: [PATCH 2/5] Fix collation safety rules for aggregations For default input collation, inner_cxt.state is marked as FDW_COLLATE_NONE. There's the situation in which agg.collation == DEFAULT_COLLATION_OID and thus it should be safe to push down the aggregation. --- contrib/postgres_fdw/deparse.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index c64da2c9b081a..dc71961fbaaa6 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -752,7 +752,7 @@ foreign_expr_walker(Node *node, * If aggregate's input collation is not derived from a * foreign Var, it can't be sent to remote. */ - if (agg->inputcollid == InvalidOid) + if (agg->inputcollid == InvalidOid || inner_cxt.state == FDW_COLLATE_NONE) /* OK, inputs are all noncollatable */ ; else if (inner_cxt.state != FDW_COLLATE_SAFE || agg->inputcollid != inner_cxt.collation) From 335f6877dbf027b0b81b0f511dbb8395e2244184 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Tue, 10 Sep 2019 17:21:34 +0200 Subject: [PATCH 3/5] Deparse Row Expression as subquery A row expression cannot be deparsed as ROW(...) cause otherwise column names are lost when sending the deparsed query to the remote. RowExpr may be present in the planner tree, as the optimizer may "pull up" aggregation subqueries and it certainly does. In particular, ST_AsMVT may produce different results or may miss the geom column if column names are generated as f1, f2, ..., fn. This solves that issue by deparsing the row expression as an alias and embedding it into a subquery. This is the intended syntax transformation in pseudo-code: SELECT row_expr FROM from_clause WHERE where_caluse => SELECT alias FROM ( SELECT row_expr_fields FROM from_clause WHERE where_clause ) alias --- contrib/postgres_fdw/deparse.c | 45 +++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index dc71961fbaaa6..3c3b8d14843fe 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -103,6 +103,7 @@ typedef struct deparse_expr_cxt * a base relation. */ StringInfo buf; /* output buffer to append to */ List **params_list; /* exprs that will become remote Params */ + RowExpr *row_expr; /* used for later generation of equivalent subquery */ } deparse_expr_cxt; #define REL_ALIAS_PREFIX "r" @@ -1006,6 +1007,7 @@ deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root, RelOptInfo *rel, context.foreignrel = rel; context.scanrel = IS_UPPER_REL(rel) ? fpinfo->outerrel : rel; context.params_list = params_list; + context.row_expr = NULL; /* Construct SELECT clause */ deparseSelectSql(tlist, is_subquery, retrieved_attrs, &context); @@ -1132,6 +1134,26 @@ deparseFromExpr(List *quals, deparse_expr_cxt *context) /* Construct FROM clause */ appendStringInfoString(buf, " FROM "); + + // We have a row expression. Add the corresponding subquery + if (context->row_expr) { + bool first; + ListCell *lc; + RowExpr *node = context->row_expr; + + appendStringInfoString(buf, "(SELECT "); + first = true; + foreach(lc, node->args) + { + if (!first) + appendStringInfo(buf, ", "); + deparseExpr((Expr *) lfirst(lc), context); + first = false; + } + + appendStringInfoString(buf, " FROM "); + } + deparseFromExprForRel(buf, context->root, scanrel, (bms_num_members(scanrel->relids) > 1), (Index) 0, NULL, context->params_list); @@ -1142,6 +1164,12 @@ deparseFromExpr(List *quals, deparse_expr_cxt *context) appendStringInfoString(buf, " WHERE "); appendConditions(quals, context); } + + // Close subquery and add an alias + if (context->row_expr) { + appendStringInfoString(buf, ") myalias"); + context->row_expr = NULL; + } } /* @@ -2376,19 +2404,12 @@ static void deparseRowExpr(RowExpr *node, deparse_expr_cxt *context) { StringInfo buf = context->buf; - bool first; - ListCell *lc; - appendStringInfoString(buf, "ROW("); - first = true; - foreach(lc, node->args) - { - if (!first) - appendStringInfo(buf, ", "); - deparseExpr((Expr *) lfirst(lc), context); - first = false; - } - appendStringInfoChar(buf, ')'); + // Add an arbitrary alias + appendStringInfoString(buf, "myalias"); + + // Just save the node for later generation of subquery + context->row_expr = node; } /* From 6fe4734d538a885e23392984ad25c14b7353749e Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Mon, 16 Sep 2019 12:40:34 +0200 Subject: [PATCH 4/5] Add public to execution environment of postgres_fdw That is the schema where postgis (and many other extensions) usually reside. This is done in order to overcome a bug in postgis type checking that yields "ERROR: parse_column_keys: no geometry column found". That bug was recently fixed in this commit: https://github.com/postgis/postgis/commit/e09679530977653a85ac54763ad057b81c7bd4ed#diff-86bc92dfea8fd30b975e278bd323067f --- contrib/postgres_fdw/connection.c | 4 ++-- contrib/postgres_fdw/expected/postgres_fdw.out | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index fe4893a8e054a..b318dfeab3379 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -359,8 +359,8 @@ configure_remote_session(PGconn *conn) { int remoteversion = PQserverVersion(conn); - /* Force the search path to contain only pg_catalog (see deparse.c) */ - do_sql_command(conn, "SET search_path = pg_catalog"); + /* Force the search path to contain only pg_catalog & public (see deparse.c) */ + do_sql_command(conn, "SET search_path = pg_catalog,public"); /* * Set remote timezone; this is basically just cosmetic, since all diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index f19f982e0ac9b..a1deffe14b5be 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -8373,13 +8373,13 @@ DROP TYPE "Colors" CASCADE; NOTICE: drop cascades to column Col of table import_source.t5 IMPORT FOREIGN SCHEMA import_source LIMIT TO (t5) FROM SERVER loopback INTO import_dest5; -- ERROR -ERROR: type "public.Colors" does not exist -LINE 4: "Col" public."Colors" OPTIONS (column_name 'Col') +ERROR: type "Colors" does not exist +LINE 4: "Col" "Colors" OPTIONS (column_name 'Col') ^ QUERY: CREATE FOREIGN TABLE t5 ( c1 integer OPTIONS (column_name 'c1'), c2 text OPTIONS (column_name 'c2') COLLATE pg_catalog."C", - "Col" public."Colors" OPTIONS (column_name 'Col') + "Col" "Colors" OPTIONS (column_name 'Col') ) SERVER loopback OPTIONS (schema_name 'import_source', table_name 't5'); CONTEXT: importing foreign table "t5" From 125f0fd3b19404b0c4a495a9fca8477d0f2d6e97 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 20 Sep 2019 15:45:55 +0200 Subject: [PATCH 5/5] Fix collation in other instances (to be reviewed) --- contrib/postgres_fdw/deparse.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 3c3b8d14843fe..3ac1c4ca627eb 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -466,7 +466,7 @@ foreign_expr_walker(Node *node, * If function's input collation is not derived from a foreign * Var, it can't be sent to remote. */ - if (fe->inputcollid == InvalidOid) + if (fe->inputcollid == InvalidOid || inner_cxt.state == FDW_COLLATE_NONE) /* OK, inputs are all noncollatable */ ; else if (inner_cxt.state != FDW_COLLATE_SAFE || fe->inputcollid != inner_cxt.collation) @@ -514,7 +514,7 @@ foreign_expr_walker(Node *node, * If operator's input collation is not derived from a foreign * Var, it can't be sent to remote. */ - if (oe->inputcollid == InvalidOid) + if (oe->inputcollid == InvalidOid || inner_cxt.state == FDW_COLLATE_NONE) /* OK, inputs are all noncollatable */ ; else if (inner_cxt.state != FDW_COLLATE_SAFE || oe->inputcollid != inner_cxt.collation) @@ -554,7 +554,7 @@ foreign_expr_walker(Node *node, * If operator's input collation is not derived from a foreign * Var, it can't be sent to remote. */ - if (oe->inputcollid == InvalidOid) + if (oe->inputcollid == InvalidOid || inner_cxt.state == FDW_COLLATE_NONE) /* OK, inputs are all noncollatable */ ; else if (inner_cxt.state != FDW_COLLATE_SAFE || oe->inputcollid != inner_cxt.collation)