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/2] 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/2] 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) {