Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)

[Kouhei Kaigai]

> >> Attached explain_forein_join.patch adds capability to show join combination
> of
> >> a ForeignScan in EXPLAIN output as an additional item “Relations”.  I 
> >> thought
> >> that using array to list relations is a good way too, but I chose one 
> >> string
> value
> >> because users would like to know order and type of joins too.
> >>
> > A bit different from my expectation... I expected to display name of the 
> > local
> > foreign tables (and its alias), not remote one, because all the local join 
> > logic
> > displays local foreign tables name.
> > Is it easy to adjust isn't it? Probably, all you need to do is, putting a 
> > local
> > relation name on the text buffer (at deparseSelectSql) instead of the 
> > deparsed
> > remote relation.
> 
> Oops, that’s right.  Attached is the revised version.  I chose fully qualified
> name, schema.relname [alias] for the output.  It would waste some cycles 
> during
> planning if that is not for EXPLAIN, but it seems difficult to get a list of 
> name
> of relations in ExplainForeignScan() phase, because planning information has 
> gone
> away at that time.
>
I understand. Private data structure of the postgres_fdw is not designed
to keep tree structure data (like relations join tree), so it seems to me
a straightforward way to implement the feature.

I have a small suggestion. This patch makes deparseSelectSql initialize
the StringInfoData if supplied, however, it usually shall be a task of
function caller, not callee.
In this case, I like to initStringInfo(&relations) next to the line of
initStingInfo(&sql) on the postgresGetForeignPlan. In my sense, it is
a bit strange to pass uninitialized StringInfoData, to get a text form.

@@ -803,7 +806,7 @@ postgresGetForeignPlan(PlannerInfo *root,
     */
    initStringInfo(&sql);
    deparseSelectSql(&sql, root, baserel, fpinfo->attrs_used, remote_conds,
-                    &params_list, &fdw_ps_tlist, &retrieved_attrs);
+                    &params_list, &fdw_ps_tlist, &retrieved_attrs, &relations);

    /*
     * Build the fdw_private list that will be available in the executor.

Also, could you merge the EXPLAIN output feature on the main patch?
I think here is no reason why to split this feature.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kai...@ak.jp.nec.com>


> -----Original Message-----
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Shigeru HANADA
> Sent: Tuesday, April 14, 2015 7:49 PM
> To: Kaigai Kouhei(海外 浩平)
> Cc: Ashutosh Bapat; Robert Haas; Tom Lane; Thom Brown;
> pgsql-hackers@postgreSQL.org
> Subject: Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
> 
> KaiGai-san,
> 
> 2015/04/14 14:04、Kouhei Kaigai <kai...@ak.jp.nec.com> のメール：
> >
> >> * Fix typos
> >>
> >> Please review the v11 patch, and mark it as “ready for committer” if it’s
> ok.
> >>
> > It's OK for me, and wants to be reviewed by other people to get it 
> > committed.
> >
> 
> Thanks!
> 
> >> In addition to essential features, I tried to implement relation listing in
> EXPLAIN
> >> output.
> >>
> >> Attached explain_forein_join.patch adds capability to show join combination
> of
> >> a ForeignScan in EXPLAIN output as an additional item “Relations”.  I 
> >> thought
> >> that using array to list relations is a good way too, but I chose one 
> >> string
> value
> >> because users would like to know order and type of joins too.
> >>
> > A bit different from my expectation... I expected to display name of the 
> > local
> > foreign tables (and its alias), not remote one, because all the local join 
> > logic
> > displays local foreign tables name.
> > Is it easy to adjust isn't it? Probably, all you need to do is, putting a 
> > local
> > relation name on the text buffer (at deparseSelectSql) instead of the 
> > deparsed
> > remote relation.
> 
> Oops, that’s right.  Attached is the revised version.  I chose fully qualified
> name, schema.relname [alias] for the output.  It would waste some cycles 
> during
> planning if that is not for EXPLAIN, but it seems difficult to get a list of 
> name
> of relations in ExplainForeignScan() phase, because planning information has 
> gone
> away at that time.
> 
> 
> 
> --
> Shigeru HANADA
> shigeru.han...@gmail.com
> 
> 
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


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