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

Hi KaiGai-san,

Thanks for the review. Attached is the v8 patch of foreign join support for postgres_fdw.

In addition to your comments, I removed useless code that retrieves ForeignPath from outer/inner RelOptInfo and store them into ForeignPath#fdw_private. Now postgres_fdw’s join pushd-down is free from existence of ForeignPath under the join relation. This means that we can support the case Robert mentioned before, that whole "(huge JOIN large) JOIN small” can be pushed down even if “(huge JOIN large)” is dominated by another join path.

2015-04-07 13:46 GMT+09:00 Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> 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.

I too found the bug. As you suggested, deparseProjectionSql() is the place to fix.

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

Simple relation reference such as "l" is not sufficient for the purpose, yes. But putting columns in parentheses would not work when a user column is referenced in original query.

I implemented deparseProjectionSql to use ROW(...) expression for a whole-row reference in the target list, in addition to ordinary column references for actually used columns and ctid.

Please see the test case for mixed use of ctid and whole-row reference to postgres_fdw’s regression tests. Now a whole-row reference in the remote query looks like this:

-- ctid with whole-row reference
EXPLAIN (COSTS false, VERBOSE)
SELECT t1.ctid, t1, t2 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t1.c1 OFFSET 100 LIMIT 10;

---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Limit
Output: t1.ctid, t1.*, t2.*, t1.c3, t1.c1
-> Sort
Output: t1.ctid, t1.*, t2.*, t1.c3, t1.c1
Sort Key: t1.c3, t1.c1
-> Foreign Scan
Output: t1.ctid, t1.*, t2.*, t1.c3, t1.c1
Remote SQL: SELECT l.a1, l.a2, l.a3, l.a4, r.a1 FROM (SELECT l.a7, ROW(l.a10, l.a11, l.a12, l.a13, l.a14, l.a15, l.a16, l.a17), l.a12, l.a10 FROM (SELECT "C 1" a10, c2 a11, c3 a12, c4 a13, c5 a14, c6 a15, c7 a1
(8 rows)

In fact l.a12 and l.a10, for t1.c3 and t1.c1, are redundant in transferred data, but IMO this would simplify the code for deparsing.

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

Exactly. I fixed the code not to loop around.

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

Oops. They are vestige of my struggle which disabled SELECT clause optimization (omit unused columns). Now width and rows are inherited from joinrel. Besides that, fdw_startup_cost and fdw_tuple_cost seem wrong, so I fixed them to use simple summary, not average.

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

Like this?

Foreign Scan on ft1 INNER JOIN ft2 ON ft1.a = ft2.b (cost=200.00..212.80 rows=1280 width=80)
…

It might produce a very long line in a case of joining many tables because it contains most of remote query other than SELECT clause, but I prefer detailed. Another idea is to print “Join Cond” and “Remote Filter” as separated EXPLAIN items.

Note that v8 patch doesn’t contain this change yet!

--
Shigeru HANADA