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

Hanada-san,

Thanks for your dedicated efforts for remote-join feature.
Below are the comments from my side.

* Bug - mixture of ctid system column and whole row-reference
In case when "ctid" system column is required, deparseSelectSql()
adds ctid reference on the base relation scan level.
On the other hands, whole-row reference is transformed to
a reference to the underlying relation. It will work fine if
system column is not specified. However, system column reference
breaks tuple layout from the expected one.
Eventually it leads an error.

postgres=# select ft1.ctid,ft1 from ft1,ft2 where a=b;
ERROR: malformed record literal: "(2,2,bbb,"(0,2)")"
DETAIL: Too many columns.
CONTEXT: column "" of foreign table "foreign join"
STATEMENT: select ft1.ctid,ft1 from ft1,ft2 where a=b;

postgres=# explain verbose
select ft1.ctid,ft1 from ft1,ft2 where a=b;
QUERY PLAN
--------------------------------------------------------------------------------
Foreign Scan (cost=200.00..208.35 rows=835 width=70)
Output: ft1.ctid, ft1.*
Remote SQL: SELECT l.a1, l.a2 FROM (SELECT l.a7, l, l.a10 FROM (SELECT id a9,
a a10, atext a11, ctid a7 FROM public.t1) l) l (a1, a2, a3) INNER JOIN (SELECT
b a10 FROM public.t2) r (a1) ON ((l.a3 = r.a1))

"l" of the first SELECT represents a whole-row reference.
However, underlying SELECT contains system columns in its target-
list.

Is it available to construct such a query?
SELECT l.a1, l.a2 FROM (SELECT (id,a,atext), ctid) l (a1, a2) ...
^^^^^^^^^^
Probably, it is a job of deparseProjectionSql().

* postgresGetForeignJoinPaths()
It walks on the root->simple_rel_array to check whether
all the relations involved are manged by same foreign
server with same credential.
We may have more graceful way for this. Pay attention on
the fdw_private field of innerrel/outerrel. If they have
a valid fdw_private, it means FDW driver (postgres_fdw)
considers all the underlying relations scan/join are
available to run the remote-server.
So, all we need to check is whether server-id and user-id
of both relations are identical or not.

* merge_fpinfo()
It seems to me fpinfo->rows should be joinrel->rows, and
fpinfo->width also should be joinrel->width.
No need to have special intelligence here, isn't it?

* explain output

EXPLAIN output may be a bit insufficient to know what does it
actually try to do.

postgres=# explain select * from ft1,ft2 where a = b;
QUERY PLAN
--------------------------------------------------------
Foreign Scan (cost=200.00..212.80 rows=1280 width=80)
(1 row)

Even though it is not an immediate request, it seems to me
better to show users joined relations and remote ON/WHERE
clause here.

Please don't hesitate to consult me, if you have any questions.

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: Friday, April 03, 2015 7:32 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)
>
> Attached is the patch which adds join push-down support to postgres_fdw (v7).
> It supports SELECT statements with JOIN, but has some more possible enhancements
> (see below). I'd like to share the WIP patch here to get comments about new FDW
> API design provided by KaiGai-san's v11 patch.
>
> To make reviewing easier, I summarized changes against Custom/Foreign join v11
> patch.
>
> Changes for Join push-down support
> ==================================
> - Add FDW API GetForeignJoinPaths(). It generates a ForeignPath which represents
> a scan against pseudo join relation represented by given RelOptInfo.
> - Expand deparsing module to handle multi-relation queries. Steps of deparsing
> a join query:
>
> 1) Optimizer calls postgresGetForeignPaths() for each BASEREL. Here
> postgres_fdw does the same things as before, except adding column aliases in SELECT
> clause.
> 2) Optimizer calls postgresGetForeignJoinPaths() for each JOINREL. Optimizer
> calls once per RelOptInfo with reloptkind == RELOPT_JOINREL, so postgres_fdw
> should consider both A JOIN B and B JOIN A in one call.
>
> postgres_fdw checks whether the join can be pushed down.
>
> a) Both outer and inner relations can be pushed down (NULL in
> RelOptInfo#fdw_private indicates such situation)
> b) Outmost command is a SELECT (this can be relaxed in the future)
> c) Join type is inner or one of outer
> d) Server of all relations in the join are identical
> e) Effective user id for all relations in the join are identical (they might be
> different some were accessed via views)
> f) No local filters (this can be relaxed if inner && non-volatile)
> g) Join conditions doesn't contain any "unsafe" expression
> h) Remote filter doesn't contain any "unsafe" expression
>
> If all criteria passed, postgres_fdw makes ForeignPath for the join and store
> these information in its fdw_private.
>
> a) ForeignPath of outer relation, first non-parameterized one
> b) ForeignPath of outer relation, first non-parameterized one
> c) join type (as integer)
> d) join conditions (as List of Expr)
> e) other conditions (as List of Expr)
>
> As of now the costs of the path is not so accurate, this is a possible enhancement.
>
> 2) Optimizer calls postgresGetForeignPlan() for the cheapest topmost Path. If
> foreign join is the cheapest way to execute the query, optimizer calls
> postgresGetForeignPlan for the topmost path generated by
> postgresGetForeignJoinPaths. As Robert and Tom mentioned in the thread,
> large_table JOIN huge_table might be removed even (large_table JOIN huge_table)
> JOIN small_table is the cheapest in the join level 3, so postgres_fdw can't assume
> that paths in lower level survived planning.
>
> To cope with the situation, I'm trying to avoid calling create_plan_recurse()
> for underlying paths by putting necessary information into PgFdwRelationInfo and
> link it to appropriate RelOptInfo.
>
> Anyway in current version query string is built up from bottom (BASEREL) to upper
> recursively. For a join, unerlying outer/inner query are put into FROM clause
> with wrapping with parenthesis and aliasing. For example:
>
> select * from pgbench_branches b join pgbench_tellers t on t.bid = b.bid;
>
> is transformed to a query like this:
>
> SELECT l.a1, l.a2, l.a3, r.a1, r.a2, r.a3, r.a4 FROM (SELECT bid a9, bbalance
> a10, filler a11 FROM public.pgbench_branches) l (a1, a2, a3) INNER JOIN (SELECT
> tid a9, bid a10, balance a11, filler a12 FROM public.pgbench_tellers) r (a1, a2,
> a3, a4) ON ((l.a1 = r.a2));
>
> As in the remote query, column aliasing uses attnum-based numbering with shifted
> by FirstLowInvalidHeapAttributeNumber to make all attnum positive. For instance,
> this system uses alias "a9" for the first user column. For readability of code
> around this, I introduced TO_RELATEVE() macro which converts absolute attnum
> (-8~) to relative ones (0~). Current deparser can also handle whole-row
> references (attnum == 0) correctly.
>
> 3) Executor calls BeginForeignScan to initialize a scan. Here TupleDesc is taken
> from the slot, not Relation.
>
> Possible enhancement
> ====================
> - Make deparseSelectSql() more general, thus it can handle both simple SELECT
> and join SELECT by calling itself recursively. This would avoid assuming that
> underlying ForeignPath remains in RelOptInfo. (WIP)
> - Move appendConditions() calls into deparse.c, to clarify responsibility of
> modules.
> - more accurate estimation
> - more detailed information for error location (currently "foreign table" is used
> as relation name always)