>> >> Here's the patch attached. > > Agreed. It seems like a good idea to keep that logic in a single location
Ok. > > I've beaten your patch around a bit and come up with the attached. The new name merge_fdw_options() is shorter than the one I chose, but we are not exactly merging options for an upper relation since there isn't the other fpinfo to merge from. But I think we can live with merge_fdw_options(). Looks like you forgot to compile the patch. It gave error postgres_fdw.c: In function ‘merge_fdw_options’: postgres_fdw.c:4337:2: error: ‘RelOptInfo’ has no member named ‘server’ postgres_fdw.c:4337:2: error: ‘RelOptInfo’ has no member named ‘server’ Once I fixed that, the testcases started showing an assertion failure, since fpinfo of a base relation can not have an outerrel. Fixed the assertion as well. If we are passing fpinfo's of joining relations, there's no need to have outerrel and innerrel in fpinfo of join. Also, you had removed comment /* + * Set the foreign server to which this join will be shipped if found safe + * to push-down. We need server specific options like extensions to decide + * push-down safety, so set FDW specific options. + */ But I think it's important to have it there, so that reader knows why merge_fdw_options() needs to be called before assessing quals. I have updated it a bit to clarify this further. Also, merge_fdw_options() shouldn't set fpinfo->server since it's not an FDW option per say. It's better to keep that outside of this function. With your patch fpinfo->server was being set twice for an upper relation. > > The changes from yours are mostly cosmetic, but I've also added a > regression test too. Thanks a lot for the test. PFA updated patch. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
foreign_outerjoin_pushdown_fix_v3.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
