pgsql: Make the order of the header file includes consistent in non-bac

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

pgsql: Make the order of the header file includes consistent in non-bac

Amit Kapila-3
Make the order of the header file includes consistent in non-backend modules.

Similar to commit 7e735035f2, this commit makes the order of header file
inclusion consistent for non-backend modules.

In passing, fix the case where we were using angle brackets (<>) for the
local module includes instead of quotes ("").

Author: Vignesh C
Reviewed-by: Amit Kapila
Discussion: https://postgr.es/m/CALDaNm2Sznv8RR6Ex-iJO6xAdsxgWhCoETkaYX=+9DW3q0QCfA@...

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/dddf4cdc3300073ec04b2c3e482a4c1fa4b8191b

Modified Files
--------------
src/bin/pg_archivecleanup/pg_archivecleanup.c           |  6 ++----
src/bin/pg_basebackup/pg_basebackup.c                   |  2 +-
src/bin/pg_basebackup/pg_receivewal.c                   |  6 ++----
src/bin/pg_basebackup/pg_recvlogical.c                  |  7 ++-----
src/bin/pg_basebackup/receivelog.c                      |  9 +++------
src/bin/pg_basebackup/streamutil.c                      |  6 ++----
src/bin/pg_basebackup/walmethods.c                      |  3 +--
src/bin/pg_config/pg_config.c                           |  2 +-
src/bin/pg_controldata/pg_controldata.c                 |  3 +--
src/bin/pg_dump/common.c                                |  8 +++-----
src/bin/pg_dump/parallel.c                              |  3 +--
src/bin/pg_dump/pg_backup_archiver.c                    |  7 +++----
src/bin/pg_dump/pg_backup_custom.c                      |  3 +--
src/bin/pg_dump/pg_backup_db.c                          | 16 +++++++---------
src/bin/pg_dump/pg_backup_directory.c                   |  8 ++++----
src/bin/pg_dump/pg_backup_null.c                        |  5 ++---
src/bin/pg_dump/pg_backup_tar.c                         | 14 +++++++-------
src/bin/pg_dump/pg_dump.c                               | 13 +++++--------
src/bin/pg_dump/pg_dump_sort.c                          |  3 +--
src/bin/pg_dump/pg_dumpall.c                            |  7 +++----
src/bin/pg_dump/pg_restore.c                            |  4 +---
src/bin/pg_resetwal/pg_resetwal.c                       |  7 +++----
src/bin/pg_rewind/datapagemap.c                         |  3 +--
src/bin/pg_rewind/fetch.c                               |  2 +-
src/bin/pg_rewind/filemap.c                             |  5 ++---
src/bin/pg_rewind/libpq_fetch.c                         |  7 +++----
src/bin/pg_rewind/parsexlog.c                           |  6 ++----
src/bin/pg_rewind/pg_rewind.c                           |  9 ++++-----
src/bin/pg_rewind/timeline.c                            |  3 +--
src/bin/pg_test_fsync/pg_test_fsync.c                   |  3 +--
src/bin/pg_upgrade/check.c                              |  1 -
src/bin/pg_upgrade/controldata.c                        |  4 ++--
src/bin/pg_upgrade/dump.c                               |  4 +---
src/bin/pg_upgrade/file.c                               | 13 ++++++-------
src/bin/pg_upgrade/function.c                           |  4 +---
src/bin/pg_upgrade/info.c                               |  4 +---
src/bin/pg_upgrade/option.c                             |  6 ++----
src/bin/pg_upgrade/parallel.c                           |  1 -
src/bin/pg_upgrade/pg_upgrade.c                         | 10 +++++-----
src/bin/pg_upgrade/relfilenode.c                        |  7 +++----
src/bin/pg_upgrade/server.c                             |  1 -
src/bin/pg_upgrade/util.c                               |  5 ++---
src/bin/pg_upgrade/version.c                            |  5 +----
src/bin/pg_waldump/compat.c                             |  2 +-
src/bin/pg_waldump/pg_waldump.c                         |  3 +--
src/bin/pgbench/pgbench.c                               | 15 ++++++++-------
src/bin/psql/common.c                                   | 10 ++++------
src/bin/psql/copy.c                                     | 10 ++++------
src/bin/psql/crosstabview.c                             |  3 +--
src/bin/psql/describe.c                                 |  7 ++-----
src/bin/psql/help.c                                     |  3 +--
src/bin/psql/input.c                                    |  5 ++---
src/bin/psql/large_obj.c                                |  5 ++---
src/bin/psql/mainloop.c                                 |  8 +++-----
src/bin/psql/prompt.c                                   |  3 +--
src/bin/psql/startup.c                                  | 10 +++-------
src/bin/psql/variables.c                                |  4 +---
src/common/f2s.c                                        |  3 +--
src/fe_utils/print.c                                    |  4 +---
src/fe_utils/recovery_gen.c                             |  3 +--
src/fe_utils/string_utils.c                             |  4 +---
src/interfaces/ecpg/compatlib/informix.c                | 16 ++++++++--------
src/interfaces/ecpg/ecpglib/connect.c                   |  4 ++--
src/interfaces/ecpg/ecpglib/data.c                      | 10 +++++-----
src/interfaces/ecpg/ecpglib/descriptor.c                |  5 ++---
src/interfaces/ecpg/ecpglib/error.c                     |  2 +-
src/interfaces/ecpg/ecpglib/execute.c                   | 17 ++++++++---------
src/interfaces/ecpg/ecpglib/memory.c                    |  4 ++--
src/interfaces/ecpg/ecpglib/misc.c                      | 11 ++++++-----
src/interfaces/ecpg/ecpglib/prepare.c                   |  4 ++--
src/interfaces/ecpg/ecpglib/sqlda.c                     |  9 ++++-----
src/interfaces/ecpg/ecpglib/typename.c                  |  5 ++---
src/interfaces/ecpg/pgtypeslib/common.c                 |  2 +-
src/interfaces/ecpg/pgtypeslib/datetime.c               |  4 ++--
src/interfaces/ecpg/pgtypeslib/dt_common.c              |  2 +-
src/interfaces/ecpg/pgtypeslib/interval.c               |  4 ++--
src/interfaces/ecpg/pgtypeslib/numeric.c                |  5 +++--
src/interfaces/ecpg/pgtypeslib/timestamp.c              |  5 ++---
src/interfaces/ecpg/preproc/c_keywords.c                |  5 ++---
src/interfaces/ecpg/preproc/ecpg_keywords.c             |  5 ++---
src/interfaces/libpq/fe-auth.c                          |  3 +--
src/interfaces/libpq/fe-connect.c                       | 16 +++++++---------
src/interfaces/libpq/fe-exec.c                          |  9 ++++-----
src/interfaces/libpq/fe-misc.c                          |  3 +--
src/interfaces/libpq/fe-protocol2.c                     |  8 +++-----
src/interfaces/libpq/fe-protocol3.c                     | 10 ++++------
src/interfaces/libpq/fe-secure-gssapi.c                 |  2 +-
src/interfaces/libpq/fe-secure.c                        |  8 ++++----
src/pl/plpgsql/src/pl_comp.c                            |  4 +---
src/pl/plpgsql/src/pl_exec.c                            |  4 +---
src/pl/plpgsql/src/pl_funcs.c                           |  4 +---
src/pl/plpgsql/src/pl_handler.c                         |  4 +---
src/port/pg_crc32c_armv8.c                              |  4 ++--
src/port/pg_crc32c_sse42.c                              |  4 ++--
src/port/tar.c                                          |  4 +++-
src/test/isolation/isolationtester.c                    |  5 ++---
.../modules/test_ginpostinglist/test_ginpostinglist.c   |  2 +-
src/test/modules/test_integerset/test_integerset.c      |  6 +++---
src/test/modules/test_rls_hooks/test_rls_hooks.c        |  7 ++-----
src/test/modules/test_shm_mq/setup.c                    |  3 +--
src/test/regress/pg_regress.c                           |  3 +--
src/test/regress/regress.c                              |  3 +--
102 files changed, 237 insertions(+), 345 deletions(-)

Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Make the order of the header file includes consistent in non-bac

