On Wed, Dec 7, 2016 at 4:51 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > On 2016/12/05 20:20, Ashutosh Bapat wrote: >> >> On Mon, Nov 28, 2016 at 3:52 PM, Etsuro Fujita >> <fujita.ets...@lab.ntt.co.jp> wrote: >>> >>> On 2016/11/24 21:45, Etsuro Fujita wrote: >>>> >>>> * I removed make_outerrel_subquery/make_innerrel_subquery from fpinfo >>>> and added a new member "is_subquery_rel" to fpinfo, as a flag on whether >>>> to deparse the relation as a subquery. > > >> Replacing make_outerrel_subquery/make_innerrel_subquery with >> is_subquery_rel >> seems to be a bad idea. Whether a relation needs to be deparsed as a >> subquery >> or not depends upon the relation which joins it with another relation. >> Take for >> example a case when a join ABC can pull up the conditions on AB, but ABD >> can >> not. In this case we will mark that AB in ABD needs to be deparsed as a >> subquery but will not mark so in ABC. So, if we choose to join ABCD as >> (ABC)D >> we don't need AB to be deparsed as a subquery. If we choose to join ABCD >> as >> (ABD)C, we will need AB to deparsed as a subquery. > > > This is not true; since (1) currently, a relation needs to be deparsed as a > subquery only when the relation is full-joined with any other relation, and > (2) the join ordering for full joins is preserved, we wouldn't have such an > example. (You might think we would have that when extending the patch to > handle PHVs, but the answer would be no, because the patch for PHVs would > determine whether a relation emitting PHVs needs to be deparsed as a > subquery, depending on the relation itself. See the patch for PHVs.) I like > is_subquery_rel more than make_outerrel_subquery/make_innerrel_subquery, > because it reduces the number of members in fpinfo. But the choice would be > arbitrary, so I wouldn't object to going back to > make_outerrel_subquery/make_innerrel_subquery.
I think you are right. And even if we were to deparse a relation as subquery, when it shouldn't be, we will only loose a syntactic optimization. So, it shouldn't result in a bug. >> 3. Why is foreignrel variable changed to rel? >> -extern void deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root, >> - RelOptInfo *foreignrel, List *tlist, >> - List *remote_conds, List *pathkeys, >> - List **retrieved_attrs, List **params_list); >> +extern void deparseSelectStmtForRel(StringInfo buf, PlannerInfo >> *root, RelOptInfo *rel, >> + List *tlist, List *remote_conds, List *pathkeys, >> + bool is_subquery, List **retrieved_attrs, >> + List **params_list); > > > I changed the name that way to match with the function definition in > deparse.c. > That's something good to do but it's unrelated to this patch. I have repeated this line several times in this thread :) -- 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
