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

> >> 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 <kaigai(at)ak(dot)jp(dot)nec(dot)com>

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)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(at)postgreSQL(dot)org
> Subject: Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
>
> KaiGai-san,
>
> 2015/04/14 14:04、Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)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(dot)hanada(at)gmail(dot)com
>
>
> --
> 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