Michael Paquier-2
Hi Amit,

On Fri, Oct 25, 2019 at 02:24:50AM +0000, Amit Kapila wrote:
> Make the order of the header file includes consistent in non-backend modules.
>
> Similar to commit 7e735035f2, this commit makes the order of header file
> inclusion consistent for non-backend modules.
>
> In passing, fix the case where we were using angle brackets (<>) for the
> local module includes instead of quotes ("").

locust has just reported a build failure here:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=locust&dt=2019-10-25%2002%3A58%3A55

And the cause is visibly this portion of the commit:
--- a/src/interfaces/ecpg/pgtypeslib/dt_common.c
+++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c
@@ -7,9 +7,9 @@
 #include <math.h>

 #include "common/string.h"
-#include "pgtypeslib_extern.h"
 #include "dt.h"
 #include "pgtypes_timestamp.h"
+#include "pgtypeslib_extern.h"  
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Make the order of the header file includes consistent in non-bac

akapila
On Fri, Oct 25, 2019 at 8:47 AM Michael Paquier <[hidden email]> wrote:

>
> Hi Amit,
>
> On Fri, Oct 25, 2019 at 02:24:50AM +0000, Amit Kapila wrote:
> > Make the order of the header file includes consistent in non-backend modules.
> >
> > Similar to commit 7e735035f2, this commit makes the order of header file
> > inclusion consistent for non-backend modules.
> >
> > In passing, fix the case where we were using angle brackets (<>) for the
> > local module includes instead of quotes ("").
>
> locust has just reported a build failure here:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=locust&dt=2019-10-25%2002%3A58%3A55
>

