Re: Push down more full joins in postgres_fdw

Thanks Fujita-san for working on this. I took a quick look at the patch.
Here are some quick comments.

1. deparsePlaceHolderVar looks odd - each of the deparse* function is named
as deparse + <name of the parser node the string would parse into>.
PlaceHolderVar is not a parser node, so no string is going to be parsed as
a PlaceHolderVar. May be you want to name it as
deparseExerFromPlaceholderVar or something like that.

2. You will need to check phlevelsup member while assessing whether a PHV
is safe to push down.

3. I think registerAlias stuff should happen really at the time of creating
paths and should be stored in fpinfo. Without that it will be computed
every time we deparse the query. This means every time we try to EXPLAIN
the query at various levels in the join tree and once for the query to be
sent to the foreign server.

4. The changes related to retrieved_attrs look unrelated to the patch.
Either your patch should use the current method of handling retrieved_attrs
or there should be a separate patch for retrieved_attrs changes. May be you
want to take a look at the discussion in join pushdown thread as to why we
assume retrieved_attrs to be non-NIL always.

5. The blocks related to inner and outer relations in
deparseFromExprForRel() look same. We should probably separate that code
out into a function and call it at two places.

6.
! if (is_placeholder)
! errcontext("placeholder expression at position %d in select list",
! errpos->cur_attno);
A user wouldn't know what a placeholder expression is, there is no such
term in the documentation. We have to device a better way to provide an
error context for an expression in general.

On Fri, Aug 19, 2016 at 2:50 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> Hi,
>
> The postgres_fdw join pushdown in 9.6 is great, but it can't handle full
> joins on relations with restrictions. The reason for that is, postgres_fdw
> can't support deparsing subqueries when creating a remote join query. So,
> by adding the deparsing logic to it, I removed that limitation. Attached
> is a patch for that, which is based on the patch I posted before [1].
>
> Also, by the deparsing logic, I improved the handling of PHVs so that PHVs
> are evaluated remotely if it's safe, as discussed in [2]. Here is an
> example from the regression test, which pushes down multiple levels of PHVs
> to the remote:
>
> EXPLAIN (VERBOSE, COSTS OFF)
> SELECT ss.*, ft2.c1 FROM ft2 LEFT JOIN (SELECT 13, q.a, ft2.c1 FROM
> (SELECT 13 FROM ft1 WHERE c1 = 13) q(a) RIGHT JOIN ft2 ON (q.a = ft2.c1)
> WHERE ft2.\
> c1 BETWEEN 10 AND 15) ss(f1, f2, f3) ON (ft2.c1 = ss.f1) WHERE ft2.c1
> BETWEEN 10 AND 15;
> \
> QUERY PLAN \
>
> ------------------------------------------------------------
> ------------------------------------------------------------
> -------------------------------\
> ------------------------------------------------------------
> ------------------------------------------------------------
> -------------------------------\
> ---------------------------------------------------------------
> Foreign Scan
> Output: (13), (13), ft2_1.c1, ft2.c1
> Relations: (public.ft2) LEFT JOIN ((public.ft2) LEFT JOIN (public.ft1))
> Remote SQL: SELECT r1."C 1", ss2.c1, ss2.c2, ss2.c3 FROM ("S 1"."T 1"
> r1 LEFT JOIN (SELECT r5."C 1", 13, ss1.c1 FROM ("S 1"."T 1" r5 LEFT JOIN
> (SELE\
> CT 13 FROM "S 1"."T 1" WHERE (("C 1" = 13))) ss1(c1) ON (((13 = r5."C
> 1")))) WHERE ((r5."C 1" >= 10)) AND ((r5."C 1" <= 15))) ss2(c1, c2, c3) ON
> (((r1.\
> "C 1" = 13)))) WHERE ((r1."C 1" >= 10)) AND ((r1."C 1" <= 15))
> (4 rows)
>
> Also, in the same way as the PHV handling, I improved the handling of
> whole-row reference (and system columns other than ctid), as proposed in
> [3]. We don't need the ""CASE WHEN ... IS NOT NULL THEN ROW(...) END"
> conditions any more, as shown in the below example:
>
> EXPLAIN (VERBOSE, COSTS OFF)
> SELECT t1.ctid, t1, t2, t1.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)
> ORDER BY t1.c3, t1.c1 OFFSET 100 LIMIT 10;
> \
> QUERY PLAN \
>
> ------------------------------------------------------------
> ------------------------------------------------------------
> -------------------------------\
> ------------------------------------------------------------
> ------------------------------------------------------------
> -------------------------------\
> -----------------------------------------------------------
> Limit
> Output: t1.ctid, t1.*, t2.*, t1.c1, t1.c3
> -> Foreign Scan
> Output: t1.ctid, t1.*, t2.*, t1.c1, t1.c3
> Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
> Remote SQL: SELECT ss1.c1, ss1.c2, ss1.c3, ss1.c4, ss2.c1 FROM
> ((SELECT ctid, ROW("C 1", c2, c3, c4, c5, c6, c7, c8), "C 1", c3 FROM "S
> 1"."T \
> 1") ss1(c1, c2, c3, c4) INNER JOIN (SELECT ROW("C 1", c2, c3, c4, c5, c6,
> c7, c8), "C 1" FROM "S 1"."T 1") ss2(c1, c2) ON (TRUE)) WHERE ((ss1.c3 =
> ss2.\
> c2)) ORDER BY ss1.c4 ASC NULLS LAST, ss1.c3 ASC NULLS LAST
> (6 rows)
>
> I'll add this to the next CF. Comments are welcome!
>
> Best regards,
> Etsuro Fujita
>
> [1] https://www.postgresql.org/message-id/5710D7E2.7010302%40lab.ntt.co.jp
> [2] https://www.postgresql.org/message-id/b4549406-909f-7d15-dc3
> 4-499835a8f0b3%40lab.ntt.co.jp
> [3] https://www.postgresql.org/message-id/a1fa1c4c-bf96-8ea5-cff
> 5-85b927298e73%40lab.ntt.co.jp
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company