Skip to content

[DNM] Aggregate syntax transform cleanup #25

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: REL_11_CARTO
Choose a base branch
from
Open

[DNM] Aggregate syntax transform cleanup #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
424820c
An experimental commit to try to push down aggregate functions
Aug 2, 2019
83b3ef3
Fix collation safety rules for aggregations
Sep 11, 2019
335f687
Deparse Row Expression as subquery
Sep 10, 2019
6fe4734
Add public to execution environment of postgres_fdw
Sep 16, 2019
125f0fd
Fix collation in other instances (to be reviewed)
Sep 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions contrib/postgres_fdw/connection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
61 changes: 56 additions & 5 deletions contrib/postgres_fdw/deparse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -182,6 +183,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);

Expand Down Expand Up @@ -464,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)
Expand Down Expand Up @@ -512,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)
Expand Down Expand Up @@ -552,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)
Expand Down Expand Up @@ -751,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)
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)
Expand All @@ -775,7 +777,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
Expand Down Expand Up @@ -998,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);
Expand Down Expand Up @@ -1124,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);
Expand All @@ -1134,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;
}
}

/*
Expand Down Expand Up @@ -2354,13 +2390,28 @@ 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));
break;
}
}

static void
deparseRowExpr(RowExpr *node, deparse_expr_cxt *context)
{
StringInfo buf = context->buf;

// Add an arbitrary alias
appendStringInfoString(buf, "myalias");

// Just save the node for later generation of subquery
context->row_expr = node;
}

/*
* Deparse given Var node into context->buf.
*
Expand Down
6 changes: 3 additions & 3 deletions contrib/postgres_fdw/expected/postgres_fdw.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down