>>
>> 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
