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 01/13] 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 02/13] 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 03/13] 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 04/13] 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 05/13] 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) From c746d2e2f9173a3d6bed24b0091368329a1aa7d4 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Wed, 2 Oct 2019 17:51:38 +0200 Subject: [PATCH 06/13] Revert "Fix collation safety rules for aggregations" This reverts commit 83b3ef30d328c089d0ceecd05404d3689dfc5b47. --- 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 3ac1c4ca627eb..4064307b3474d 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -753,7 +753,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 || inner_cxt.state == FDW_COLLATE_NONE) + if (agg->inputcollid == InvalidOid) /* OK, inputs are all noncollatable */ ; else if (inner_cxt.state != FDW_COLLATE_SAFE || agg->inputcollid != inner_cxt.collation) From 7bc49bc80b9fce79b367e56b57fa5b029728055b Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Wed, 2 Oct 2019 17:51:56 +0200 Subject: [PATCH 07/13] Revert "Fix collation in other instances (to be reviewed)" This reverts commit 125f0fd3b19404b0c4a495a9fca8477d0f2d6e97. --- 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 4064307b3474d..86530091ac4e4 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 || inner_cxt.state == FDW_COLLATE_NONE) + if (fe->inputcollid == InvalidOid) /* 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 || inner_cxt.state == FDW_COLLATE_NONE) + if (oe->inputcollid == InvalidOid) /* 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 || inner_cxt.state == FDW_COLLATE_NONE) + if (oe->inputcollid == InvalidOid) /* OK, inputs are all noncollatable */ ; else if (inner_cxt.state != FDW_COLLATE_SAFE || oe->inputcollid != inner_cxt.collation) From 75ccf76770df81c1bddb49ad55006e20d9343dba Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 3 Oct 2019 08:51:54 +0200 Subject: [PATCH 08/13] A better implementation of case T_RowExpr --- contrib/postgres_fdw/deparse.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 86530091ac4e4..3a2dc0fa61852 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -778,12 +778,27 @@ foreign_expr_walker(Node *node, } break; 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; + { + RowExpr *re = (RowExpr *) node; + + /* + * Recurse to input subexpressions. + */ + if (!foreign_expr_walker((Node *) re->args, + glob_cxt, &inner_cxt)) + return false; + + /* + * From primnodes.h: + * + * We don't need to store a collation either. The result type + * is necessarily composite, and composite types never have a + * collation. + */ + collation = InvalidOid; + state = FDW_COLLATE_NONE; + } + break; default: /* From f3e6de28cb1421d38936802a1d866e46874fdf5e Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Thu, 3 Oct 2019 18:56:50 +0200 Subject: [PATCH 09/13] A better implementation of RowExpr deparsing This works with nested row expressions. E.g: EXPLAIN (analyze,VERBOSE) select count(((t, 'hola'::text))) FROM (SELECT cartodb_id, street FROM cdb_fdw_local_pg11.sf_stclines) t; Remote SQL: SELECT count(s1) FROM (SELECT s2, 'hola'::text FROM (SELECT cartodb_id, street FROM carto_lite.sf_stclines) s2) s1 --- contrib/postgres_fdw/deparse.c | 94 ++++++++++++++++++++++++---------- 1 file changed, 67 insertions(+), 27 deletions(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 3a2dc0fa61852..3bcfd6e179c11 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -103,7 +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 */ + List **row_exprs_list; /* used for later generation of equivalent subquery */ } deparse_expr_cxt; #define REL_ALIAS_PREFIX "r" @@ -780,13 +780,33 @@ foreign_expr_walker(Node *node, case T_RowExpr: { RowExpr *re = (RowExpr *) node; + ListCell *lc; + + elog(NOTICE, "in case T_RowExpr"); + + if(re->row_typeid != RECORDOID) + { + elog(NOTICE, "not a RECORDOID, returning false"); + return false; + } /* * Recurse to input subexpressions. */ - if (!foreign_expr_walker((Node *) re->args, - glob_cxt, &inner_cxt)) - return false; + /* if (!foreign_expr_walker((Node *) re->args, */ + /* glob_cxt, &inner_cxt)) */ + /* return false; */ + + /* + * Recurse to input subexpressions. + */ + foreach(lc, re->args) + { + if (!foreign_expr_walker((Node *) lfirst(lc), + glob_cxt, &inner_cxt)) + return false; + } + /* * From primnodes.h: @@ -1009,6 +1029,7 @@ deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root, RelOptInfo *rel, deparse_expr_cxt context; PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) rel->fdw_private; List *quals; + List *row_exprs_list = NIL; /* * We handle relations for foreign tables, joins between those and upper @@ -1022,7 +1043,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; + context.row_exprs_list = &row_exprs_list; /* Construct SELECT clause */ deparseSelectSql(tlist, is_subquery, retrieved_attrs, &context); @@ -1142,6 +1163,7 @@ deparseFromExpr(List *quals, deparse_expr_cxt *context) { StringInfo buf = context->buf; RelOptInfo *scanrel = context->scanrel; + List **row_exprs_list = context->row_exprs_list; /* For upper relations, scanrel must be either a joinrel or a baserel */ Assert(!IS_UPPER_REL(context->foreignrel) || @@ -1150,23 +1172,29 @@ 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) + // We have one or more row expressions. Add the corresponding subqueries + if (*row_exprs_list != NIL) + { + ListCell *lcre; + foreach(lcre, *row_exprs_list) { - if (!first) - appendStringInfo(buf, ", "); - deparseExpr((Expr *) lfirst(lc), context); - first = false; - } + RowExpr *node = (RowExpr *) lfirst(lcre); + bool first; + ListCell *lc; + + appendStringInfoString(buf, "(SELECT "); + first = true; + foreach(lc, node->args) + { + if (!first) + appendStringInfo(buf, ", "); + deparseExpr((Expr *) lfirst(lc), context); + first = false; + } + + appendStringInfoString(buf, " FROM "); - appendStringInfoString(buf, " FROM "); + } } deparseFromExprForRel(buf, context->root, scanrel, @@ -1181,9 +1209,17 @@ deparseFromExpr(List *quals, deparse_expr_cxt *context) } // Close subquery and add an alias - if (context->row_expr) { - appendStringInfoString(buf, ") myalias"); - context->row_expr = NULL; + if (*row_exprs_list != NIL) + { + ListCell *lc; + int i = list_length(*row_exprs_list); + foreach(lc, *row_exprs_list) + { + appendStringInfo(buf, ") %s%d", + SUBQUERY_REL_ALIAS_PREFIX, + i); + i--; + } } } @@ -2419,12 +2455,16 @@ static void deparseRowExpr(RowExpr *node, deparse_expr_cxt *context) { StringInfo buf = context->buf; - - // Add an arbitrary alias - appendStringInfoString(buf, "myalias"); + List **row_exprs_list = context->row_exprs_list; + elog(NOTICE, "In deparseRowExpr"); // Just save the node for later generation of subquery - context->row_expr = node; + *row_exprs_list = lappend(*row_exprs_list, node); + + // Add an arbitrary alias + appendStringInfo(buf, "%s%d", + SUBQUERY_REL_ALIAS_PREFIX, + list_length(*row_exprs_list)); } /* From ee627e01ceca5f9fd590d9a5c8655ac49f16e1de Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 4 Oct 2019 10:39:39 +0200 Subject: [PATCH 10/13] Cleanup deparseRowExpr function --- contrib/postgres_fdw/deparse.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 3bcfd6e179c11..f5df3294fe2d3 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -782,8 +782,6 @@ foreign_expr_walker(Node *node, RowExpr *re = (RowExpr *) node; ListCell *lc; - elog(NOTICE, "in case T_RowExpr"); - if(re->row_typeid != RECORDOID) { elog(NOTICE, "not a RECORDOID, returning false"); @@ -793,20 +791,9 @@ foreign_expr_walker(Node *node, /* * Recurse to input subexpressions. */ - /* if (!foreign_expr_walker((Node *) re->args, */ - /* glob_cxt, &inner_cxt)) */ - /* return false; */ - - /* - * Recurse to input subexpressions. - */ - foreach(lc, re->args) - { - if (!foreign_expr_walker((Node *) lfirst(lc), - glob_cxt, &inner_cxt)) - return false; - } - + if (!foreign_expr_walker((Node *) re->args, + glob_cxt, &inner_cxt)) + return false; /* * From primnodes.h: @@ -814,9 +801,11 @@ foreign_expr_walker(Node *node, * We don't need to store a collation either. The result type * is necessarily composite, and composite types never have a * collation. + * + * Bubble up collation state from args, just like for lists. */ - collation = InvalidOid; - state = FDW_COLLATE_NONE; + collation = inner_cxt.collation; + state = inner_cxt.state; } break; default: From 3e3ace4b7f2319ca533f3ac6fd7d2e9d86da300e Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 4 Oct 2019 10:41:22 +0200 Subject: [PATCH 11/13] Fix collation of Const: mark as safe when using default. --- contrib/postgres_fdw/deparse.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index f5df3294fe2d3..e880fe0ec926d 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -382,11 +382,16 @@ foreign_expr_walker(Node *node, * non-collation-sensitive context. */ collation = c->constcollid; - if (collation == InvalidOid || - collation == DEFAULT_COLLATION_OID) - state = FDW_COLLATE_NONE; - else - state = FDW_COLLATE_UNSAFE; + switch(collation) { + case InvalidOid: + state = FDW_COLLATE_NONE; + break; + case DEFAULT_COLLATION_OID: + state = FDW_COLLATE_SAFE; + break; + default: + state = FDW_COLLATE_UNSAFE; + } } break; case T_Param: From 9b56fca149071f4abc297920fee91d731ebd840c Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 4 Oct 2019 10:46:06 +0200 Subject: [PATCH 12/13] Remove elog NOTICE --- contrib/postgres_fdw/deparse.c | 1 - 1 file changed, 1 deletion(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index e880fe0ec926d..6e0d97dfd90e8 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -2450,7 +2450,6 @@ deparseRowExpr(RowExpr *node, deparse_expr_cxt *context) { StringInfo buf = context->buf; List **row_exprs_list = context->row_exprs_list; - elog(NOTICE, "In deparseRowExpr"); // Just save the node for later generation of subquery *row_exprs_list = lappend(*row_exprs_list, node); From 3349a38cbbdd247c9ca32d1228f26e15621b949c Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 4 Oct 2019 10:49:15 +0200 Subject: [PATCH 13/13] Remove unused variable --- contrib/postgres_fdw/deparse.c | 1 - 1 file changed, 1 deletion(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 6e0d97dfd90e8..c89819c38e2a0 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -785,7 +785,6 @@ foreign_expr_walker(Node *node, case T_RowExpr: { RowExpr *re = (RowExpr *) node; - ListCell *lc; if(re->row_typeid != RECORDOID) {