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

Here are patches rebased on recent commit
cc592c48c58d9c1920f8e2063756dcbcce79e4dd. Renamed original deparseSelectSql
as deparseSelectSqlForBaseRel and added deparseSelectSqlForJoinRel to
construct SELECT and FROM clauses for base and join relations.

pg_fdw_core_v5.patch GetUserMappingById changes
pg_fdw_join_v5.patch: postgres_fdw changes for join pushdown including
suggestions as described below
pg_join_pd_v5.patch: combined patch for ease of testing.

The patches also have following changes along with the changes described in
my last mail.
1. Revised the way targetlists are handled. For a bare base relation the
SELECT clause is deparsed from fpinfo->attrs_used but for a base relation
which is part of join relation, the expected targetlist is passed down to
deparseSelectSqlForBaseRel(). This change removed 75 odd lines in
build_tlist_to_deparse() which were very similar to
deparseTargetListFromAttrsUsed() in the previous patch.

2. Refactored postgresGetForeignJoinPaths to be more readable moving the
code to assess safety of join pushdown into a separate function.

On Sat, Jan 30, 2016 at 7:58 PM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:

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

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