From 5ab6c730d81671fdd3923cb75c56c8ab79b628de Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 2 Aug 2019 15:50:01 +0200 Subject: [PATCH 1/3] An experimental branch to try to push down aggregate functions What this does: - it adds a ton of traces, that are more suitable to follow certain aspects of the code that raw gdb on the foreign_expr_walker recursive exploration of the parse tree. - it modifies the code related to checking of collations. I don't really understand if it is a bug or if there's actually a conflict with collations and it is unsafe to push, but I tend to think it is indeed a bug as no collation should affect it. - 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 really valid and fails with an error on the remote end. It shall not confuse the representation in the EXPLAIN of ROW() with the SQL ROW() row constructor. In short this just half-works, and we really need both halfs for it to do something interesting. Nevertheless I think it is good to share, to give an idea of the type of work to accomplish to finish this and similar cases. --- contrib/postgres_fdw/deparse.c | 138 ++++++++++++++++++++++++++------- 1 file changed, 111 insertions(+), 27 deletions(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index e7b3cf35eca94..9bb8d69ee3db3 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); @@ -338,8 +339,10 @@ foreign_expr_walker(Node *node, */ if (var->varattno < 0 && var->varattno != SelfItemPointerAttributeNumber && - var->varattno != ObjectIdAttributeNumber) + var->varattno != ObjectIdAttributeNumber) { + elog(WARNING, "RTORRE return false 1"); return false; + } /* Else check the collation */ collation = var->varcollid; @@ -407,8 +410,10 @@ foreign_expr_walker(Node *node, ArrayRef *ar = (ArrayRef *) node; /* Assignment should not be in restrictions. */ - if (ar->refassgnexpr != NULL) + if (ar->refassgnexpr != NULL) { + elog(WARNING, "RTORRE return false 2"); return false; + } /* * Recurse to remaining subexpressions. Since the array @@ -416,14 +421,20 @@ foreign_expr_walker(Node *node, * affect the inner_cxt state. */ if (!foreign_expr_walker((Node *) ar->refupperindexpr, - glob_cxt, &inner_cxt)) + glob_cxt, &inner_cxt)) { + elog(WARNING, "RTORRE return false 3"); return false; + } if (!foreign_expr_walker((Node *) ar->reflowerindexpr, - glob_cxt, &inner_cxt)) + glob_cxt, &inner_cxt)) { + elog(WARNING, "RTORRE return false 4"); return false; + } if (!foreign_expr_walker((Node *) ar->refexpr, glob_cxt, &inner_cxt)) + elog(WARNING, "RTORRE return false 5"); { return false; + } /* * Array subscripting should yield same collation as input, @@ -450,15 +461,19 @@ foreign_expr_walker(Node *node, * can't be sent to remote because it might have incompatible * semantics on remote side. */ - if (!is_shippable(fe->funcid, ProcedureRelationId, fpinfo)) + if (!is_shippable(fe->funcid, ProcedureRelationId, fpinfo)) { + elog(WARNING, "RTORRE return false 6"); return false; + } /* * Recurse to input subexpressions. */ if (!foreign_expr_walker((Node *) fe->args, - glob_cxt, &inner_cxt)) + glob_cxt, &inner_cxt)) { + elog(WARNING, "RTORRE return false 7"); return false; + } /* * If function's input collation is not derived from a foreign @@ -467,8 +482,10 @@ foreign_expr_walker(Node *node, if (fe->inputcollid == InvalidOid) /* OK, inputs are all noncollatable */ ; else if (inner_cxt.state != FDW_COLLATE_SAFE || - fe->inputcollid != inner_cxt.collation) + fe->inputcollid != inner_cxt.collation) { + elog(WARNING, "RTORRE return false 8"); return false; + } /* * Detect whether node is introducing a collation not derived @@ -498,15 +515,19 @@ foreign_expr_walker(Node *node, * (If the operator is shippable, we assume its underlying * function is too.) */ - if (!is_shippable(oe->opno, OperatorRelationId, fpinfo)) + if (!is_shippable(oe->opno, OperatorRelationId, fpinfo)) { + elog(WARNING, "RTORRE return false 9"); return false; + } /* * Recurse to input subexpressions. */ if (!foreign_expr_walker((Node *) oe->args, - glob_cxt, &inner_cxt)) + glob_cxt, &inner_cxt)) { + elog(WARNING, "RTORRE return false 10"); return false; + } /* * If operator's input collation is not derived from a foreign @@ -515,8 +536,10 @@ foreign_expr_walker(Node *node, if (oe->inputcollid == InvalidOid) /* OK, inputs are all noncollatable */ ; else if (inner_cxt.state != FDW_COLLATE_SAFE || - oe->inputcollid != inner_cxt.collation) + oe->inputcollid != inner_cxt.collation) { + elog(WARNING, "RTORRE return false 11"); return false; + } /* Result-collation handling is same as for functions */ collation = oe->opcollid; @@ -538,15 +561,19 @@ foreign_expr_walker(Node *node, /* * Again, only shippable operators can be sent to remote. */ - if (!is_shippable(oe->opno, OperatorRelationId, fpinfo)) + if (!is_shippable(oe->opno, OperatorRelationId, fpinfo)) { + elog(WARNING, "RTORRE return false 12"); return false; + } /* * Recurse to input subexpressions. */ if (!foreign_expr_walker((Node *) oe->args, - glob_cxt, &inner_cxt)) + glob_cxt, &inner_cxt)) { + elog(WARNING, "RTORRE return false 13"); return false; + } /* * If operator's input collation is not derived from a foreign @@ -555,8 +582,10 @@ foreign_expr_walker(Node *node, if (oe->inputcollid == InvalidOid) /* OK, inputs are all noncollatable */ ; else if (inner_cxt.state != FDW_COLLATE_SAFE || - oe->inputcollid != inner_cxt.collation) + oe->inputcollid != inner_cxt.collation) { + elog(WARNING, "RTORRE return false 14"); return false; + } /* Output is always boolean and so noncollatable. */ collation = InvalidOid; @@ -571,8 +600,10 @@ foreign_expr_walker(Node *node, * Recurse to input subexpression. */ if (!foreign_expr_walker((Node *) r->arg, - glob_cxt, &inner_cxt)) + glob_cxt, &inner_cxt)) { + elog(WARNING, "RTORRE return false 15"); return false; + } /* * RelabelType must not introduce a collation not derived from @@ -598,8 +629,10 @@ foreign_expr_walker(Node *node, * Recurse to input subexpressions. */ if (!foreign_expr_walker((Node *) b->args, - glob_cxt, &inner_cxt)) + glob_cxt, &inner_cxt)) { + elog(WARNING, "RTORRE return false 16"); return false; + } /* Output is always boolean and so noncollatable. */ collation = InvalidOid; @@ -614,8 +647,10 @@ foreign_expr_walker(Node *node, * Recurse to input subexpressions. */ if (!foreign_expr_walker((Node *) nt->arg, - glob_cxt, &inner_cxt)) + glob_cxt, &inner_cxt)) { + elog(WARNING, "RTORRE return false 17"); return false; + } /* Output is always boolean and so noncollatable. */ collation = InvalidOid; @@ -630,8 +665,10 @@ foreign_expr_walker(Node *node, * Recurse to input subexpressions. */ if (!foreign_expr_walker((Node *) a->elements, - glob_cxt, &inner_cxt)) + glob_cxt, &inner_cxt)) { + elog(WARNING, "RTORRE return false 18"); return false; + } /* * ArrayExpr must not introduce a collation not derived from @@ -660,8 +697,10 @@ foreign_expr_walker(Node *node, foreach(lc, l) { if (!foreign_expr_walker((Node *) lfirst(lc), - glob_cxt, &inner_cxt)) + glob_cxt, &inner_cxt)) { + elog(WARNING, "RTORRE return false 19"); return false; + } } /* @@ -681,16 +720,22 @@ foreign_expr_walker(Node *node, ListCell *lc; /* Not safe to pushdown when not in grouping context */ - if (!IS_UPPER_REL(glob_cxt->foreignrel)) + if (!IS_UPPER_REL(glob_cxt->foreignrel)) { + elog(WARNING, "RTORRE return false 20"); return false; + } /* Only non-split aggregates are pushable. */ - if (agg->aggsplit != AGGSPLIT_SIMPLE) + if (agg->aggsplit != AGGSPLIT_SIMPLE) { + elog(WARNING, "RTORRE return false 21"); return false; + } /* As usual, it must be shippable. */ - if (!is_shippable(agg->aggfnoid, ProcedureRelationId, fpinfo)) + if (!is_shippable(agg->aggfnoid, ProcedureRelationId, fpinfo)) { + elog(WARNING, "RTORRE return false 22"); return false; + } /* * Recurse to input args. aggdirectargs, aggorder and @@ -709,8 +754,10 @@ foreign_expr_walker(Node *node, n = (Node *) tle->expr; } - if (!foreign_expr_walker(n, glob_cxt, &inner_cxt)) + if (!foreign_expr_walker(n, glob_cxt, &inner_cxt)) { + elog(WARNING, "RTORRE return false 23"); return false; + } } /* @@ -737,25 +784,31 @@ foreign_expr_walker(Node *node, if (srt->sortop != typentry->lt_opr && srt->sortop != typentry->gt_opr && !is_shippable(srt->sortop, OperatorRelationId, - fpinfo)) + fpinfo)) { + elog(WARNING, "RTORRE return false 24"); return false; + } } } /* Check aggregate filter */ if (!foreign_expr_walker((Node *) agg->aggfilter, - glob_cxt, &inner_cxt)) + glob_cxt, &inner_cxt)) { + elog(WARNING, "RTORRE return false 25"); 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) + 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) + agg->inputcollid != inner_cxt.collation) { + elog(WARNING, "RTORRE return false 26"); return false; + } /* * Detect whether node is introducing a collation not derived @@ -775,12 +828,21 @@ 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. + */ + ereport(WARNING, (errmsg_internal("RTORRE in a T_RowExpr"))); + return true; + 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. */ + elog(WARNING, "RTORRE unrecognized node type: %d", (int) nodeTag(node)); return false; } @@ -2354,6 +2416,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 +2426,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, "("); + 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 625debf5fe97f63f8f7e3e296ee64d34cfdecae2 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Fri, 2 Aug 2019 17:54:22 +0200 Subject: [PATCH 2/3] Add ROW back (easier for debugging) --- 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 9bb8d69ee3db3..59c33c5b87478 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -2433,7 +2433,7 @@ deparseRowExpr(RowExpr *node, deparse_expr_cxt *context) bool first; ListCell *lc; - appendStringInfoString(buf, "("); + appendStringInfoString(buf, "ROW("); first = true; foreach(lc, node->args) { From d286c3cc0db379a1027a3d3750a8f16b5097deb2 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Tue, 10 Sep 2019 17:21:34 +0200 Subject: [PATCH 3/3] 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 59c33c5b87478..893b1db32aeec 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" @@ -1060,6 +1061,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); @@ -1186,6 +1188,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); @@ -1196,6 +1218,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; + } } /* @@ -2430,19 +2458,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; } /*