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

Hanada-san,

> Thanks for further review, but I found two bugs in v10 patch.
> I’ve fixed them and wrapped up v11 patch here.
>
> * Fix bug about illegal column order
>
> Scan against a base relation returns columns in order of column definition, but
> its target list might require different order. This can be resolved by tuple
> projection in usual cases, but pushing down joins skips the step, so we need to
> treat it in remote query.
>
> Before this fix, deparseProjectionSql() was called only for queries which have
> ctid or whole-row reference in its target list, but it was a too-much optimization.
> We always need to call it, because usual column list might require ordering
> conversion. Checking ordering is not impossible, but it seems useless effort.
>
> Another way to resolve this issue is to reorder SELECT clause of a query for base
> relation if it was a source of a join, but it requires stepping back in planning,
> so the fix above was chosen.
>
> "three tables join" test case is also changed to check this behavior.
>
Sorry for my oversight. Yep, var-node reference on join relation cannot
expect any column orders predefined like as base relations.
All reasonable way I know is, relying on the targetlist of the RelOptInfo
that contains all the referenced columns in the later stage.

> * Fix bug of duplicate fdw_ps_tlist contents.
>
> I coded that deparseSelectSql passes fdw_ps_tlist to deparseSelectSql for
> underlying RelOptInfo, but it causes redundant entries in fdw_ps_tlist in cases
> of joining more than two foreign tables. I changed to pass NULL as fdw_ps_tlist
> for recursive call of deparseSelectSql.
>
It's reasonable, and also makes performance benefit because descriptor
constructed based on the ps_tlist will match expected result tuple, so
it allows to avoid unnecessary projection.

> * 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.

> 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.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>