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

On Tue, Aug 4, 2015 at 2:20 PM, Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>
wrote:

> Hi Ashutosh,
>
> Sorry for leaving the thread.
>
> 2015-07-20 16:09 GMT+09:00 Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com
> >:
> > In find_user_mapping(), if the first cache search returns a valid tuple,
> it
> > is checked twice for validity, un-necessarily. Instead if the first
> search
> > returns a valid tuple, it should be returned immediately. I see that this
> > code was copied from GetUserMapping(), where as well it had the same
> > problem.
>
> Oops. I changed find_user_mapping to exit immediately when any valid
> cache was found.
>

Thanks.

>
> > In build_join_rel(), we check whether user mappings of the two joining
> > relations are same. If the user mappings are same, doesn't that imply
> that
> > the foreign server and local user are same too?
>
> Yes, validity of umid is identical to serverid. We can remove the
> check for serverid for some cycles. One idea is to put Assert for
> serverid inside the if-statement block.
>

> > Rest of the patch looks fine.
>
> Thanks
>
>
I started reviewing the other patches.

In patch foreign_join_v16.patch, the user mapping structure being passed to
GetConnection() is the one obtained from GetUserMappingById().
GetUserMappingById() constructs the user mapping structure from the user
mapping catalog. For public user mappings, catalog entries have InvalidOid
as userid. Thus, with this patch there is a chance that userid in
UserMapping being passed to GetConnection(), contains InvalidOid as userid.
This is not the case today. The UserMapping structure constructed using
GetUserMapping(Oid userid, Oid serverid) (which ultimately gets passed to
GetConnection()), has the passed in userid and not the one in the catalog.
Is this change intentional?

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