Fix postgres_fdw to cope with duplicate GROUP BY entries.

Commit 7012b132, which added the ability to push down aggregates and
grouping to the remote server, wasn't careful to ensure that the remote
server would have the same idea we do about which columns are the grouping
columns, in cases where there are textually identical GROUP BY expressions.
Such cases typically led to "targetlist item has multiple sortgroupref
labels" errors.

To fix this reliably, switch over to using "GROUP BY column-number" syntax
rather than "GROUP BY expression" in transmitted queries, and adjust
foreign_grouping_ok() to be more careful about duplicating the sortgroupref
labeling of the local pathtarget.

Per bug #14890 from Sean Johnston.  Back-patch to v10 where the buggy code
was introduced.

Jeevan Chalke, reviewed by Ashutosh Bapat

Discussion: https://postgr.es/m/20171107134948.1508.94783@wrigleys.postgresql.org

contrib/postgres_fdw/deparse.c

@@ -178,7 +178,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 Node *deparseSortGroupClause(Index ref, List *tlist,
+static Node *deparseSortGroupClause(Index ref, List *tlist, bool force_colno,
 					   deparse_expr_cxt *context);
 
 /*
@@ -2853,7 +2853,7 @@ appendAggOrderBy(List *orderList, List *targetList, deparse_expr_cxt *context)
 		first = false;
 
 		sortexpr = deparseSortGroupClause(srt->tleSortGroupRef, targetList,
-										  context);
+										  false, context);
 		sortcoltype = exprType(sortexpr);
 		/* See whether operator is default < or > for datatype */
 		typentry = lookup_type_cache(sortcoltype,
@@ -2960,7 +2960,7 @@ appendGroupByClause(List *tlist, deparse_expr_cxt *context)
 			appendStringInfoString(buf, ", ");
 		first = false;
 
-		deparseSortGroupClause(grp->tleSortGroupRef, tlist, context);
+		deparseSortGroupClause(grp->tleSortGroupRef, tlist, true, context);
 	}
 }
 
@@ -3047,7 +3047,8 @@ appendFunctionName(Oid funcid, deparse_expr_cxt *context)
  * need not find it again.
  */
 static Node *
-deparseSortGroupClause(Index ref, List *tlist, deparse_expr_cxt *context)
+deparseSortGroupClause(Index ref, List *tlist, bool force_colno,
+					   deparse_expr_cxt *context)
 {
 	StringInfo	buf = context->buf;
 	TargetEntry *tle;
@@ -3056,7 +3057,13 @@ deparseSortGroupClause(Index ref, List *tlist, deparse_expr_cxt *context)
 	tle = get_sortgroupref_tle(ref, tlist);
 	expr = tle->expr;
 
-	if (expr && IsA(expr, Const))
+	if (force_colno)
+	{
+		/* Use column-number form when requested by caller. */
+		Assert(!tle->resjunk);
+		appendStringInfo(buf, "%d", tle->resno);
+	}
+	else if (expr && IsA(expr, Const))
 	{
 		/*
 		 * Force a typecast here so that we don't emit something like "GROUP

contrib/postgres_fdw/postgres_fdw.c

@@ -4591,7 +4591,7 @@ static bool
 foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel)
 {
 	Query	   *query = root->parse;
-	PathTarget *grouping_target;
+	PathTarget *grouping_target = root->upper_targets[UPPERREL_GROUP_AGG];
 	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) grouped_rel->fdw_private;
 	PgFdwRelationInfo *ofpinfo;
 	List	   *aggvars;
@@ -4599,7 +4599,7 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel)
 	int			i;
 	List	   *tlist = NIL;
 
-	/* Grouping Sets are not pushable */
+	/* We currently don't support pushing Grouping Sets. */
 	if (query->groupingSets)
 		return false;
 
@@ -4607,7 +4607,7 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel)
 	ofpinfo = (PgFdwRelationInfo *) fpinfo->outerrel->fdw_private;
 
 	/*
-	 * If underneath input relation has any local conditions, those conditions
+	 * If underlying scan relation has any local conditions, those conditions
 	 * are required to be applied before performing aggregation.  Hence the
 	 * aggregate cannot be pushed down.
 	 */
@@ -4615,21 +4615,11 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel)
 		return false;
 
 	/*
-	 * The targetlist expected from this node and the targetlist pushed down
-	 * to the foreign server may be different. The latter requires
-	 * sortgrouprefs to be set to push down GROUP BY clause, but should not
-	 * have those arising from ORDER BY clause. These sortgrouprefs may be
-	 * different from those in the plan's targetlist. Use a copy of path
-	 * target to record the new sortgrouprefs.
-	 */
-	grouping_target = copy_pathtarget(root->upper_targets[UPPERREL_GROUP_AGG]);
-
-	/*
-	 * Evaluate grouping targets and check whether they are safe to push down
-	 * to the foreign side.  All GROUP BY expressions will be part of the
-	 * grouping target and thus there is no need to evaluate it separately.
-	 * While doing so, add required expressions into target list which can
-	 * then be used to pass to foreign server.
+	 * Examine grouping expressions, as well as other expressions we'd need to
+	 * compute, and check whether they are safe to push down to the foreign
+	 * server.  All GROUP BY expressions will be part of the grouping target
+	 * and thus there is no need to search for them separately.  Add grouping
+	 * expressions into target list which will be passed to foreign server.
 	 */
 	i = 0;
 	foreach(lc, grouping_target->exprs)