Yeah, I have noticed that and working on it.

> And the cause is visibly this portion of the commit:
> --- a/src/interfaces/ecpg/pgtypeslib/dt_common.c
> +++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c
> @@ -7,9 +7,9 @@
>  #include <math.h>
>
>  #include "common/string.h"
> -#include "pgtypeslib_extern.h"
>  #include "dt.h"
>  #include "pgtypes_timestamp.h"
> +#include "pgtypeslib_extern.h"

Right, but trying to see why it hasn't failed on other machines and in
my local build.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Make the order of the header file includes consistent in non-bac

akapila
On Fri, Oct 25, 2019 at 8:54 AM Amit Kapila <[hidden email]> wrote:

>
> On Fri, Oct 25, 2019 at 8:47 AM Michael Paquier <[hidden email]> wrote:
> >
> > Hi Amit,
> >
> > On Fri, Oct 25, 2019 at 02:24:50AM +0000, Amit Kapila wrote:
> > > Make the order of the header file includes consistent in non-backend modules.
> > >
> > > Similar to commit 7e735035f2, this commit makes the order of header file
> > > inclusion consistent for non-backend modules.
> > >
> > > In passing, fix the case where we were using angle brackets (<>) for the
> > > local module includes instead of quotes ("").
> >
> > locust has just reported a build failure here:
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=locust&dt=2019-10-25%2002%3A58%3A55
> >
>
> Yeah, I have noticed that and working on it.
>
> > And the cause is visibly this portion of the commit:
> > --- a/src/interfaces/ecpg/pgtypeslib/dt_common.c
> > +++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c
> > @@ -7,9 +7,9 @@
> >  #include <math.h>
> >
> >  #include "common/string.h"
> > -#include "pgtypeslib_extern.h"
> >  #include "dt.h"
> >  #include "pgtypes_timestamp.h"
> > +#include "pgtypeslib_extern.h"
>
> Right, but trying to see why it hasn't failed on other machines and in
> my local build.
>

