On Fri, Jan 13, 2017 at 12:30 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > On 2017/01/12 18:25, Ashutosh Bapat wrote: >> >> On Wed, Jan 11, 2017 at 5:45 PM, Etsuro Fujita >> <fujita.ets...@lab.ntt.co.jp> wrote: > > >>>>> On 2017/01/05 21:11, Ashutosh Bapat wrote: >>>>>> >>>>>> IIUC, for a relation with use_remote_estimates we will deparse the >>>>>> query twice and will build the targetlist twice. > > >>> While working on this, I noticed a weird case. Consider: >>> >>> postgres=# explain verbose select 1 from ft1 left join ft2 on (ft1.a = >>> ft2.a) inner join test on (true); >>> QUERY PLAN >>> >>> ------------------------------------------------------------------------------------------------- >>> Nested Loop (cost=100.00..103.06 rows=1 width=4) >>> Output: 1 >>> -> Foreign Scan (cost=100.00..102.04 rows=1 width=0) >>> Relations: (public.ft1) LEFT JOIN (public.ft2) >>> Remote SQL: SELECT NULL FROM (public.t1 r1 LEFT JOIN public.t2 >>> r2 >>> ON (((r1.a = r2.a)))) >>> -> Seq Scan on public.test (cost=0.00..1.01 rows=1 width=0) >>> Output: test.a, test.b >>> (7 rows) >>> >>> In this case the fpinfo->tlist of the foreign join is NIL, so whether or >>> not >>> the tlist is already built cannot be discriminated by the fpinfo->tlist. >>> We >>> might need another flag to show that the tlist has been built already. >>> Since this is an existing issue and we would need to give careful thought >>> to >>> this, so I'd like to leave this for another patch. > > >> I think in that case, relation's targetlist will also be NIL or >> contain no Var node. It wouldn't be expensive to build it again and >> again. That's better than maintaining a new flag. > > > I think that's ugly. A more clean way I'm thinking is: (1) in > postgresGetForeignJoinPaths(), create a tlist by build_tlist_to_deparse() > and save it in fpinfo->tlist before estimate_path_cost_size() if > use_remote_estimates=true, (2) in estimate_path_cost_size(), use the > fpinfo->tlist if use_remote_estimates=true, and (3) in > postgresGetForeignPlan(), use the fpinfo->tlist as the fdw_scan_tlist if > use_remote_estimates=true, otherwise create a tlist as the fdw_scan_tlist by > build_tlist_to_deparse(), like the attached. > > What do you think about that? > > Another change is: I simplified build_tlist_to_deparse() a bit and put that > in postgres_fdw.c because that is used only in postgres_fdw.c. > > I still think we should discuss this separately because this is an existing > issue and that makes it easy to review the patch. If the attached is the > right way to go, I'll update the join-pushdown patch on top of the patch.
I don't think it's right to assume that the targetlist will be available only when use_remote_estimate is true; for grouping it's certainly not. But I don't see this discussion getting anywhere. I will leave it to the committer's judgement. I think we should pick up your patch on 27th December, update the comment per your mail on 5th Jan. I will review it once and list down the things left to committer's judgement. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
