Re: Join push-down support for foreign tables

Thanks for the detailed comments.

2015-03-03 18:01 GMT+09:00 Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> Hanada-san,
>
> I checked the patch, below is the random comments from my side.
>
> * Context variables
> -------------------
> Sorry, I might give you a wrong suggestion.
> The foreign_glob_cxt and deparse_expr_cxt were re-defined as follows:
>
> typedef struct foreign_glob_cxt
> {
> PlannerInfo *root; /* global planner state */
> - RelOptInfo *foreignrel; /* the foreign relation we are planning
> + RelOptInfo *outerrel; /* the foreign relation, or outer child
> + RelOptInfo *innerrel; /* inner child, only set for join */
> } foreign_glob_cxt;
>
> /*
> @@ -86,9 +89,12 @@ typedef struct foreign_loc_cxt
> typedef struct deparse_expr_cxt
> {
> PlannerInfo *root; /* global planner state */
> - RelOptInfo *foreignrel; /* the foreign relation we are planning
> + RelOptInfo *outerrel; /* the foreign relation, or outer child
> + RelOptInfo *innerrel; /* inner child, only set for join */
> StringInfo buf; /* output buffer to append to */
> List **params_list; /* exprs that will become remote Params
> + ForeignScan *outerplan; /* outer child's ForeignScan node */
> + ForeignScan *innerplan; /* inner child's ForeignScan node */
> } deparse_expr_cxt;
>
> However, the outerrel does not need to have double-meaning.
>
> RelOptInfo->reloptkind gives us information whether the target
> relation is base-relation or join-relation.
> So, foreign_expr_walker() can be implemented as follows:
> if (bms_is_member(var->varno, glob_cxt->foreignrel->relids) &&
> var->varlevelsup == 0)
> {
> :
> also, deparseVar() can checks relation type using:
> if (context->foreignrel->reloptkind == RELOPT_JOINREL)
> {
> deparseJoinVar(...);
>
> In addition, what we need in deparse_expr_cxt are target-list of
> outer and inner relation in deparse_expr_cxt.
> How about to add inner_tlist/outer_tlist instead of innerplan and
> outerplan in deparse_expr_cxt?
> The deparseJoinVar() references these fields, but only target list.

Ah, I've totally misunderstood your suggestion. Now I reverted my
changes and use target lists to know whether the var came from either
of the relations.

> * GetForeignJoinPath method of FDW
> ----------------------------------
> It should be portion of the interface patch, so I added these
> enhancement of FDW APIs with documentation updates.
> Please see the newer version of foreign/custom-join interface patch.

Agreed.

> * enable_foreignjoin parameter
> ------------------------------
> I'm uncertain whether we should have this GUC parameter that affects
> to all FDW implementation. Rather than a built-in one, my preference
> is an individual GUC variable defined with DefineCustomBoolVariable(),
> by postgres_fdw.
> Pros: It offers user more flexible configuration.
> Cons: Each FDW has to implement this GUC by itself?

Hum...
In a sense, I added this GUC parameter for debugging purpose. As you
pointed out, users might want to control join push-down feature
per-FDW. I'd like to hear others' opinion.

> * syntax violated query if empty targetlist
> -------------------------------------------
> At least NULL shall be injected if no columns are referenced.
> Also, add a dummy entry to fdw_ps_tlist to fit slot tuple descriptor.
>
> postgres=# explain verbose select NULL from ft1,ft2 where aid=bid;
> QUERY PLAN
> ---------------------------------------------------------------------------
> Foreign Scan (cost=100.00..129.25 rows=2925 width=0)
> Output: NULL::unknown
> Remote SQL: SELECT FROM (SELECT bid, NULL FROM public.t2) l (a_0, a_1) INNER
> JOIN (SELECT aid, NULL FROM public.t1) r (a_0, a_1) ON ((r.a_0 = l.a_0))

Fixed.

> * Bug reported by Thom Brown
> -----------------------------
> # EXPLAIN VERBOSE SELECT NULL FROM (SELECT people.id FROM people INNER JOIN countries ON people.country_id = countries.id LIMIT 3) x;
> ERROR: could not open relation with OID 0
>
> Sorry, it was a problem caused by my portion. The patched setrefs.c
> checks fdw_/custom_ps_tlist to determine whether Foreign/CustomScan
> node is associated with a certain base relation. If *_ps_tlist is
> valid, it also expects scanrelid == 0.
> However, things we should check is incorrect. We may have a case
> with empty *_ps_tlist if remote join expects no columns.
> So, I adjusted the condition to check scanrelid instead.

Is this issue fixed by v5 custom/foreign join patch?

> * make_tuple_from_result_row() call
> ------------------------------------
> The 4th argument (newly added) is referenced without NULL checks,
> however, store_returning_result() and analyze_row_processor() put
> NULL on this argument, then it leads segmentation fault.
> RelationGetDescr(fmstate->rel or astate->rel) should be given on
> the caller.

Fixed.

>
> * regression test crash
> ------------------------
> The query below gets crashed:
> UPDATE ft2 SET c2 = ft2.c2 + 500, c3 = ft2.c3 || '_update9', c7 = DEFAULT
> FROM ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 9;
>
> According to the crash dump, tidin() got a cstring input with
> unexpected format. I guess column number is incorrectly assigned,
> but have no clear scenario at this moment.
>
> #0 0x00007f9b45a11513 in conversion_error_callback (arg=0x7fffc257ecc0)
> at postgres_fdw.c:3293
> #1 0x00000000008e51d6 in errfinish (dummy=0) at elog.c:436
> #2 0x00000000008935cd in tidin (fcinfo=0x7fffc257e8a0) at tid.c:69
> /home/kaigai/repo/sepgsql/src/backend/utils/adt/tid.c:69:1734:beg:0x8935cd
> (gdb) p str
> $6 = 0x1d17cf7 "foo"

Join push-down underlying UPDATE or DELETE requires ctid as its
output, but it seems not fully supported. I'm fixing this issue now.

Regards,
--
Shigeru HANADA