One possible reason could be that pgtypeslib_extern.h has define like below:
#ifndef bool
#define bool char
#endif /* ndef bool */

It is possible that all the functions that have bool has a parameter
in dt_common.c treat it as 'char' where as the declaration of same
functions in dt.h doesn't consider bool as char because now
pgtypeslib_extern.h is included after dt.h.  It seems that the
platforms where it failed doesn't have bool defined.  What do you
think?  I think we can revert the change in that file.

BTW, prairiedog is also show similar failure and both seems to have
similar OS except for versions.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Make the order of the header file includes consistent in non-bac

akapila
On Fri, Oct 25, 2019 at 9:32 AM Amit Kapila <[hidden email]> wrote:

>
> On Fri, Oct 25, 2019 at 8:54 AM Amit Kapila <[hidden email]> wrote:
> >
> > > And the cause is visibly this portion of the commit:
> > > --- a/src/interfaces/ecpg/pgtypeslib/dt_common.c
> > > +++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c
> > > @@ -7,9 +7,9 @@
> > >  #include <math.h>
> > >
> > >  #include "common/string.h"
> > > -#include "pgtypeslib_extern.h"
> > >  #include "dt.h"
> > >  #include "pgtypes_timestamp.h"
> > > +#include "pgtypeslib_extern.h"
> >
> > Right, but trying to see why it hasn't failed on other machines and in
> > my local build.
> >
>
> One possible reason could be that pgtypeslib_extern.h has define like below:
> #ifndef bool
> #define bool char
> #endif /* ndef bool */
>
> It is possible that all the functions that have bool has a parameter
> in dt_common.c treat it as 'char' where as the declaration of same
> functions in dt.h doesn't consider bool as char because now
> pgtypeslib_extern.h is included after dt.h.  It seems that the
> platforms where it failed doesn't have bool defined.  What do you
> think?  I think we can revert the change in that file.
>
I am planning to revert that part of the change as attached.  We can
think of even moving the above definition of bool but I am not sure
what is the right place for the same and I am not sure if it is a
better idea.  I think that can be explored later.  It is better to
revert that change for now.  Do you think we should add some comment
like:
"We need to include pgtypeslib_extern.h before dt.h as it defines bool
which is used in dt.h."?  I have not added as I couldn't experiment
any thing related to that.

I will apply the attached in one hour or so unless you or someone has
a better idea.  In the meantime, we can see if there is any other
problem with buildfarm.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

0001-Revert-part-of-commit-dddf4cdc3.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Make the order of the header file includes consistent in non-bac

Michael Paquier-2
In reply to this post by akapila
On Fri, Oct 25, 2019 at 09:32:29AM +0530, Amit Kapila wrote:
> One possible reason could be that pgtypeslib_extern.h has define like below:
> #ifndef bool
> #define bool char
> #endif /* ndef bool */

Possible.  I have not looked closely.

> It is possible that all the functions that have bool has a parameter
> in dt_common.c treat it as 'char' where as the declaration of same
> functions in dt.h doesn't consider bool as char because now
> pgtypeslib_extern.h is included after dt.h.  It seems that the
> platforms where it failed doesn't have bool defined.  What do you
> think?  I think we can revert the change in that file.
>
> BTW, prairiedog is also show similar failure and both seems to have
> similar OS except for versions.

Remembering the recent efforts that went behind stdbool.h (see
9a95a77), I think that we should be able to cleanup this header more.
Reverting partially the change looks better for now to put that the
buildfarm back to green.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Make the order of the header file includes consistent in non-bac

Andrew Gierth
In reply to this post by akapila
>>>>> "Amit" == Amit Kapila <[hidden email]> writes:

 Amit> BTW, prairiedog is also show similar failure and both seems to
 Amit> have similar OS except for versions.

