Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs)

On Sat, May 16, 2015 at 9:04 AM, Shigeru Hanada
<shigeru(dot)hanada(at)gmail(dot)com> wrote:
> d) All relations must have the same effective user id
> This check is done with userid stored in PgFdwRelationInfo, which is
> valid only when underlying relations have the same effective user id.
> Here "effective user id" means id of the user executing the query, or
> the owner of the view when the foreign table is accessed via view.
> Tom suggested that it is necessary to check that user mapping matches
> for security reason, and now I think it's better than checking
> effective user as current postgres_fdw does.

So, should this be a separate patch?

One of my concerns about this patch is that it's got a lot of stuff in
it that isn't obviously related to the patch. Anything that is a
separate change should be separated out into its own patch. Perhaps
you can post a set of patches that apply one on top of the next, with
the changes for each one clearly separated.

> e) Each source relation must not have any local filter
> Evaluating conditions of join source talbe potentially produces
> different result in OUTER join cases. This can be relaxed for the
> cases when the join is INNER and local filters don't contain any
> volatile function/operator, but it is left as future enhancement.

I think this restriction is a sign that you're not really doing this
right. Consider:

(1) SELECT * FROM a LEFT JOIN b ON a.x = b.x AND b.x = 3;
(2) SELECT * FROM a LEFT JOIN b ON a.x = b.x WHERE b.x = 3;

If you push down the scan of b, you can include the b.x = 3 qual in
case (1) but not in case (2). If you push down the join, you can
include the qual in either case, but you must attach it in the same
place where it was before.

> One big change about deparsing base relation is aliasing. This patch
> adds column alias to SELECT clause even original query is a simple
> single table SELECT.
>
> fdw=# EXPLAIN (VERBOSE, COSTS false) SELECT * FROM pgbench_branches b;
> QUERY PLAN
> ------------------------------------------------------------------------------------
> Foreign Scan on public.pgbench_branches b
> Output: bid, bbalance, filler
> Remote SQL: SELECT bid a9, bbalance a10, filler a11 FROM
> public.pgbench_branches
> (3 rows)
>
> As you see, every column has alias in format "a%d" with index value
> derived from pg_attribute.attnum. Index value is attnum + 8, and the
> magic number "8" comes from FirstLowInvalidHeapAttributeNumber for the
> adjustment that makes attribute number of system attributes positive.

Yeah. I'm not sure this is a good idea. The column labels are
pointless at the outermost level.

I'm not sure it isn't a good idea, either, but I have some doubts.

> One thing tricky is "peusdo projection" which is done by
> deparseProjectionSql for whole-row reference. This is done by put the
> query string in FROM subquery and add whole-row reference in outer
> SELECT clause. This is done by ExecProjection in 9.4 and older, but
> we need to do this in postgres_fdw because ExecProjection is not
> called any more.

What commit changed this?

Thanks for your work on this. Although I know progress has been slow,
I think this work is really important to the project.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company