Re: Join push-down support for foreign tables

Thanks for the detailed comments.

2015-03-03 18:01 GMT+09:00 Kouhei Kaigai <kaigai@ak.jp.nec.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.idLIMIT 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