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

Hi Hanada-san,
I have reviewed usermapping patch and here are some comments.

The patch applies cleanly on Head and compiles without problem. The make
check in regression folder does not show any failure.

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.

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?

Rest of the patch looks fine.

On Thu, May 28, 2015 at 10:50 AM, Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>
wrote:

> 2015-05-22 18:37 GMT+09:00 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> > 2015-05-21 23:11 GMT+09:00 Robert Haas <robertmhaas(at)gmail(dot)com>:
> >> On Sat, May 16, 2015 at 9:04 AM, Shigeru Hanada
> >> <shigeru(dot)hanada(at)gmail(dot)com> wrote:
> >>> d) All relations must have the same effective user id
> >>> This check is done with userid stored in PgFdwRelationInfo, which is
> >>> valid only when underlying relations have the same effective user id.
> >>> Here "effective user id" means id of the user executing the query, or
> >>> the owner of the view when the foreign table is accessed via view.
> >>> Tom suggested that it is necessary to check that user mapping matches
> >>> for security reason, and now I think it's better than checking
> >>> effective user as current postgres_fdw does.
> >>
> >> So, should this be a separate patch?
>
> I wrote patches for this issue. Let me describe their design.
>
> To require the matching of user mapping between two relations (base or
> join) which involves foreign tables, it would require these stuffs:
>
> a) Add new field umid to RelOptInfo to hold OID of user mapping used
> for the relation and children
> b) Propagate umid up to join relation only when both outer and inner
> have the save valid values
> c) Check matching of umid between two relations to be joined before
> calling GetForeignJoinPaths
>
> For a), adding an OID field would not be a serious burden. Obtaining
> the OID of user mapping can be accomplished by calling GetUserMapping
> in get_relation_info, but it allocates an unnecessary UserMapping
> object, so I added GetUserMappingId which just returns OID.
>
> One concern about getting user mapping is checkAsUser. Currently FDW
> authors are required to consider which user is valid as argument of
> GetUserMapping, because it is different from the result of GetUserId
> when the target relation is accessed via a view which is owned by
> another user. This requirement would be easily ignored by FDW authors
> and the ignorance causes terrible security issues. So IMO it should
> be hidden in the core code, and FDW authors should use user mappings
> determined by the core. This would break FDW I/F, so we can't change
> it right now, but making GetUserMapping obsolete and mention it in the
> documents would guide FDW authors to the right direction.
>
> For b), such check should be done in build_join_rel, similarly to serverid.
>
> For c), we can reuse the check about RelOptInfo#fdwroutine in
> add_paths_to_joinrel, because all of serverid, umid and fdwroutine are
> valid only when both the servers and the user mappings match between
> outer and inner relations.
>
> Attached are the patches which implement the idea above except
> checkAsUser issue.
> usermapping_matching.patch: check matching of user mapping OIDs
> add_GetUserMappingById.patch: add helper function which is handy for
> FDWs to obtain UserMapping
> foreign_join_v16.patch: postgres_fdw which assumes user mapping
> matching is done in core
>
> Another idea about a) is to have an entire UserMapping object for each
> RelOptInfo, but it would require UserMapping to have extra field of
> its Oid, say umid, to compare them by OID. But IMO creating
> UserMapping for every RelOptInfo seems waste of space and time.
>
> Thoughts?
> --
> Shigeru HANADA
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

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