Re: [HACKERS] Obsolete comment in postgres_fdw.c

[Robert Haas]

On Mon, Mar 14, 2016 at 4:31 AM, Etsuro Fujita
 wrote:
>> It was left there intentionally to document all the conditions in one
>> place (some from the core and some from the FDW itself), for a ready
>> reference. In case tomorrow core thinks that matching user mapping is
>> not required, postgres_fdw would still require it to be incorporated.
>
> Thank you for the explanation!  I understand the reason, but that seems
> confusing to me.

Agreed.

>> In addition, I'd like to update some related comments in
>> src/include/nodes/relation.h and
>> src/backend/optimizer/path/joinpath.c.
>>
>>
>> Those look fine. Sorry for missing those in the commit and thanks for
>> providing a patch for the same.
>
> No problem.

Committed.

-- 
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

Re: [HACKERS] Obsolete comment in postgres_fdw.c

[Etsuro Fujita]

On 2016/03/14 16:42, Ashutosh Bapat wrote:

On Mon, Mar 14, 2016 at 9:05 AM, Etsuro Fujita
> wrote:



Here is the comments for foreign_join_ok in postgres_fdw.c:

/*
  * Assess whether the join between inner and outer relations can be
pushed down
  * to the foreign server. As a side effect, save information we obtain
in this
  * function to PgFdwRelationInfo passed in.
  *
  * Joins that satisfy conditions below are safe to push down.
  *
  * 1) Join type is INNER or OUTER (one of LEFT/RIGHT/FULL)
  * 2) Both outer and inner portions are safe to push-down
  * 3) All foreign tables in the join belong to the same foreign server
and use
  *the same user mapping.
  * 4) All join conditions are safe to push down
  * 5) No relation has local filter (this can be relaxed for INNER JOIN,
if we
  *can move unpushable clauses upwards in the join tree).
  */

The condition 3 is now checked by the core, so I'd like to remove that
condition from the above comments.



It was left there intentionally to document all the conditions in one
place (some from the core and some from the FDW itself), for a ready
reference. In case tomorrow core thinks that matching user mapping is
not required, postgres_fdw would still require it to be incorporated.


Thank you for the explanation!  I understand the reason, but that seems 
confusing to me.



In addition, I'd like to update some related comments in
src/include/nodes/relation.h and src/backend/optimizer/path/joinpath.c.


Those look fine. Sorry for missing those in the commit and thanks for
providing a patch for the same.


No problem.

Best regards,
Etsuro Fujita




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Obsolete comment in postgres_fdw.c

[Ashutosh Bapat]

On Mon, Mar 14, 2016 at 9:05 AM, Etsuro Fujita 
wrote:

> Hi,
>
> Here is the comments for foreign_join_ok in postgres_fdw.c:
>
> /*
>  * Assess whether the join between inner and outer relations can be
> pushed down
>  * to the foreign server. As a side effect, save information we obtain
> in this
>  * function to PgFdwRelationInfo passed in.
>  *
>  * Joins that satisfy conditions below are safe to push down.
>  *
>  * 1) Join type is INNER or OUTER (one of LEFT/RIGHT/FULL)
>  * 2) Both outer and inner portions are safe to push-down
>  * 3) All foreign tables in the join belong to the same foreign server
> and use
>  *the same user mapping.
>  * 4) All join conditions are safe to push down
>  * 5) No relation has local filter (this can be relaxed for INNER JOIN,
> if we
>  *can move unpushable clauses upwards in the join tree).
>  */
>
>
The condition 3 is now checked by the core, so I'd like to remove that
> condition from the above comments.
>

It was left there intentionally to document all the conditions in one place
(some from the core and some from the FDW itself), for a ready reference.
In case tomorrow core thinks that matching user mapping is not required,
postgres_fdw would still require it to be incorporated.


>
> In addition, I'd like to update some related comments in
> src/include/nodes/relation.h and src/backend/optimizer/path/joinpath.c.
>

Those look fine. Sorry for missing those in the commit and thanks for
providing a patch for the same.

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

[HACKERS] Obsolete comment in postgres_fdw.c

[Etsuro Fujita]

Hi,

Here is the comments for foreign_join_ok in postgres_fdw.c:

/*
 * Assess whether the join between inner and outer relations can be
pushed down
 * to the foreign server. As a side effect, save information we obtain
in this
 * function to PgFdwRelationInfo passed in.
 *
 * Joins that satisfy conditions below are safe to push down.
 *
 * 1) Join type is INNER or OUTER (one of LEFT/RIGHT/FULL)
 * 2) Both outer and inner portions are safe to push-down
 * 3) All foreign tables in the join belong to the same foreign server
and use
 *the same user mapping.
 * 4) All join conditions are safe to push down
 * 5) No relation has local filter (this can be relaxed for INNER JOIN,
if we
 *can move unpushable clauses upwards in the join tree).
 */

The condition 3 is now checked by the core, so I'd like to remove that
condition from the above comments.

In addition, I'd like to update some related comments in
src/include/nodes/relation.h and src/backend/optimizer/path/joinpath.c.

Attached is a patch for that.

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 3345,3354  postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
   *
   * 1) Join type is INNER or OUTER (one of LEFT/RIGHT/FULL)
   * 2) Both outer and inner portions are safe to push-down
!  * 3) All foreign tables in the join belong to the same foreign server and use
!  *	  the same user mapping.
!  * 4) All join conditions are safe to push down
!  * 5) No relation has local filter (this can be relaxed for INNER JOIN, if we
   *	  can move unpushable clauses upwards in the join tree).
   */
  static bool
--- 3345,3352 
   *
   * 1) Join type is INNER or OUTER (one of LEFT/RIGHT/FULL)
   * 2) Both outer and inner portions are safe to push-down
!  * 3) All join conditions are safe to push down
!  * 4) No relation has local filter (this can be relaxed for INNER JOIN, if we
   *	  can move unpushable clauses upwards in the join tree).
   */
  static bool
*** a/src/backend/optimizer/path/joinpath.c
--- b/src/backend/optimizer/path/joinpath.c
***
*** 213,219  add_paths_to_joinrel(PlannerInfo *root,
  
  	/*
  	 * 5. If inner and outer relations are foreign tables (or joins) belonging
! 	 * to the same server, give the FDW a chance to push down joins.
  	 */
  	if (joinrel->fdwroutine &&
  		joinrel->fdwroutine->GetForeignJoinPaths)
--- 213,220 
  
  	/*
  	 * 5. If inner and outer relations are foreign tables (or joins) belonging
! 	 * to the same server and using the same user mapping, give the FDW a
! 	 * chance to push down joins.
  	 */
  	if (joinrel->fdwroutine &&
  		joinrel->fdwroutine->GetForeignJoinPaths)
*** a/src/include/nodes/relation.h
--- b/src/include/nodes/relation.h
***
*** 416,421  typedef struct PlannerInfo
--- 416,422 
   * all belong to the same foreign server, these fields will be set:
   *
   *		serverid - OID of foreign server, if foreign table (else InvalidOid)
+  *		umid - OID of user mapping, if foreign table (else InvalidOid)
   *		fdwroutine - function hooks for FDW, if foreign table (else NULL)
   *		fdw_private - private state for FDW, if foreign table (else NULL)
   *

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers