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

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&amp;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

On Fri, Oct 25, 2019 at 8:47 AM Michael Paquier <michael@paquier.xyz> 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&amp;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

On Fri, Oct 25, 2019 at 8:54 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Oct 25, 2019 at 8:47 AM Michael Paquier <michael@paquier.xyz> 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&amp;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

On Fri, Oct 25, 2019 at 9:32 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Oct 25, 2019 at 8:54 AM Amit Kapila <amit.kapila16@gmail.com> 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

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

"Amit" == Amit Kapila <amit.kapila16@gmail.com> 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)

On Fri, Oct 25, 2019 at 1:30 PM Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:

"Amit" == Amit Kapila <amit.kapila16@gmail.com> 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]/messages/by-id/CAA4eK1LmaKO7Du9M9Lo=kxGU8sB6aL8fa3sF6z6d5yYYVe3BuQ@mail.gmail.com.

[1]: /messages/by-id/CAA4eK1LmaKO7Du9M9Lo=kxGU8sB6aL8fa3sF6z6d5yYYVe3BuQ@mail.gmail.com

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

On Fri, Oct 25, 2019 at 11:47 AM Michael Paquier <michael@paquier.xyz> 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

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