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@gmail.com> wrote:

On Thu, Jan 28, 2016 at 11:26 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.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