On Thu, Jan 28, 2016 at 11:26 AM, Ashutosh Bapat <ashutosh.ba...@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. 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". 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. 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? 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. copy_path_for_epq_recheck() and friends are really ugly. Should we consider just adding copyObject() support for those node types instead? 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. I'm sure there's more -- this is a huge patch and I don't fully understand it yet -- but I'm out of energy for tonight. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
