Skip to content

[DNM] Rowexpr fix collations #26

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 13 commits into
base: REL_11_CARTO
Choose a base branch
from
Open

[DNM] Rowexpr fix collations #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 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
c746d2e
Revert "Fix collation safety rules for aggregations"
Oct 2, 2019
7bc49bc
Revert "Fix collation in other instances (to be reviewed)"
Oct 2, 2019
75ccf76
A better implementation of case T_RowExpr
Oct 3, 2019
f3e6de2
A better implementation of RowExpr deparsing
Oct 3, 2019
ee627e0
Cleanup deparseRowExpr function
Oct 4, 2019
3e3ace4
Fix collation of Const: mark as safe when using default.
Oct 4, 2019
9b56fca
Remove elog NOTICE
Oct 4, 2019
3349a38
Remove unused variable
Oct 4, 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
110 changes: 104 additions & 6 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 */
List **row_exprs_list; /* 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 @@ -380,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:
Expand Down Expand Up @@ -775,7 +782,37 @@ foreign_expr_walker(Node *node,
state = FDW_COLLATE_UNSAFE;
}
break;
default:
case T_RowExpr:
{
RowExpr *re = (RowExpr *) node;

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;

/*
* 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.
*
* Bubble up collation state from args, just like for lists.
*/
collation = inner_cxt.collation;
state = inner_cxt.state;
}
break;
default:

/*
* If it's anything else, assume it's unsafe. This list can be
Expand Down Expand Up @@ -985,6 +1022,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
Expand All @@ -998,6 +1036,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_exprs_list = &row_exprs_list;

/* Construct SELECT clause */
deparseSelectSql(tlist, is_subquery, retrieved_attrs, &context);
Expand Down Expand Up @@ -1117,13 +1156,40 @@ 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) ||
IS_JOIN_REL(scanrel) || IS_SIMPLE_REL(scanrel));

/* Construct FROM clause */
appendStringInfoString(buf, " FROM ");

// We have one or more row expressions. Add the corresponding subqueries
if (*row_exprs_list != NIL)
{
ListCell *lcre;
foreach(lcre, *row_exprs_list)
{
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 ");

}
}

deparseFromExprForRel(buf, context->root, scanrel,
(bms_num_members(scanrel->relids) > 1),
(Index) 0, NULL, context->params_list);
Expand All @@ -1134,6 +1200,20 @@ deparseFromExpr(List *quals, deparse_expr_cxt *context)
appendStringInfoString(buf, " WHERE ");
appendConditions(quals, context);
}

// Close subquery and add an alias
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--;
}
}
}

/*
Expand Down Expand Up @@ -2354,13 +2434,31 @@ 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;
List **row_exprs_list = context->row_exprs_list;

// Just save the node for later generation of subquery
*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));
}

/*
* 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