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