@@ -4641,51 +4631,59 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel)
 		/* Check whether this expression is part of GROUP BY clause */
 		if (sgref && get_sortgroupref_clause_noerr(sgref, query->groupClause))
 		{
+			TargetEntry *tle;
+
 			/*
-			 * If any of the GROUP BY expression is not shippable we can not
+			 * If any GROUP BY expression is not shippable, then we cannot
 			 * push down aggregation to the foreign server.
 			 */
 			if (!is_foreign_expr(root, grouped_rel, expr))
 				return false;
 
-			/* Pushable, add to tlist */
-			tlist = add_to_flat_tlist(tlist, list_make1(expr));
+			/*
+			 * Pushable, so add to tlist.  We need to create a TLE for this
+			 * expression and apply the sortgroupref to it.  We cannot use
+			 * add_to_flat_tlist() here because that avoids making duplicate
+			 * entries in the tlist.  If there are duplicate entries with
+			 * distinct sortgrouprefs, we have to duplicate that situation in
+			 * the output tlist.
+			 */
+			tle = makeTargetEntry(expr, list_length(tlist) + 1, NULL, false);
+			tle->ressortgroupref = sgref;
+			tlist = lappend(tlist, tle);
 		}
 		else
 		{
-			/* Check entire expression whether it is pushable or not */
+			/*
+			 * Non-grouping expression we need to compute.  Is it shippable?
+			 */
 			if (is_foreign_expr(root, grouped_rel, expr))
 			{
-				/* Pushable, add to tlist */
+				/* Yes, so add to tlist as-is; OK to suppress duplicates */
 				tlist = add_to_flat_tlist(tlist, list_make1(expr));
 			}
 			else
 			{
-				/*
-				 * If we have sortgroupref set, then it means that we have an
-				 * ORDER BY entry pointing to this expression.  Since we are
-				 * not pushing ORDER BY with GROUP BY, clear it.
-				 */
-				if (sgref)
-					grouping_target->sortgrouprefs[i] = 0;
-
-				/* Not matched exactly, pull the var with aggregates then */
+				/* Not pushable as a whole; extract its Vars and aggregates */
 				aggvars = pull_var_clause((Node *) expr,
 										  PVC_INCLUDE_AGGREGATES);
 
+				/*
+				 * If any aggregate expression is not shippable, then we
+				 * cannot push down aggregation to the foreign server.
+				 */
 				if (!is_foreign_expr(root, grouped_rel, (Expr *) aggvars))
 					return false;
 
 				/*
-				 * Add aggregates, if any, into the targetlist.  Plain var
-				 * nodes should be either same as some GROUP BY expression or
-				 * part of some GROUP BY expression. In later case, the query
-				 * cannot refer plain var nodes without the surrounding
-				 * expression.  In both the cases, they are already part of
+				 * Add aggregates, if any, into the targetlist.  Plain Vars
+				 * outside an aggregate can be ignored, because they should be
+				 * either same as some GROUP BY column or part of some GROUP
+				 * BY expression.  In either case, they are already part of
 				 * the targetlist and thus no need to add them again.  In fact
-				 * adding pulled plain var nodes in SELECT clause will cause
-				 * an error on the foreign server if they are not same as some
-				 * GROUP BY expression.
+				 * including plain Vars in the tlist when they do not match a
+				 * GROUP BY column would cause the foreign server to complain
+				 * that the shipped query is invalid.
 				 */
 				foreach(l, aggvars)
 				{
@@ -4701,7 +4699,7 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel)
 	}
 
 	/*
-	 * Classify the pushable and non-pushable having clauses and save them in
+	 * Classify the pushable and non-pushable HAVING clauses and save them in
 	 * remote_conds and local_conds of the grouped rel's fpinfo.
 	 */
 	if (root->hasHavingQual && query->havingQual)
@@ -4771,9 +4769,6 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel)
 		}
 	}
 
-	/* Transfer any sortgroupref data to the replacement tlist */
-	apply_pathtarget_labeling_to_tlist(tlist, grouping_target);
-
 	/* Store generated targetlist */
 	fpinfo->grouped_tlist = tlist;
 

contrib/postgres_fdw/sql/postgres_fdw.sql

@@ -636,6 +636,12 @@ explain (verbose, costs off)
 select count(c2) w, c2 x, 5 y, 7.0 z from ft1 group by 2, y, 9.0::int order by 2;
 select count(c2) w, c2 x, 5 y, 7.0 z from ft1 group by 2, y, 9.0::int order by 2;
 
+-- GROUP BY clause referring to same column multiple times
+-- Also, ORDER BY contains an aggregate function
+explain (verbose, costs off)
+select c2, c2 from ft1 where c2 > 6 group by 1, 2 order by sum(c1);
+select c2, c2 from ft1 where c2 > 6 group by 1, 2 order by sum(c1);
+
 -- Testing HAVING clause shippability
 explain (verbose, costs off)
 select c2, sum(c1) from ft2 group by c2 having avg(c1) < 500 and sum(c1) < 49800 order by c2;