diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index c5149a0..1d5aa83 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -210,7 +210,7 @@ classifyConditions(PlannerInfo *root, foreach(lc, input_conds) { - RestrictInfo *ri = (RestrictInfo *) lfirst(lc); + RestrictInfo *ri = lfirst_node(RestrictInfo, lc); if (is_foreign_expr(root, baserel, ri->clause)) *remote_conds = lappend(*remote_conds, ri); @@ -869,6 +869,7 @@ build_tlist_to_deparse(RelOptInfo *foreignrel) { List *tlist = NIL; PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private; + ListCell *lc; /* * For an upper relation, we have already built the target list while @@ -884,9 +885,14 @@ build_tlist_to_deparse(RelOptInfo *foreignrel) tlist = add_to_flat_tlist(tlist, pull_var_clause((Node *) foreignrel->reltarget->exprs, PVC_RECURSE_PLACEHOLDERS)); - tlist = add_to_flat_tlist(tlist, - pull_var_clause((Node *) fpinfo->local_conds, - PVC_RECURSE_PLACEHOLDERS)); + foreach(lc, fpinfo->local_conds) + { + RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc); + + tlist = add_to_flat_tlist(tlist, + pull_var_clause((Node *) rinfo->clause, + PVC_RECURSE_PLACEHOLDERS)); + } return tlist; } @@ -1049,6 +1055,7 @@ deparseSelectSql(List *tlist, bool is_subquery, List **retrieved_attrs, * "buf". * * quals is the list of clauses to be included in the WHERE clause. + * (These may or may not include RestrictInfo decoration.) */ static void deparseFromExpr(List *quals, deparse_expr_cxt *context) @@ -1262,6 +1269,9 @@ deparseLockingClause(deparse_expr_cxt *context) * * The conditions in the list are assumed to be ANDed. This function is used to * deparse WHERE clauses, JOIN .. ON clauses and HAVING clauses. + * + * Depending on the caller, the list elements might be either RestrictInfos + * or bare clauses. */ static void appendConditions(List *exprs, deparse_expr_cxt *context) @@ -1278,16 +1288,9 @@ appendConditions(List *exprs, deparse_expr_cxt *context) { Expr *expr = (Expr *) lfirst(lc); - /* - * Extract clause from RestrictInfo, if required. See comments in - * declaration of PgFdwRelationInfo for details. - */ + /* Extract clause from RestrictInfo, if required */ if (IsA(expr, RestrictInfo)) - { - RestrictInfo *ri = (RestrictInfo *) expr; - - expr = ri->clause; - } + expr = ((RestrictInfo *) expr)->clause; /* Connect expressions with "AND" and parenthesize each condition. */ if (!is_first) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 6670df5..c520852 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -64,7 +64,8 @@ enum FdwScanPrivateIndex { /* SQL statement to execute remotely (as a String node) */ FdwScanPrivateSelectSql, - /* List of restriction clauses that can be executed remotely */ + /* List of qual clauses that can be executed remotely */ + /* (DO NOT try to use these at runtime; see postgresGetForeignPlan) */ FdwScanPrivateRemoteConds, /* Integer list of attribute numbers retrieved by the SELECT */ FdwScanPrivateRetrievedAttrs, @@ -576,7 +577,7 @@ postgresGetForeignRelSize(PlannerInfo *root, &fpinfo->attrs_used); foreach(lc, fpinfo->local_conds) { - RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc); + RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc); pull_varattnos((Node *) rinfo->clause, baserel->relid, &fpinfo->attrs_used); @@ -1119,14 +1120,14 @@ postgresGetForeignPlan(PlannerInfo *root, PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private; Index scan_relid; List *fdw_private; - List *remote_conds = NIL; List *remote_exprs = NIL; List *local_exprs = NIL; List *params_list = NIL; + List *fdw_scan_tlist = NIL; + List *fdw_recheck_quals = NIL; List *retrieved_attrs; StringInfoData sql; ListCell *lc; - List *fdw_scan_tlist = NIL; /* * For base relations, set scan_relid as the relid of the relation. For @@ -1163,9 +1164,6 @@ postgresGetForeignPlan(PlannerInfo *root, * * This code must match "extract_actual_clauses(scan_clauses, false)" * except for the additional decision about remote versus local execution. - * Note however that we don't strip the RestrictInfo nodes from the - * remote_conds list, since appendWhereClause expects a list of - * RestrictInfos. */ foreach(lc, scan_clauses) { @@ -1176,26 +1174,27 @@ postgresGetForeignPlan(PlannerInfo *root, continue; if (list_member_ptr(fpinfo->remote_conds, rinfo)) - { - remote_conds = lappend(remote_conds, rinfo); remote_exprs = lappend(remote_exprs, rinfo->clause); - } else if (list_member_ptr(fpinfo->local_conds, rinfo)) local_exprs = lappend(local_exprs, rinfo->clause); else if (is_foreign_expr(root, foreignrel, rinfo->clause)) - { - remote_conds = lappend(remote_conds, rinfo); remote_exprs = lappend(remote_exprs, rinfo->clause); - } else local_exprs = lappend(local_exprs, rinfo->clause); } if (IS_JOIN_REL(foreignrel) || IS_UPPER_REL(foreignrel)) { - /* For a join relation, get the conditions from fdw_private structure */ - remote_conds = fpinfo->remote_conds; - local_exprs = fpinfo->local_conds; + + /* + * For a join or upper relation, get the conditions from fdw_private + * structure. Note that we leave fdw_recheck_quals empty in this case, + * since there is no need for EPQ recheck for relations other than base + * relations. See postgresGetForeignJoinPaths() for more details on how + * joins implement EPQ. + */ + remote_exprs = extract_actual_clauses(fpinfo->remote_conds, false); + local_exprs = extract_actual_clauses(fpinfo->local_conds, false); /* Build the list of columns to be fetched from the foreign server. */ fdw_scan_tlist = build_tlist_to_deparse(foreignrel); @@ -1238,6 +1237,14 @@ postgresGetForeignPlan(PlannerInfo *root, } } } + else + { + /* + * For a base-relation scan, we have to support EPQ recheck, which + * should recheck all remote quals + */ + fdw_recheck_quals = remote_exprs; + } /* * Build the query string to be sent for execution, and identify @@ -1245,7 +1252,7 @@ postgresGetForeignPlan(PlannerInfo *root, */ initStringInfo(&sql); deparseSelectStmtForRel(&sql, root, foreignrel, fdw_scan_tlist, - remote_conds, best_path->path.pathkeys, + remote_exprs, best_path->path.pathkeys, false, &retrieved_attrs, ¶ms_list); /* @@ -1253,7 +1260,7 @@ postgresGetForeignPlan(PlannerInfo *root, * Items in the list must match order in enum FdwScanPrivateIndex. */ fdw_private = list_make4(makeString(sql.data), - remote_conds, + remote_exprs, retrieved_attrs, makeInteger(fpinfo->fetch_size)); if (IS_JOIN_REL(foreignrel) || IS_UPPER_REL(foreignrel)) @@ -1266,6 +1273,13 @@ postgresGetForeignPlan(PlannerInfo *root, * Note that the remote parameter expressions are stored in the fdw_exprs * field of the finished plan node; we can't keep them in private state * because then they wouldn't be subject to later planner processing. + * + * We have some foreign qual conditions hidden away within fdw_private's + * FdwScanPrivateRemoteConds item, which would be unsafe per the above + * consideration. But those will only be used by postgresPlanDirectModify, + * which may extract them to use in a rewritten plan. We assume that + * nothing will be done between here and there that would need to modify + * those expressions. */ return make_foreignscan(tlist, local_exprs, @@ -1273,7 +1287,7 @@ postgresGetForeignPlan(PlannerInfo *root, params_list, fdw_private, fdw_scan_tlist, - remote_exprs, + fdw_recheck_quals, outer_plan); } @@ -2199,7 +2213,9 @@ postgresPlanDirectModify(PlannerInfo *root, rel = heap_open(rte->relid, NoLock); /* - * Extract the baserestrictinfo clauses that can be evaluated remotely. + * Extract the qual clauses that can be evaluated remotely. (These are + * bare clauses not RestrictInfos, but deparse.c's appendConditions() + * doesn't care.) */ remote_conds = (List *) list_nth(fscan->fdw_private, FdwScanPrivateRemoteConds); @@ -4064,7 +4080,6 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype, PgFdwRelationInfo *fpinfo_i; ListCell *lc; List *joinclauses; - List *otherclauses; /* * We support pushing down INNER, LEFT, RIGHT and FULL OUTER joins. @@ -4094,29 +4109,48 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype, if (fpinfo_o->local_conds || fpinfo_i->local_conds) return false; - /* Separate restrict list into join quals and quals on join relation */ - if (IS_OUTER_JOIN(jointype)) - extract_actual_join_clauses(extra->restrictlist, &joinclauses, &otherclauses); - else - { - /* - * Unlike an outer join, for inner join, the join result contains only - * the rows which satisfy join clauses, similar to the other clause. - * Hence all clauses can be treated as other quals. This helps to push - * a join down to the foreign server even if some of its join quals - * are not safe to pushdown. - */ - otherclauses = extract_actual_clauses(extra->restrictlist, false); - joinclauses = NIL; - } - - /* Join quals must be safe to push down. */ - foreach(lc, joinclauses) + /* + * Separate restrict list into join quals and pushed-down (other) quals. + * + * Other clauses are applied after the join has been performed and thus + * need not be all pushable. We will push those which can be pushed to + * reduce the number of rows fetched from the foreign server. Rest of them + * will be applied locally after fetching join result. Add them to fpinfo + * so that other joins involving this joinrel will know that this joinrel + * has local clauses. + * + * Unlike an outer join, for an inner join, the join result contains only + * the rows which satisfy join clauses, similar to the other clause. Hence + * all clauses can be treated as other quals. This helps to push a join + * down to the foreign server even if some of its join quals are not safe + * to pushdown. + */ + joinclauses = NIL; + foreach(lc, extra->restrictlist) { - Expr *expr = (Expr *) lfirst(lc); + RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc); + bool is_remote_clause = is_foreign_expr(root, joinrel, + rinfo->clause); - if (!is_foreign_expr(root, joinrel, expr)) - return false; + if (!IS_OUTER_JOIN(jointype) || rinfo->is_pushed_down) + { + if (is_remote_clause) + fpinfo->remote_conds = lappend(fpinfo->remote_conds, rinfo); + else + fpinfo->local_conds = lappend(fpinfo->local_conds, rinfo); + } + else + { + /* + * Join quals must be safe to push down. We might have some clause + * in local_conds and remote_conds list, but those will not be used + * if we deem the join as unshippable. So it's fine to return from + * here with incomplete lists. + */ + if (!is_remote_clause) + return false; + joinclauses = lappend(joinclauses, rinfo); + } } /* @@ -4142,24 +4176,6 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype, /* Save the join clauses, for later use. */ fpinfo->joinclauses = joinclauses; - /* - * Other clauses are applied after the join has been performed and thus - * need not be all pushable. We will push those which can be pushed to - * reduce the number of rows fetched from the foreign server. Rest of them - * will be applied locally after fetching join result. Add them to fpinfo - * so that other joins involving this joinrel will know that this joinrel - * has local clauses. - */ - foreach(lc, otherclauses) - { - Expr *expr = (Expr *) lfirst(lc); - - if (!is_foreign_expr(root, joinrel, expr)) - fpinfo->local_conds = lappend(fpinfo->local_conds, expr); - else - fpinfo->remote_conds = lappend(fpinfo->remote_conds, expr); - } - fpinfo->outerrel = outerrel; fpinfo->innerrel = innerrel; fpinfo->jointype = jointype; @@ -4631,11 +4647,25 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel) foreach(lc, (List *) query->havingQual) { Expr *expr = (Expr *) lfirst(lc); + RestrictInfo *rinfo; - if (!is_foreign_expr(root, grouped_rel, expr)) - fpinfo->local_conds = lappend(fpinfo->local_conds, expr); + /* + * Currently, the core code doesn't wrap havingQuals in + * RestrictInfos, so we must make our own. + */ + Assert(!IsA(expr, RestrictInfo)); + rinfo = make_restrictinfo(expr, + true, + false, + false, + root->qual_security_level, + grouped_rel->relids, + NULL, + NULL); + if (is_foreign_expr(root, grouped_rel, expr)) + fpinfo->remote_conds = lappend(fpinfo->remote_conds, rinfo); else - fpinfo->remote_conds = lappend(fpinfo->remote_conds, expr); + fpinfo->local_conds = lappend(fpinfo->local_conds, rinfo); } } @@ -4645,9 +4675,17 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel) */ if (fpinfo->local_conds) { + List *aggvars = NIL; ListCell *lc; - List *aggvars = pull_var_clause((Node *) fpinfo->local_conds, - PVC_INCLUDE_AGGREGATES); + + foreach(lc, fpinfo->local_conds) + { + RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc); + + aggvars = list_concat(aggvars, + pull_var_clause((Node *) rinfo->clause, + PVC_INCLUDE_AGGREGATES)); + } foreach(lc, aggvars) { @@ -4664,7 +4702,7 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel) if (!is_foreign_expr(root, grouped_rel, expr)) return false; - tlist = add_to_flat_tlist(tlist, aggvars); + tlist = add_to_flat_tlist(tlist, list_make1(expr)); } } } diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h index 57dbb79..df75518 100644 --- a/contrib/postgres_fdw/postgres_fdw.h +++ b/contrib/postgres_fdw/postgres_fdw.h @@ -34,16 +34,8 @@ typedef struct PgFdwRelationInfo /* * Restriction clauses, divided into safe and unsafe to pushdown subsets. - * - * For a base foreign relation this is a list of clauses along-with - * RestrictInfo wrapper. Keeping RestrictInfo wrapper helps while dividing - * scan_clauses in postgresGetForeignPlan into safe and unsafe subsets. - * Also it helps in estimating costs since RestrictInfo caches the - * selectivity and qual cost for the clause in it. - * - * For a join relation, however, they are part of otherclause list - * obtained from extract_actual_join_clauses, which strips RestrictInfo - * construct. So, for a join relation they are list of bare clauses. + * All entries in these lists should have RestrictInfo wrappers; that + * improves efficiency of selectivity and cost estimation. */ List *remote_conds; List *local_conds; @@ -91,7 +83,7 @@ typedef struct PgFdwRelationInfo RelOptInfo *outerrel; RelOptInfo *innerrel; JoinType jointype; - List *joinclauses; + List *joinclauses; /* List of RestrictInfo */ /* Grouping information */ List *grouped_tlist;