Re: postgres_fdw join pushdown (was Re: Custom/Foreign-Join-APIs)

PFA patches
pg_fdw_core_v4.patch GetUserMappingById changes
pg_fdw_join_v4.patch: postgres_fdw changes for join pushdown including
suggestions as described below
pg_join_pd_v4.patch: combined patch for ease of testing.

On Fri, Jan 29, 2016 at 9:51 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Thu, Jan 28, 2016 at 11:26 AM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> > 2. pg_fdw_join_v3.patch: changes to postgres_fdw - more description below
>
> This patch no longer quite applies because of conflicts with one of
> your other patches that I applied today (cf. commit
> fbe5a3fb73102c2cfec11aaaa4a67943f4474383).
>
>
And then I broke it some more by committing a patch to extract
> deparseLockingClause from postgresGetForeignPlan and move it to
> deparse.c, but that should pretty directly reduce the size of this
> patch. I wonder if there are any other bits of refactoring of that
> sort that we can do in advance of landing the main patch, just to
> simplify review. But I'm not sure there are: this patch removes very
> little existing code; it just adds a ton of new stuff.
>

The patches are rebased. A separate patch to move select statement
deparsing code into a separate function has been submitted in a separate
mail.

>
> I think the names deparseColumnRefForJoinrel and
> deparseColumnRefForBaserel are better than the previous names, but I
> would capitalize the last "r", so "Rel" instead of "rel".

Done.

> But it's
> weird that we have those functions and also just plain old
> deparseColumnRef, which is logically part of
> deparseColumnRefForBaserel but inexplicably remains a separate
> function. I still don't see why you can't just add a bunch of new
> logic to the existing deparseColumnRef, change the last argument to be
> of type deparse_expr_cxt instead of PlannerInfo, and have that one
> function simply handle more cases than it does currently.
>

1. There are existing callers of deparseColumnRef() like deparseInsertSql()
which will need few lines added to create deparse context. 2. These callers
pass relid and attribute number as arguments as against
deparseColumnRefForJoinRel, which needs a Var node as input (to be searched
in inner and outer targetlists. You seemed to object to different signature
of deparseColumnRefForJoinRel and deparseColumnRefForBaseRel. So, I left
that change in this patch. I am fine with that change as well, if 1 and 2
are acceptable.

>
> It seems unlikely to me that postgresGetForeignPlan really needs to
> call list_free_deep(fdw_scan_tlist). Can't we just let memory context
> reset clean that up?
>

Done.

>
> In postgresGetForeignPlan (and I think some other functions), you
> renamed the argument from baserel to foreignrel. But I think it would
> be better to just go with "rel". We do that elsewhere in various
> places, and it seems fine here too. And it's shorter.
>

We are using rel as variable name for Relation variable name and we need
both of them RelOptInfo and Relation atlest in deparseSelectStmtForRel().
So, used a different name foreignrel.

>
> copy_path_for_epq_recheck() and friends are really ugly. Should we
> consider just adding copyObject() support for those node types
> instead?
>

Done. I had pessimistically code to copy the paths deeply, but that's not
required. We need to copy the path in case it gets freed by the planner
while ejecting it. A flat copy suffices.

>
> The error message quality in conversion_error_callback() looks
> unacceptably poor. The column number we're proposing to output will
> be utterly meaningless to the user. It would ideally be desirable to
> output the base table name and the column name from that table.
>

Done. In conversion_error_callback(), fdw_scan_tlist and Estate are used to
obtain the name of the table and column.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company