Misplacement of function declaration in contrib/postgres_fdw/postgres_fdw.h

Started by Etsuro Fujitaabout 9 years ago4 messages
#1Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
1 attachment(s)

Hi,

While working on pushing down more joins/updates to the remote, I
noticed that in contrib/postgres_fdw/postgres_fdw.h the declaration of
get_jointype_name is misplaced in the section of shippable.c. Since
that function is defined in contrib/postgres_fdw/deparse.c, we should
put that declaration in the section of deparse.c in the header file.
Attached is a patch for that.

Best regards,
Etsuro Fujita

Attachments:

postgres-fdw-h.patchtext/x-patch; name=postgres-fdw-h.patchDownload
*** a/contrib/postgres_fdw/postgres_fdw.h
--- b/contrib/postgres_fdw/postgres_fdw.h
***************
*** 163,172 **** extern void deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root,
  						RelOptInfo *foreignrel, List *tlist,
  						List *remote_conds, List *pathkeys,
  						List **retrieved_attrs, List **params_list);
  
  /* in shippable.c */
  extern bool is_builtin(Oid objectId);
  extern bool is_shippable(Oid objectId, Oid classId, PgFdwRelationInfo *fpinfo);
- extern const char *get_jointype_name(JoinType jointype);
  
  #endif   /* POSTGRES_FDW_H */
--- 163,172 ----
  						RelOptInfo *foreignrel, List *tlist,
  						List *remote_conds, List *pathkeys,
  						List **retrieved_attrs, List **params_list);
+ extern const char *get_jointype_name(JoinType jointype);
  
  /* in shippable.c */
  extern bool is_builtin(Oid objectId);
  extern bool is_shippable(Oid objectId, Oid classId, PgFdwRelationInfo *fpinfo);
  
  #endif   /* POSTGRES_FDW_H */
#2Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Etsuro Fujita (#1)
Re: Misplacement of function declaration in contrib/postgres_fdw/postgres_fdw.h

On Thu, Jan 12, 2017 at 8:49 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

Hi,

While working on pushing down more joins/updates to the remote, I noticed
that in contrib/postgres_fdw/postgres_fdw.h the declaration of
get_jointype_name is misplaced in the section of shippable.c. Since that
function is defined in contrib/postgres_fdw/deparse.c, we should put that
declaration in the section of deparse.c in the header file. Attached is a
patch for that.

I think, initially (probably in a never committed patch) the function
was used to check whether a join type is shippable and if so return
the name to be used in the query. That may be the reason why it ended
up in shippability.c. But later the shippability test was separated
from the code which required the string representation. Thanks for
pointing out the descripancy. The patch looks good. As a side change,
should we include "JOIN" in the string returned by this fuction? The
two places where this function is called, append "JOIN" to the string
returned by this function. Although, even without that change, the
patch looks good.

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

#3Etsuro Fujita
fujita.etsuro@lab.ntt.co.jp
In reply to: Ashutosh Bapat (#2)
Re: Misplacement of function declaration in contrib/postgres_fdw/postgres_fdw.h

On 2017/01/12 13:52, Ashutosh Bapat wrote:

On Thu, Jan 12, 2017 at 8:49 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

While working on pushing down more joins/updates to the remote, I noticed
that in contrib/postgres_fdw/postgres_fdw.h the declaration of
get_jointype_name is misplaced in the section of shippable.c. Since that
function is defined in contrib/postgres_fdw/deparse.c, we should put that
declaration in the section of deparse.c in the header file. Attached is a
patch for that.

I think, initially (probably in a never committed patch) the function
was used to check whether a join type is shippable and if so return
the name to be used in the query. That may be the reason why it ended
up in shippability.c. But later the shippability test was separated
from the code which required the string representation.

Thanks for the explanation!

Thanks for
pointing out the descripancy. The patch looks good. As a side change,
should we include "JOIN" in the string returned by this fuction? The
two places where this function is called, append "JOIN" to the string
returned by this function.

I was thinking that, so +1.

Although, even without that change, the
patch looks good.

Thanks again.

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

#4Robert Haas
robertmhaas@gmail.com
In reply to: Etsuro Fujita (#1)
Re: Misplacement of function declaration in contrib/postgres_fdw/postgres_fdw.h

On Wed, Jan 11, 2017 at 10:19 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

While working on pushing down more joins/updates to the remote, I noticed
that in contrib/postgres_fdw/postgres_fdw.h the declaration of
get_jointype_name is misplaced in the section of shippable.c. Since that
function is defined in contrib/postgres_fdw/deparse.c, we should put that
declaration in the section of deparse.c in the header file. Attached is a
patch for that.

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