Be aware that prairiedog and locust (unlike the rest of the buildfarm)
have compilers whose "bool" type is not 1 byte long; PG can't use that,
so we don't include <stdbool.h> on those platforms.

Looking at c.h, the problem seems to be that when we define our own bool
in the absence of a usable stdbool.h, we do so as a typedef and not a
macro. So pgtypeslib_extern.h ends up redefining it in that case, and
(this bit may be my fault: see d26a810eb) uses a different type to c.h
(char vs. unsigned char).

(stdbool.h is required by spec to do #define bool _Bool rather than a
typedef, hence the difference in behavior)

I'm no expert on the ECPG internals, but this doesn't look like an
exposed interface, so maybe it just shouldn't be trying to #define bool
at all and just rely on c.h (from postgres_fe.h) to declare the type?

--
Andrew (irc:RhodiumToad)


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Make the order of the header file includes consistent in non-bac

akapila
On Fri, Oct 25, 2019 at 1:30 PM Andrew Gierth
<[hidden email]> wrote:

>
> >>>>> "Amit" == Amit Kapila <[hidden email]> writes:
>
>  Amit> BTW, prairiedog is also show similar failure and both seems to
>  Amit> have similar OS except for versions.
>
> Be aware that prairiedog and locust (unlike the rest of the buildfarm)
> have compilers whose "bool" type is not 1 byte long; PG can't use that,
> so we don't include <stdbool.h> on those platforms.
>
> Looking at c.h, the problem seems to be that when we define our own bool
> in the absence of a usable stdbool.h, we do so as a typedef and not a
> macro. So pgtypeslib_extern.h ends up redefining it in that case, and
> (this bit may be my fault: see d26a810eb) uses a different type to c.h
> (char vs. unsigned char).
>
> (stdbool.h is required by spec to do #define bool _Bool rather than a
> typedef, hence the difference in behavior)
>
> I'm no expert on the ECPG internals, but this doesn't look like an
> exposed interface, so maybe it just shouldn't be trying to #define bool
> at all and just rely on c.h (from postgres_fe.h) to declare the type?
>

+1.  I think we can discuss this on hackers, so started a thread [1].

[1] - https://www.postgresql.org/message-id/CAA4eK1LmaKO7Du9M9Lo%3DkxGU8sB6aL8fa3sF6z6d5yYYVe3BuQ%40mail.gmail.com

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Make the order of the header file includes consistent in non-bac

akapila
In reply to this post by Michael Paquier-2
On Fri, Oct 25, 2019 at 11:47 AM Michael Paquier <[hidden email]> wrote:

>
> On Fri, Oct 25, 2019 at 09:32:29AM +0530, Amit Kapila wrote:
> > One possible reason could be that pgtypeslib_extern.h has define like below:
> > #ifndef bool
> > #define bool char
> > #endif /* ndef bool */
>
> Possible.  I have not looked closely.
>
> > It is possible that all the functions that have bool has a parameter
> > in dt_common.c treat it as 'char' where as the declaration of same
> > functions in dt.h doesn't consider bool as char because now
> > pgtypeslib_extern.h is included after dt.h.  It seems that the
> > platforms where it failed doesn't have bool defined.  What do you
> > think?  I think we can revert the change in that file.
> >
> > BTW, prairiedog is also show similar failure and both seems to have
> > similar OS except for versions.
>
> Remembering the recent efforts that went behind stdbool.h (see
> 9a95a77), I think that we should be able to cleanup this header more.
> Reverting partially the change looks better for now to put that the
> buildfarm back to green.
>

Reverted and the buildfarm is green again.  Thanks!

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: pgsql: Make the order of the header file includes consistent in non-bac

Michael Paquier-2
On Fri, Oct 25, 2019 at 03:21:13PM +0530, Amit Kapila wrote:
> Reverted and the buildfarm is green again.  Thanks!

Thanks for fixing, and the follow-up thread.  (Just saw the rest as
well.)
--
Michael

signature.asc (849 bytes) Download Attachment