On 2017/01/30 21:05, Ashutosh Bapat wrote:
On Mon, Jan 30, 2017 at 5:00 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
On 2017/01/27 21:25, Etsuro Fujita wrote:
Sorry, I started thinking we went in the wrong direction.  I added to
deparseSelectStmtForRel build_subquery_tlists, which creates a tlist for
each subquery present in a given join tree prior to deparsing a whole
remote query.  But that's nothing but an overhead; we need to create a
tlist for the top-level query because we use it as fdw_scan_tlist, but
not for subqueries, and we need to create retrieved_attrs for the
top-level query while deparsing the targetlist in
deparseExplicitTargetList, but not for subqueries.  So, we should need
some work to avoid such a useless overhead.

I think we can avoid the useless overhead by adding a new function,
deparseSubqueryTargetList, that deparses expressions in the given relation's
reltarget, not the tlist, as a SELECT clause of the subquery representing
the given relation.  That would also allow us to make the 1-to-1
relationship between the subquery output columns and their aliases more
explicit, which was your original comment.  Please find attached the new
version.  (The patch doesn't need the patch to avoid duplicate construction
of the tlist, discussed upthread.)

I have not looked at the patch, but the reason we use a tlist instead
of list of expressions is because fdw_scan_tlist is expected in that

Actually, we wouldn't need that to deparse a remote join query; a list of expressions would be enough.

and we don't want two different representations one to deparse
SELECT and one to interpret results from the foreign server. What you
describe above seems to introduce exactly that hazard.

I agree with you to some extent, BUT:
* I don't think it's a good idea to create a tlist for each base/join relation that is deparsed as a subquery, to just avoid that hazard. As I said above, that's nothing but an overhead. * I think we would need to have two different representations for at least base relations; we use fpinfo->attrs_used to deparse a simple foreign table scan query for a base relation, but when deparsing the base relation as a subquery, we would need to use the list of expressions in the base relation's reltarget, to deparse a SELECT clause of the subquery, because we need to deparse a whole-row reference to the base relation as ROW, not all the user columns expanded, as shown in this extracted from the regression tests in the patch:

+ SELECT t1.c1, ss.a, ss.b FROM (SELECT c1 FROM "S 1"."T 3" WHERE c1 = 50) t1 INNER JOIN (SELECT t2.c1, t3.c1 FROM (SELECT c1 FROM ft4 WHERE c1 between 50 and 60) t2 FULL JOIN (SELECT c1 FROM ft5 WHERE c1 between 50 and 60) t3 ON (t2.c1 = t3.c1) WHERE t2.c1 IS NULL OR t2.c1 IS NOT NULL) ss(a, b) ON (TRUE) ORDER BY t1.c1, ss.a, ss.b FOR UPDATE OF t1; + QUERY PLAN

+ ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+  LockRows
+    Output: "T 3".c1, ft4.c1, ft5.c1, "T 3".ctid, ft4.*, ft5.*
+    ->  Nested Loop
+          Output: "T 3".c1, ft4.c1, ft5.c1, "T 3".ctid, ft4.*, ft5.*
+          ->  Foreign Scan
+                Output: ft4.c1, ft4.*, ft5.c1, ft5.*
+                Relations: (public.ft4) FULL JOIN (public.ft5)
+ Remote SQL: SELECT s8.c1, s8.c2, s9.c1, s9.c2 FROM ((SELECT c1, ROW(c1, c2, c3) FROM "S 1"."T 3" WHERE ((c1 >= 50)) AND ((c1 <= 60))) s8(c1, c2) FULL JOIN (SELECT c1, ROW(c1, c2, c3) FROM "S 1"."T 4" WHERE ((c1 >= 50)) AND ((c1 <= 60))) s9(c1, c2) ON (((s8.c1 = s9.c1)))) WHERE (((s8.c1 IS NULL) OR (s8.c1 IS NOT NULL))) ORDER BY s8.c1 ASC NULLS LAST, s9.c1 ASC NULLS LAST
+                ->  Hash Full Join
+                      Output: ft4.c1, ft4.*, ft5.c1, ft5.*
+                      Hash Cond: (ft4.c1 = ft5.c1)
+                      Filter: ((ft4.c1 IS NULL) OR (ft4.c1 IS NOT NULL))
+                      ->  Foreign Scan on public.ft4
+                            Output: ft4.c1, ft4.*
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3" WHERE ((c1 >= 50)) AND ((c1 <= 60))
+                      ->  Hash
+                            Output: ft5.c1, ft5.*
+                            ->  Foreign Scan on public.ft5
+                                  Output: ft5.c1, ft5.*
+ Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4" WHERE ((c1 >= 50)) AND ((c1 <= 60))
+          ->  Materialize
+                Output: "T 3".c1, "T 3".ctid
+                ->  Seq Scan on "S 1"."T 3"
+                      Output: "T 3".c1, "T 3".ctid
+                      Filter: ("T 3".c1 = 50)
+ (25 rows)

Other changes:
* I went back to make_outerrel_subquery and make_innerrel_subquery, which
are flags to indicate whether to deparse the input relations as subqueries.
is_subquery_rel would work well for handling the cases of full joins with
restrictions on the input relations, but I noticed that that wouldn't work
well when extending to handle the cases where we deparse the input relations
as subqueries to evaluate PHVs remotely.

I had objected to using a single variable instead of two previously
and you had argued against it in [1]. There you had mentioned that PHV
doesn't need two variables, but now you are taking the other stand,
without any apparent reason. Can you please clarify it?

Sorry, I missed some cases. Consider the join {A, B, C} satisfying the identity 3 in optimizer/README, ie,

    (A leftjoin B on (Pab)) leftjoin C on (Pbc)
    = A leftjoin (B leftjoin C on (Pbc)) on (Pab)

where predicate Pbc fails for all-null B rows. Assume that B has a PHV in the relation's reltarget. While considering 2-way joins, foreign_join_ok would determine to deparse B as a subquery and hence set the B's is_subquery_rel to true for the join relation {A, B}. However, if the planner selects the join order {A leftjoin (B leftjoin C on (Pbc)) on (Pab)} as the cheapest path for the join {A, B, C}, we would deparse B as a subquery according to the is_subquery_rel flag while creating the FROM clause entry for the join relation {B, C}. That wouldn't look good. That would be logically correct, though. To avoid this, I'd like to go back to the two variables previously proposed.

Best regards,
Etsuro Fujita

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to