Re: GinPageIs* don't actually return a boolean

Hi,

#define GinPageIsLeaf(page) ( GinPageGetOpaque(page)->flags & GIN_LEAF )
#define GinPageIsData(page) ( GinPageGetOpaque(page)->flags & GIN_DATA )
#define GinPageIsList(page) ( GinPageGetOpaque(page)->flags & GIN_LIST )
...

These macros don't actually return a boolean that's comparable with our
true/false. That doesn't strike me as a good idea.

If there's actually a boolean type defined by some included header (in
which case we don't overwrite it in c.h!) this actually can lead to
tests failing. If e.g. stdbool.h is included in c.h the tests fail with
gcc-4.9.

I think we should add a !! to these macros to make sure it's an actual
boolean.

This has been the case since gin's initial commit in 8a3631f8d86cdd9b0 .

Greetings,

Andres Freund

On 2015-08-11 17:42:37 +0200, Andres Freund wrote:
> Hi,
>
> #define GinPageIsLeaf(page) ( GinPageGetOpaque(page)->flags & GIN_LEAF )
> #define GinPageIsData(page) ( GinPageGetOpaque(page)->flags & GIN_DATA )
> #define GinPageIsList(page) ( GinPageGetOpaque(page)->flags & GIN_LIST )
> ...
>
> These macros don't actually return a boolean that's comparable with our
> true/false. That doesn't strike me as a good idea.
>
> If there's actually a boolean type defined by some included header (in
> which case we don't overwrite it in c.h!) this actually can lead to
> tests failing. If e.g. stdbool.h is included in c.h the tests fail with
> gcc-4.9.

I guess the reason here is that if it's a boolean type known to the
compiler it's free to assume that it only contains true/false...

> I think we should add a !! to these macros to make sure it's an actual
> boolean.
>
> This has been the case since gin's initial commit in 8a3631f8d86cdd9b0 .

Even worse, we have that kind of macro all over. I thought
e.g. HeapTupleHeaderIs* would be safe agains that, but that turns out
not be the case.

Greetings,

Andres Freund

On Tue, Aug 11, 2015 at 11:42 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> #define GinPageIsLeaf(page) ( GinPageGetOpaque(page)->flags & GIN_LEAF )
> #define GinPageIsData(page) ( GinPageGetOpaque(page)->flags & GIN_DATA )
> #define GinPageIsList(page) ( GinPageGetOpaque(page)->flags & GIN_LIST )
> ...
>
> These macros don't actually return a boolean that's comparable with our
> true/false. That doesn't strike me as a good idea.
>
> If there's actually a boolean type defined by some included header (in
> which case we don't overwrite it in c.h!) this actually can lead to
> tests failing. If e.g. stdbool.h is included in c.h the tests fail with
> gcc-4.9.

!! is unknown to our codebase except where you've added it, and
personally, I hate that idiom. I think we should write (val) != 0
instead of !!val.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2015-08-11 12:04:48 -0400, Robert Haas wrote:
> On Tue, Aug 11, 2015 at 11:42 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > #define GinPageIsLeaf(page) ( GinPageGetOpaque(page)->flags & GIN_LEAF )
> > #define GinPageIsData(page) ( GinPageGetOpaque(page)->flags & GIN_DATA )
> > #define GinPageIsList(page) ( GinPageGetOpaque(page)->flags & GIN_LIST )
> > ...
> >
> > These macros don't actually return a boolean that's comparable with our
> > true/false. That doesn't strike me as a good idea.
> >
> > If there's actually a boolean type defined by some included header (in
> > which case we don't overwrite it in c.h!) this actually can lead to
> > tests failing. If e.g. stdbool.h is included in c.h the tests fail with
> > gcc-4.9.
>
> !! is unknown to our codebase except where you've added it, and
> personally, I hate that idiom. I think we should write (val) != 0
> instead of !!val.

Hm. I find !! slightly simpler and it's pretty widely used in other
projects, but I don't care much. As long as we fix the underlying issue,
which != 0 certainly does...

Andres Freund <andres(at)anarazel(dot)de> writes:
> #define GinPageIsLeaf(page) ( GinPageGetOpaque(page)->flags & GIN_LEAF )
> #define GinPageIsData(page) ( GinPageGetOpaque(page)->flags & GIN_DATA )
> #define GinPageIsList(page) ( GinPageGetOpaque(page)->flags & GIN_LIST )

> These macros don't actually return a boolean that's comparable with our
> true/false. That doesn't strike me as a good idea.

Agreed, this is risky. For example, if the bit being tested is to the
left of the lowest byte of "flags", storing the result into a bool
variable would do the wrong thing.

> I think we should add a !! to these macros to make sure it's an actual
> boolean.

Please write it more like

#define GinPageIsLeaf(page) ((GinPageGetOpaque(page)->flags & GIN_LEAF) != 0)

We do not use !! elsewhere for this purpose, and I for one find it a
pretty ugly locution.

regards, tom lane

On Tue, Aug 11, 2015 at 12:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> We do not use !! elsewhere for this purpose, and I for one find it a
> pretty ugly locution.

We do, actually, now, in contrib/pg_xlogdump/pg_xlogdump.c. I'd be in
favor of getting rid of those.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2015-08-11 12:43:03 -0400, Robert Haas wrote:
> On Tue, Aug 11, 2015 at 12:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > We do not use !! elsewhere for this purpose, and I for one find it a
> > pretty ugly locution.
>
> We do, actually, now, in contrib/pg_xlogdump/pg_xlogdump.c. I'd be in
> favor of getting rid of those.

And a bunch more places actually. Blame me. I'll come up with a patch
fixing the macros and converting existing !! style ones.

- Andres

Andres Freund wrote:
> On 2015-08-11 12:43:03 -0400, Robert Haas wrote:
> > On Tue, Aug 11, 2015 at 12:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > We do not use !! elsewhere for this purpose, and I for one find it a
> > > pretty ugly locution.
> >
> > We do, actually, now, in contrib/pg_xlogdump/pg_xlogdump.c. I'd be in
> > favor of getting rid of those.
>
> And a bunch more places actually. Blame me. I'll come up with a patch
> fixing the macros and converting existing !! style ones.

Actually there's one that's not yours ...

alvin: master 0$ git grep '!!' -- \*.c | grep -v '!!!'
contrib/isn/isn.c: * It's a helper function, not intended to be used!!
contrib/sepgsql/uavc.c: sepgsql_audit_log(!!denied,
src/backend/access/gist/gist.c: /* yes!!, found */
src/backend/access/gist/gist.c: * awful!!, we need search tree to find parent ... , but before we
src/backend/access/gist/gistbuild.c: /* yes!!, found it */
src/backend/access/transam/xact.c: Assert(!!(parsed->xinfo & XACT_XINFO_HAS_ORIGIN) == (origin_id != InvalidRepOriginId));
src/backend/executor/nodeModifyTable.c: * ctid!! */
src/backend/replication/logical/reorderbuffer.c: Assert(!create || !!txn);
src/backend/storage/lmgr/lwlock.c: !!(state & LW_VAL_EXCLUSIVE),
src/backend/storage/lmgr/lwlock.c: !!(state & LW_FLAG_HAS_WAITERS),
src/backend/storage/lmgr/lwlock.c: !!(state & LW_FLAG_RELEASE_OK))));
src/backend/tsearch/wparser_def.c: * order must be the same as in typedef enum {} TParserState!!
src/backend/utils/adt/like.c: * A big hack of the regexp.c code!! Contributed by
src/bin/pg_ctl/pg_ctl.c: * manager checkpoint, it's got nothing to do with database checkpoints!!
src/interfaces/ecpg/preproc/c_keywords.c: * !!WARNING!!: This list must be sorted, because binary
src/interfaces/ecpg/preproc/ecpg_keywords.c: * !!WARNING!!: This list must be sorted, because binary
src/pl/plpgsql/src/pl_scanner.c: * !!WARNING!!: These lists must be sorted by ASCII name, because binary

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2015-08-11 13:49:19 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> > On 2015-08-11 12:43:03 -0400, Robert Haas wrote:
> > > On Tue, Aug 11, 2015 at 12:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > > We do not use !! elsewhere for this purpose, and I for one find it a
> > > > pretty ugly locution.
> > >
> > > We do, actually, now, in contrib/pg_xlogdump/pg_xlogdump.c. I'd be in
> > > favor of getting rid of those.

Those got already removed by Heikki's WAL format changes...

> > And a bunch more places actually. Blame me. I'll come up with a patch
> > fixing the macros and converting existing !! style ones.
>
> Actually there's one that's not yours ...

I'm not touching sepgsql... ;)

I went through all headers in src/include and checked for macros
containing [^&]&[^&] and checked whether they have this hazard. Found a
fair number.

That patch also changes !! tests into != 0 style.

Greetings,

Andres Freund

Andres Freund <andres(at)anarazel(dot)de> writes:
> I went through all headers in src/include and checked for macros
> containing [^&]&[^&] and checked whether they have this hazard. Found a
> fair number.

> That patch also changes !! tests into != 0 style.

Looks OK to me, except I wonder why you did this

#define TRIGGER_FIRED_FOR_ROW(event) \
- ((event) & TRIGGER_EVENT_ROW)
+ (((event) & TRIGGER_EVENT_ROW) == TRIGGER_EVENT_ROW)

rather than != 0. That way doesn't look either more efficient or
more readable.

regards, tom lane

On 2015-08-12 18:52:59 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > I went through all headers in src/include and checked for macros
> > containing [^&]&[^&] and checked whether they have this hazard. Found a
> > fair number.
>
> > That patch also changes !! tests into != 0 style.
>
> Looks OK to me, except I wonder why you did this
>
> #define TRIGGER_FIRED_FOR_ROW(event) \
> - ((event) & TRIGGER_EVENT_ROW)
> + (((event) & TRIGGER_EVENT_ROW) == TRIGGER_EVENT_ROW)
>
> rather than != 0. That way doesn't look either more efficient or
> more readable.

Purely consistency with the surrounding code. I was on the fence about
that one...

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2015-08-12 18:52:59 -0400, Tom Lane wrote:
>> Looks OK to me, except I wonder why you did this
>>
>> #define TRIGGER_FIRED_FOR_ROW(event) \
>> - ((event) & TRIGGER_EVENT_ROW)
>> + (((event) & TRIGGER_EVENT_ROW) == TRIGGER_EVENT_ROW)
>>
>> rather than != 0. That way doesn't look either more efficient or
>> more readable.

> Purely consistency with the surrounding code. I was on the fence about
> that one...

The adjacent code is doing something different than a bit-test, though:
it's checking whether multibit fields have particular values.

regards, tom lane

On 2015-08-12 19:03:50 -0400, Tom Lane wrote:
> The adjacent code is doing something different than a bit-test, though:
> it's checking whether multibit fields have particular values.

Yea, I know, that's why I was on the fence about it. Since you have an
opinion and I couldn't really decide it's pretty clear which direction
to go ;)

Andres Freund wrote:

> I went through all headers in src/include and checked for macros
> containing [^&]&[^&] and checked whether they have this hazard. Found a
> fair number.
>
> That patch also changes !! tests into != 0 style.

I don't think you pushed this, did you?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On вторник, 29 сентября 2015 г. 19:02:59 MSK, Alvaro Herrera wrote:
> Andres Freund wrote:
>
>> I went through all headers in src/include and checked for macros
>> containing [^&]&[^&] and checked whether they have this hazard. Found a
>> fair number.
>>
>> That patch also changes !! tests into != 0 style.
>
> I don't think you pushed this, did you?
>

Hello hackers!
I've just run into a problem with these macro. Function ginStepRight breaks
completely when compiled using the MSVC2013 and MSVC2015 (since these
releases use C99's bools but without stdbool.h like C++).
I don't understand why the patch has not been commited yet. It seems to me
not so important !! or ! = 0, the solution is all that matters.

Thanks!

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Wed, Feb 10, 2016 at 1:17 AM, Yury Zhuravlev
<u(dot)zhuravlev(at)postgrespro(dot)ru> wrote:
> I've just run into a problem with these macro. Function ginStepRight breaks
> completely when compiled using the MSVC2013 and MSVC2015 (since these
> releases use C99's bools but without stdbool.h like C++).
> I don't understand why the patch has not been commited yet. It seems to me
> not so important !! or ! = 0, the solution is all that matters.

+1 for applying it. There were some conflicts in gist and lwlock, that
I fixed as per the attached. I have added as well an entry in next CF:
https://commitfest.postgresql.org/9/507/
If a committer wants to pick up that before, feel free. For now it
won't fall in the void, that's better than nothing.
--
Michael

On Tue, Feb 9, 2016 at 11:53 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Feb 10, 2016 at 1:17 AM, Yury Zhuravlev
> <u(dot)zhuravlev(at)postgrespro(dot)ru> wrote:
>> I've just run into a problem with these macro. Function ginStepRight breaks
>> completely when compiled using the MSVC2013 and MSVC2015 (since these
>> releases use C99's bools but without stdbool.h like C++).
>> I don't understand why the patch has not been commited yet. It seems to me
>> not so important !! or ! = 0, the solution is all that matters.
>
> +1 for applying it. There were some conflicts in gist and lwlock, that
> I fixed as per the attached. I have added as well an entry in next CF:
> https://commitfest.postgresql.org/9/507/
> If a committer wants to pick up that before, feel free. For now it
> won't fall in the void, that's better than nothing.

Not actually attached.

Are we thinking to back-patch this? I would be disinclined to
back-patch widespread changes like this. If there's a specific
instance related to Gin where this is causing a tangible problem, we
could back-patch just that part, with a clear description of that
problem. Otherwise, I think this should be master-only.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-02-11 08:50:41 -0500, Robert Haas wrote:
> Are we thinking to back-patch this? I would be disinclined to
> back-patch widespread changes like this. If there's a specific
> instance related to Gin where this is causing a tangible problem, we
> could back-patch just that part, with a clear description of that
> problem. Otherwise, I think this should be master-only.

I'm not sure. It's pretty darn nasty that right now we fail in some
places in the code if stdbool.h is included previously. That's probably
going to become more and more common. On the other hand it's invasive,
as you say. Partially patching things doesn't seem like a really viable
approach to me, bugs caused by this are hard to find/trigger.

- Andres

On Thu, Feb 11, 2016 at 9:15 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-02-11 08:50:41 -0500, Robert Haas wrote:
>> Are we thinking to back-patch this? I would be disinclined to
>> back-patch widespread changes like this. If there's a specific
>> instance related to Gin where this is causing a tangible problem, we
>> could back-patch just that part, with a clear description of that
>> problem. Otherwise, I think this should be master-only.
>
> I'm not sure. It's pretty darn nasty that right now we fail in some
> places in the code if stdbool.h is included previously. That's probably
> going to become more and more common. On the other hand it's invasive,
> as you say. Partially patching things doesn't seem like a really viable
> approach to me, bugs caused by this are hard to find/trigger.

I have never been quite clear on what you think is going to cause
stdbool.h inclusions to become more common.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-02-11 09:51:26 -0500, Robert Haas wrote:
> I have never been quite clear on what you think is going to cause
> stdbool.h inclusions to become more common.

Primarily because it's finally starting to be supported across the
board, thanks to MS finally catching up.

Then there's uglyness like:
http://archives.postgresql.org/message-id/508bd3a0-b0a1-4f10-ad9c-2f86656f6e79%40postgrespro.ru

On Thu, Feb 11, 2016 at 9:54 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-02-11 09:51:26 -0500, Robert Haas wrote:
>> I have never been quite clear on what you think is going to cause
>> stdbool.h inclusions to become more common.
>
> Primarily because it's finally starting to be supported across the
> board, thanks to MS finally catching up.
>
> Then there's uglyness like:
> http://archives.postgresql.org/message-id/508bd3a0-b0a1-4f10-ad9c-2f86656f6e79%40postgrespro.ru

So, is it being pulled in indirectly by some other #include?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> So, is it being pulled in indirectly by some other #include?

I can double-check it tomorrow.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2016-02-11 09:51:26 -0500, Robert Haas wrote:
>> I have never been quite clear on what you think is going to cause
>> stdbool.h inclusions to become more common.

> Primarily because it's finally starting to be supported across the
> board, thanks to MS finally catching up.

There are exactly 0 references to stdbool in our source tree. The
only way it'd get pulled in is if some other system header does so.
(Which seems possible, admittedly. I'm not sure whether the C
standard allows or forbids such things.)

It's worth worrying also about extensions that might want to include
stdbool.h --- anything in our own headers that would conflict would
be a problem. But I'm unconvinced that we need to make our .c files
prepared for stdbool.h to be #included in them. By that argument,
any random symbol in any system header in any platform on the planet
is a hazard to us.

regards, tom lane

On 2016-02-11 13:06:14 -0500, Tom Lane wrote:
> But I'm unconvinced that we need to make our .c files prepared for
> stdbool.h to be #included in them.

The fixes, besides two stylistic edits around !! use, are all in
headers. The issue is that we return things meant to be used in a
boolean context, that's not just 0/1 but some random set bit. Looking
over the (outdated) patch attached to
http://archives.postgresql.org/message-id/20150812161140.GD25424%40awork2.anarazel.de
it's not completely outlandish that one wants to use one of these
functions, but also something that uses stdbool.h.

> By that argument, any random
> symbol in any system header in any platform on the planet is a hazard
> to us.

Don't think that's really the same. C99's booleans are part of the
standard, and have surprising behaviour that you can't get on the C
level (they always contain 1/0 after assignment). That makes for more
likely and more subtle bugs.

And anyway, these macros are a potential issue even without stdbool.h
style booleans. If you compare them using equality, you can get into
trouble...

Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> And anyway, these macros are a potential issue even without stdbool.h
> style booleans.

Absolutely; they don't work safely for testing bits that aren't in the
rightmost byte of a flag word, for instance. I'm on board with making
these fixes, I'm just unconvinced that stdbool is a good reason for it.

regards, tom lane

On 2016-02-11 13:37:17 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > And anyway, these macros are a potential issue even without stdbool.h
> > style booleans.
>
> Absolutely; they don't work safely for testing bits that aren't in the
> rightmost byte of a flag word, for instance. I'm on board with making
> these fixes, I'm just unconvinced that stdbool is a good reason for it.

Oh, ok. Interactions with stdbool was what made me looking into this,
that's primarily why I mentioned it. What's your thinking about
back-patching, independent of that then?

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2016-02-11 13:37:17 -0500, Tom Lane wrote:
>> Absolutely; they don't work safely for testing bits that aren't in the
>> rightmost byte of a flag word, for instance. I'm on board with making
>> these fixes, I'm just unconvinced that stdbool is a good reason for it.

> Oh, ok. Interactions with stdbool was what made me looking into this,
> that's primarily why I mentioned it. What's your thinking about
> back-patching, independent of that then?

Well, Yury was saying upthread that some MSVC versions have a problem
with the existing coding, which would be a reason to back-patch ...
but I'd like to see a failing buildfarm member first. Don't particularly
want to promise to support compilers not represented in the farm.

regards, tom lane

On Fri, Feb 12, 2016 at 3:45 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
>> On 2016-02-11 13:37:17 -0500, Tom Lane wrote:
>>> Absolutely; they don't work safely for testing bits that aren't in the
>>> rightmost byte of a flag word, for instance. I'm on board with making
>>> these fixes, I'm just unconvinced that stdbool is a good reason for it.
>
>> Oh, ok. Interactions with stdbool was what made me looking into this,
>> that's primarily why I mentioned it. What's your thinking about
>> back-patching, independent of that then?
>
> Well, Yury was saying upthread that some MSVC versions have a problem
> with the existing coding, which would be a reason to back-patch ...
> but I'd like to see a failing buildfarm member first. Don't particularly
> want to promise to support compilers not represented in the farm.

Grmbl. Forgot to attach the rebased patch upthread. Here is it now.

As of now the only complain has been related to MS2015 and MS2013. If
we follow the pattern of cec8394b and [1], support to compile on newer
versions of MSVC would be master and REL9_5_STABLE, but MS2013 is
supported down to 9.3. Based on this reason, we would want to
backpatch down to 9.3 the patch of this thread.

[1]: http://www.postgresql.org/message-id/529D05CC.7070806@gmx.de
--
Michael

Yury Zhuravlev wrote:
> Robert Haas wrote:
>> So, is it being pulled in indirectly by some other #include?
>
> I can double-check it tomorrow.
>

I've found who include stdbool.h and think it is inevitable for MSVC2013
and MSVC2015.
In port/win32.h we inlcude process.h.
In process.h included corecrt_startup.h.
In corecrt_startup.h included vcruntime_startup.h.
In vcruntime_startup.h we included stdbool.h tadam!

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Thu, Feb 11, 2016 at 9:30 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Feb 12, 2016 at 3:45 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Andres Freund <andres(at)anarazel(dot)de> writes:
>>> On 2016-02-11 13:37:17 -0500, Tom Lane wrote:
>>>> Absolutely; they don't work safely for testing bits that aren't in the
>>>> rightmost byte of a flag word, for instance. I'm on board with making
>>>> these fixes, I'm just unconvinced that stdbool is a good reason for it.
>>
>>> Oh, ok. Interactions with stdbool was what made me looking into this,
>>> that's primarily why I mentioned it. What's your thinking about
>>> back-patching, independent of that then?
>>
>> Well, Yury was saying upthread that some MSVC versions have a problem
>> with the existing coding, which would be a reason to back-patch ...
>> but I'd like to see a failing buildfarm member first. Don't particularly
>> want to promise to support compilers not represented in the farm.
>
> Grmbl. Forgot to attach the rebased patch upthread. Here is it now.
>
> As of now the only complain has been related to MS2015 and MS2013. If
> we follow the pattern of cec8394b and [1], support to compile on newer
> versions of MSVC would be master and REL9_5_STABLE, but MS2013 is
> supported down to 9.3. Based on this reason, we would want to
> backpatch down to 9.3 the patch of this thread.

OK, that seems reasonable from here. What I'm still fuzzy about is
why including stdbool.h causes a failure. Is it because it defines a
type called "bool" that clashes with ours? That seems like the
obvious explanation, but then how does that not cause the compiler to
just straight-up error out?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> OK, that seems reasonable from here. What I'm still fuzzy about is
> why including stdbool.h causes a failure. Is it because it defines a
> type called "bool" that clashes with ours? That seems like the
> obvious explanation, but then how does that not cause the compiler to
> just straight-up error out?

stdbool.h defines the '_Bool' type. The standard says:

C99 and C11 §6.3.1.2/1 “When any scalar value is converted to _Bool, the
result is 0 if the value compares equal to 0; otherwise, the result is 1.”

It seems that MSVC's bool (_Bool) assignment complies to the standard.

--
Dmitry Ivanov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

On 2016-02-12 07:59:06 -0500, Robert Haas wrote:
> OK, that seems reasonable from here. What I'm still fuzzy about is
> why including stdbool.h causes a failure. Is it because it defines a
> type called "bool" that clashes with ours? That seems like the
> obvious explanation, but then how does that not cause the compiler to
> just straight-up error out?

No, that's not the problem. Our own definition is actually
conditional. The problem is that the standard's booleans behave
differently. They only ever contain 0 or 1, even if you assign something
outside of that range - essentially they store !!(right-hand-side). If
you know compare a boolean with the values returned by one of these
macros you'll get into problems.

E.g. if you include stdbool.h:
Buffer
ginStepRight(Buffer buffer, Relation index, int lockmode)
{
bool isLeaf = GinPageIsLeaf(page);
bool isData = GinPageIsData(page);
...
if (isLeaf != GinPageIsLeaf(page) || isData != GinPageIsData(page))
elog(ERROR, "right sibling of GIN page is of different type");

doesn't work correctly because isLeaf/isData contain only 0/1 (due to
the standard's bool behaviour), but the values they're compared to
returns some bit set. Due to integer promotion rules isLeaf is promoted
to an integer and compared... And thus the tests fail.

Andres

On Fri, Feb 12, 2016 at 8:48 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-02-12 07:59:06 -0500, Robert Haas wrote:
>> OK, that seems reasonable from here. What I'm still fuzzy about is
>> why including stdbool.h causes a failure. Is it because it defines a
>> type called "bool" that clashes with ours? That seems like the
>> obvious explanation, but then how does that not cause the compiler to
>> just straight-up error out?
>
> No, that's not the problem. Our own definition is actually
> conditional. The problem is that the standard's booleans behave
> differently. They only ever contain 0 or 1, even if you assign something
> outside of that range - essentially they store !!(right-hand-side). If
> you know compare a boolean with the values returned by one of these
> macros you'll get into problems.
>
> E.g. if you include stdbool.h:
> Buffer
> ginStepRight(Buffer buffer, Relation index, int lockmode)
> {
> bool isLeaf = GinPageIsLeaf(page);
> bool isData = GinPageIsData(page);
> ...
> if (isLeaf != GinPageIsLeaf(page) || isData != GinPageIsData(page))
> elog(ERROR, "right sibling of GIN page is of different type");
>
> doesn't work correctly because isLeaf/isData contain only 0/1 (due to
> the standard's bool behaviour), but the values they're compared to
> returns some bit set. Due to integer promotion rules isLeaf is promoted
> to an integer and compared... And thus the tests fail.

Ah-ha. OK, now I get it. So then I agree we should back-patch this
at least as far as 9.3 where MSVC 2013 became a supported platform,
per Michael's remarks, and maybe all the way. Do you want to do that?
If not, I'll do it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Feb 12, 2016 at 8:48 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> E.g. if you include stdbool.h [ ginStepRight breaks ]

> Ah-ha. OK, now I get it. So then I agree we should back-patch this
> at least as far as 9.3 where MSVC 2013 became a supported platform,

Um, no, that does not follow. The unanswered question here is why,
when we *have not* included stdbool.h and *have* typedef'd bool as
just plain "char", we would get C99 bool behavior. There is something
happening there that should not be happening, and I'm not really satisfied
with the explanation "Microsoft is brain-dead as usual". I think we
should dig deeper, because whatever is going on there may have deeper
effects than we now realize.

regards, tom lane

On Fri, Feb 12, 2016 at 9:39 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Feb 12, 2016 at 8:48 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>> E.g. if you include stdbool.h [ ginStepRight breaks ]
>
>> Ah-ha. OK, now I get it. So then I agree we should back-patch this
>> at least as far as 9.3 where MSVC 2013 became a supported platform,
>
> Um, no, that does not follow. The unanswered question here is why,
> when we *have not* included stdbool.h and *have* typedef'd bool as
> just plain "char", we would get C99 bool behavior. There is something
> happening there that should not be happening, and I'm not really satisfied
> with the explanation "Microsoft is brain-dead as usual". I think we
> should dig deeper, because whatever is going on there may have deeper
> effects than we now realize.

http://www.postgresql.org/message-id/d2106c2d-0f46-4cf9-af27-54f81ef6e20c@postgrespro.ru
seems to explain what happens pretty clearly. We #include something
which #includes something which #includes something which #includes
<stdbool.h>. It's not that surprising, is it? I mean, things with
"std" in the name figure to be commonly-included.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-02-12 09:39:20 -0500, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Fri, Feb 12, 2016 at 8:48 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >> E.g. if you include stdbool.h [ ginStepRight breaks ]
>
> > Ah-ha. OK, now I get it. So then I agree we should back-patch this
> > at least as far as 9.3 where MSVC 2013 became a supported platform,
>
> Um, no, that does not follow.

Well, these headers are generally buggy, so ...

> The unanswered question here is why,
> when we *have not* included stdbool.h and *have* typedef'd bool as
> just plain "char", we would get C99 bool behavior. There is something
> happening there that should not be happening, and I'm not really satisfied
> with the explanation "Microsoft is brain-dead as usual". I think we
> should dig deeper, because whatever is going on there may have deeper
> effects than we now realize.

Well,
http://archives.postgresql.org/message-id/d2106c2d-0f46-4cf9-af27-54f81ef6e20c%40postgrespro.ru
outlines how stdbool.h gets included. That happens fairly at the
begining of c.h. Later our definitions are guarded by ifdefs:

#ifndef bool
typedef char bool;
#endif

#ifndef true
#define true ((bool) 1)
#endif

#ifndef false
#define false ((bool) 0)
#endif

So we can lament that MS standard libraries include stdbool.h instead of
using _Bool. But I doubt that's going to buy us super much.

Btw, there's a distinct advantage including stdbool: Compilers actually
generate a fair bit better error messages/warnings in some cases. And
the generated code sometimes is a bit better, too.

Andres

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Feb 12, 2016 at 9:39 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Um, no, that does not follow. The unanswered question here is why,
>> when we *have not* included stdbool.h and *have* typedef'd bool as
>> just plain "char", we would get C99 bool behavior.

> http://www.postgresql.org/message-id/d2106c2d-0f46-4cf9-af27-54f81ef6e20c@postgrespro.ru
> seems to explain what happens pretty clearly. We #include something
> which #includes something which #includes something which #includes
> <stdbool.h>. It's not that surprising, is it?

Well, the thing that is scaring me here is allowing a platform-specific
definition of "bool" to be adopted. If, for example, the compiler
writer decided that that should be int width rather than char width,
all hell would break loose.

regards, tom lane

Tom Lane wrote:
> Well, the thing that is scaring me here is allowing a platform-specific
> definition of "bool" to be adopted.

You say that something strange. bool from stdbool.h is C99 standart.
A different behavior would be regarded as a compiler error.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2016-02-12 09:47:47 -0500, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Fri, Feb 12, 2016 at 9:39 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Um, no, that does not follow. The unanswered question here is why,
> >> when we *have not* included stdbool.h and *have* typedef'd bool as
> >> just plain "char", we would get C99 bool behavior.
>
> > http://www.postgresql.org/message-id/d2106c2d-0f46-4cf9-af27-54f81ef6e20c@postgrespro.ru
> > seems to explain what happens pretty clearly. We #include something
> > which #includes something which #includes something which #includes
> > <stdbool.h>. It's not that surprising, is it?
>
> Well, the thing that is scaring me here is allowing a platform-specific
> definition of "bool" to be adopted. If, for example, the compiler
> writer decided that that should be int width rather than char width,
> all hell would break loose.

Well, for some reason c.h has been written to allow for that for a long
time. I think it's fairly unlikely that somebody writes a _Bool
implementation where sizeof(_Bool) is bigger than sizeof(char). Although
that'd be, by my reading of the standard. permissible. It just says
6.2.5-2: An object declared as type _Bool is large enough to store the
values 0 and 1.
6.7.2.1: While the number of bits in a _Bool object is at least
CHAR_BIT, the width (number of sign and value bits) of a _Bool
may be just 1 bit.

afaics that's pretty much all said about the size of _Bool, except some
bitfield special cases.

But we also only support e.g. CHAR_BIT = 8, so I'm not super concerned
about _Bool being defined too wide.

Andres

On Fri, Feb 12, 2016 at 9:47 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Feb 12, 2016 at 9:39 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Um, no, that does not follow. The unanswered question here is why,
>>> when we *have not* included stdbool.h and *have* typedef'd bool as
>>> just plain "char", we would get C99 bool behavior.
>
>> http://www.postgresql.org/message-id/d2106c2d-0f46-4cf9-af27-54f81ef6e20c@postgrespro.ru
>> seems to explain what happens pretty clearly. We #include something
>> which #includes something which #includes something which #includes
>> <stdbool.h>. It's not that surprising, is it?
>
> Well, the thing that is scaring me here is allowing a platform-specific
> definition of "bool" to be adopted. If, for example, the compiler
> writer decided that that should be int width rather than char width,
> all hell would break loose.

That's true, but it doesn't really seem like a reason not to commit
this patch. I mean, the coding here is (a) dangerous by your own
admission and (b) actually breaks on platforms for which we allege
support. If we find out that somebody has implemented an int-width
bool we'll have some bigger decisions to make, but I don't see any
particular reason why we've got to make those decisions now.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

One more option for patch:

#define GinPageIsLeaf(page) ((bool)(GinPageGetOpaque(page)->flags & GIN_LEAF))

Seems it will work on any platform with built-in bool. But I don't know will it
work with 'typedef char bool' if high bit will be set.

> That's true, but it doesn't really seem like a reason not to commit
> this patch. I mean, the coding here is (a) dangerous by your own
> admission and (b) actually breaks on platforms for which we allege
> support. If we find out that somebody has implemented an int-width
> bool we'll have some bigger decisions to make, but I don't see any
> particular reason why we've got to make those decisions now.

+1

--
Teodor Sigaev E-mail: teodor(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/

Teodor Sigaev <teodor(at)sigaev(dot)ru> writes:
> One more option for patch:
> #define GinPageIsLeaf(page) ((bool)(GinPageGetOpaque(page)->flags & GIN_LEAF))

I think that's a seriously bad coding pattern to adopt, because it would
work for some people but not others if the flag bit is to the left of the
rightmost byte. We should standardize on the "((var & FLAG) != 0)"
pattern, which works reliably in all cases.

The pattern "(!!(var & FLAG))" would work too, but I dislike it because
it is not "say what you mean" but more of a cute coding trick to save a
keystroke or two. People who aren't longtime C coders would have to stop
and think about what it does.

(I'd expect reasonable compilers to generate pretty much the same code
in any of these cases, so that aspect of it shouldn't be an issue.)

regards, tom lane

On February 12, 2016 5:15:59 PM GMT+01:00, Teodor Sigaev <teodor(at)sigaev(dot)ru> wrote:
>One more option for patch:
>
>#define GinPageIsLeaf(page) ((bool)(GinPageGetOpaque(page)->flags &
>GIN_LEAF))
>
>Seems it will work on any platform with built-in bool. But I don't know
>will it
>work with 'typedef char bool' if high bit will be set.

Unless I am missing something major, that doesn't seem to achieve all that much. A cast to a char based bool wouldn't normalize this to 0 or 1. So you're still not guaranteed to be able to do somebool == anotherbool when either are set based on such a macro.

Andres

---
Please excuse brevity and formatting - I am writing this on my mobile phone.

On February 12, 2016 5:29:44 PM GMT+01:00, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> We should standardize on the "((var & FLAG) != 0)"
>pattern, which works reliably in all cases.

That's what the second version of my patch, and I presume Michael's updated one as well, does. I think the only open question is how far to backpatch. While annoying work, I think we should go all the way.

Andres

---
Please excuse brevity and formatting - I am writing this on my mobile phone.

Andres Freund wrote:
> Unless I am missing something major, that doesn't seem to
> achieve all that much. A cast to a char based bool wouldn't
> normalize this to 0 or 1. So you're still not guaranteed to be
> able to do somebool == anotherbool when either are set based on
> such a macro.
>

In C99 cast to bool return 0 or 1 only. In older compilers nothing changes
(Now the code is designed to "char == char").
I think this is a good option. But of course to write bool and use char
strange.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On February 12, 2016 5:40:29 PM GMT+01:00, Yury Zhuravlev <u(dot)zhuravlev(at)postgrespro(dot)ru> wrote:
>Andres Freund wrote:
>> Unless I am missing something major, that doesn't seem to
>> achieve all that much. A cast to a char based bool wouldn't
>> normalize this to 0 or 1. So you're still not guaranteed to be
>> able to do somebool == anotherbool when either are set based on
>> such a macro.
>>
>
>In C99 cast to bool return 0 or 1 only.

Don't you say. That's why I brought all this up.

> In older compilers nothing
>changes
>(Now the code is designed to "char == char").
>I think this is a good option. But of course to write bool and use char
>
>strange.

Did you read what I wrote? That's not correct for char booleans, because the can have different bits set.

Andres

---
Please excuse brevity and formatting - I am writing this on my mobile phone.

Andres Freund <andres(at)anarazel(dot)de> writes:
> On February 12, 2016 5:29:44 PM GMT+01:00, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> We should standardize on the "((var & FLAG) != 0)"
>> pattern, which works reliably in all cases.

> That's what the second version of my patch, and I presume Michael's updated one as well, does. I think the only open question is how far to backpatch. While annoying work, I think we should go all the way.

I don't object to that, if someone wants to do the work. A good argument
for it is that we'd otherwise be laying a nasty trap for future
back-patched bug fixes, which might well rely on the cleaned-up behavior.

regards, tom lane

Andres Freund wrote:
> Did you read what I wrote?
Read.

>That's not correct for char booleans, because the can have different bits set.
Current code support this behavior. But to shoot his leg becomes easier.
!= 0 much better of course.

Thanks.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Sat, Feb 13, 2016 at 1:48 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
>> On February 12, 2016 5:29:44 PM GMT+01:00, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> We should standardize on the "((var & FLAG) != 0)"
>>> pattern, which works reliably in all cases.
>
>> That's what the second version of my patch, and I presume Michael's updated one as well, does. I think the only open question is how far to backpatch. While annoying work, I think we should go all the way.
>
> I don't object to that, if someone wants to do the work. A good argument
> for it is that we'd otherwise be laying a nasty trap for future
> back-patched bug fixes, which might well rely on the cleaned-up behavior.

From the MSVC-only perspective, that's down to 9.3, but it would
definitely make sense to backpatch 2 versions further down to
facilitate future bug fix integration, so +1 to get that down to 9.1.
Andres, I guess that you are on that? That's your patch after all.
--
Michael

On 2/11/16 9:30 PM, Michael Paquier wrote:
>> Well, Yury was saying upthread that some MSVC versions have a problem
>> with the existing coding, which would be a reason to back-patch ...
>> but I'd like to see a failing buildfarm member first. Don't particularly
>> want to promise to support compilers not represented in the farm.
>
> Grmbl. Forgot to attach the rebased patch upthread. Here is it now.
>
> As of now the only complain has been related to MS2015 and MS2013. If
> we follow the pattern of cec8394b and [1], support to compile on newer
> versions of MSVC would be master and REL9_5_STABLE, but MS2013 is
> supported down to 9.3. Based on this reason, we would want to
> backpatch down to 9.3 the patch of this thread.

After reviewing this thread and relevant internet lore, I think this
might be the wrong way to address this problem. It is in general not
guaranteed in C that a Boolean-sounding function or macro returns 0 or
1. Prime examples are ctype.h functions such as isupper(). This is
normally not a problem because built-in conditionals such as if, &&, ||
handle this just fine. So code like

- Assert(!create || !!txn);
+ Assert(!create || txn != NULL);

is arguably silly either way. There is no risk in writing just

Assert(!create || txn);

The problem only happens if you compare two "Boolean" values directly
with each other; and so maybe you shouldn't do that, or at least place
the extra care there instead, instead of fighting a permanent battle
with the APIs you're using. (This isn't an outrageous requirement: You
can't compare floats or strings either without extra care.)

A quick look through the code based on the provided patch shows that
approximately the only place affected by this is

if (isLeaf != GinPageIsLeaf(page) || isData != GinPageIsData(page))
elog(ERROR, "right sibling of GIN page is of different type");

and that's not actually a problem because isLeaf and isData are earlier
populated by the same macros. It would still be worth fixing, but a
localized fix seems better.

Now on the matter of stdbool, I tried putting an #include <stdbool.h>
near the top of c.h and compile that to see what would happen. This is
the first warning I see:

ginlogic.c: In function 'shimTriConsistentFn':
ginlogic.c:171:24: error: comparison of constant '2' with boolean
expression is always false [-Werror=bool-compare]
if (key->entryRes[i] == GIN_MAYBE)
^

and then later on something related:

../../../../src/include/tsearch/ts_utils.h:107:13: note: expected '_Bool
(*)(void *, QueryOperand *) {aka _Bool (*)(void *, struct <anonymous>
*)}' but argument is of type 'GinTernaryValue (*)(void *, QueryOperand
*) {aka char (*)(void *, struct <anonymous> *)}'

So the compiler is actually potentially helpful, but as it stands,
PostgreSQL code is liable to break if you end up with stdbool.h somehow.

(plperl also fails to compile because of a hot-potato game about who is
actually responsible for defining bool.)

So one idea would be to actually get ahead of the game, include
stdbool.h if available, fix the mentioned issues, and maybe get more
robust code that way.

But the lore on the internet casts some doubt on that: There is no
guarantee that bool is 1 byte, that bool can be passed around like char,
or even that bool arrays are laid out like char arrays. Maybe this all
works out okay, just like it has worked out so far that int is 4 bytes,
but we don't know enough about it. We could probably add some configure
tests around that.

We could also go the other way and forcibly undefine an existing bool
type (since stdbool.h is supposed to use macros, not typedefs). But
that might not work well if a header that is included later pulls in
stdbool.h on top of that.

My proposal on this particular patch is to do nothing. The stdbool
issues should be looked into, for the sake of Windows and other
future-proofness. But that will likely be an entirely different patch.

On Wed, Mar 2, 2016 at 9:48 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 2/11/16 9:30 PM, Michael Paquier wrote:
>>> Well, Yury was saying upthread that some MSVC versions have a problem
>>> with the existing coding, which would be a reason to back-patch ...
>>> but I'd like to see a failing buildfarm member first. Don't particularly
>>> want to promise to support compilers not represented in the farm.
>>
>> Grmbl. Forgot to attach the rebased patch upthread. Here is it now.
>>
>> As of now the only complain has been related to MS2015 and MS2013. If
>> we follow the pattern of cec8394b and [1], support to compile on newer
>> versions of MSVC would be master and REL9_5_STABLE, but MS2013 is
>> supported down to 9.3. Based on this reason, we would want to
>> backpatch down to 9.3 the patch of this thread.
>
> After reviewing this thread and relevant internet lore, I think this
> might be the wrong way to address this problem. It is in general not
> guaranteed in C that a Boolean-sounding function or macro returns 0 or
> 1. Prime examples are ctype.h functions such as isupper(). This is
> normally not a problem because built-in conditionals such as if, &&, ||
> handle this just fine. So code like
>
> - Assert(!create || !!txn);
> + Assert(!create || txn != NULL);
>
> is arguably silly either way. There is no risk in writing just
>
> Assert(!create || txn);
>
> The problem only happens if you compare two "Boolean" values directly
> with each other; and so maybe you shouldn't do that, or at least place
> the extra care there instead, instead of fighting a permanent battle
> with the APIs you're using. (This isn't an outrageous requirement: You
> can't compare floats or strings either without extra care.)
>
> A quick look through the code based on the provided patch shows that
> approximately the only place affected by this is
>
> if (isLeaf != GinPageIsLeaf(page) || isData != GinPageIsData(page))
> elog(ERROR, "right sibling of GIN page is of different type");
>
> and that's not actually a problem because isLeaf and isData are earlier
> populated by the same macros. It would still be worth fixing, but a
> localized fix seems better.
>
>
> Now on the matter of stdbool, I tried putting an #include <stdbool.h>
> near the top of c.h and compile that to see what would happen. This is
> the first warning I see:
>
> ginlogic.c: In function 'shimTriConsistentFn':
> ginlogic.c:171:24: error: comparison of constant '2' with boolean
> expression is always false [-Werror=bool-compare]
> if (key->entryRes[i] == GIN_MAYBE)
> ^
>
> and then later on something related:
>
> ../../../../src/include/tsearch/ts_utils.h:107:13: note: expected '_Bool
> (*)(void *, QueryOperand *) {aka _Bool (*)(void *, struct <anonymous>
> *)}' but argument is of type 'GinTernaryValue (*)(void *, QueryOperand
> *) {aka char (*)(void *, struct <anonymous> *)}'
>
> So the compiler is actually potentially helpful, but as it stands,
> PostgreSQL code is liable to break if you end up with stdbool.h somehow.
>
> (plperl also fails to compile because of a hot-potato game about who is
> actually responsible for defining bool.)
>
> So one idea would be to actually get ahead of the game, include
> stdbool.h if available, fix the mentioned issues, and maybe get more
> robust code that way.
>
> But the lore on the internet casts some doubt on that: There is no
> guarantee that bool is 1 byte, that bool can be passed around like char,
> or even that bool arrays are laid out like char arrays. Maybe this all
> works out okay, just like it has worked out so far that int is 4 bytes,
> but we don't know enough about it. We could probably add some configure
> tests around that.
>
> We could also go the other way and forcibly undefine an existing bool
> type (since stdbool.h is supposed to use macros, not typedefs). But
> that might not work well if a header that is included later pulls in
> stdbool.h on top of that.
>
>
> My proposal on this particular patch is to do nothing. The stdbool
> issues should be looked into, for the sake of Windows and other
> future-proofness. But that will likely be an entirely different patch.

We need to decide what to do about this. I disagree with Peter: I
think that regardless of stdbool, what we've got right now is sloppy
coding - bad style if nothing else. Furthermore, I think that while C
lets you use any non-zero value to represent true, our bool type is
supposed to contain only one of those two values. Therefore, I think
we should commit the full patch, back-patch it as far as somebody has
the energy for, and move on. But regardless, this patch can't keep
sitting in the CommitFest - we either have to take it or reject it,
and soon.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Mar 10, 2016 at 11:40 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> We need to decide what to do about this. I disagree with Peter: I
> think that regardless of stdbool, what we've got right now is sloppy
> coding - bad style if nothing else. Furthermore, I think that while C
> lets you use any non-zero value to represent true, our bool type is
> supposed to contain only one of those two values. Therefore, I think
> we should commit the full patch, back-patch it as far as somebody has
> the energy for, and move on. But regardless, this patch can't keep
> sitting in the CommitFest - we either have to take it or reject it,
> and soon.

+1, I would suggest to move ahead, !! is not really Postgres-like anyway.
--
Michael

On 2016-03-02 21:48:16 -0500, Peter Eisentraut wrote:
> After reviewing this thread and relevant internet lore, I think this
> might be the wrong way to address this problem. It is in general not
> guaranteed in C that a Boolean-sounding function or macro returns 0 or
> 1. Prime examples are ctype.h functions such as isupper(). This is
> normally not a problem because built-in conditionals such as if, &&, ||
> handle this just fine. So code like
>
> - Assert(!create || !!txn);
> + Assert(!create || txn != NULL);
>
> is arguably silly either way. There is no risk in writing just
>
> Assert(!create || txn);

Yes, obviously. I doubt that anyone misunderstood that. I personally
find the !! easier to read when contrasting to a negated value (as in
the above assert).

> The problem only happens if you compare two "Boolean" values directly
> with each other;

That's not correct. It also happen if you compare an expression with a
stored version, i.e. only one side is a 'proper bool'.

> A quick look through the code based on the provided patch shows that
> approximately the only place affected by this is
>
> if (isLeaf != GinPageIsLeaf(page) || isData != GinPageIsData(page))
> elog(ERROR, "right sibling of GIN page is of different type");
>
> and that's not actually a problem because isLeaf and isData are earlier
> populated by the same macros. It would still be worth fixing, but a
> localized fix seems better.

That's maybe the only place where we compare stored booleans to
expressions, but it's definitely not the only place where some
expression is stored in a boolean variable. And in all those cases
there's an int32 (or whatever type the expression has) to bool (usually
1byte char). That's definitely problematic.

> But the lore on the internet casts some doubt on that: There is no
> guarantee that bool is 1 byte, that bool can be passed around like char,
> or even that bool arrays are laid out like char arrays. Maybe this all
> works out okay, just like it has worked out so far that int is 4 bytes,
> but we don't know enough about it. We could probably add some configure
> tests around that.

So?

> My proposal on this particular patch is to do nothing. The stdbool
> issues should be looked into, for the sake of Windows and other
> future-proofness. But that will likely be an entirely different patch.

That seems to entirely miss the part of this discussion dealing with
high bits set and such?

Greetings,

Andres Freund

On 2016-03-11 04:50:45 +0100, Michael Paquier wrote:
> On Thu, Mar 10, 2016 at 11:40 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > We need to decide what to do about this. I disagree with Peter: I
> > think that regardless of stdbool, what we've got right now is sloppy
> > coding - bad style if nothing else. Furthermore, I think that while C
> > lets you use any non-zero value to represent true, our bool type is
> > supposed to contain only one of those two values. Therefore, I think
> > we should commit the full patch, back-patch it as far as somebody has
> > the energy for, and move on. But regardless, this patch can't keep
> > sitting in the CommitFest - we either have to take it or reject it,
> > and soon.

I plan to commit something like this, unless there's very loud protest
from Peter's side.

> +1, I would suggest to move ahead, !! is not really Postgres-like anyway.

The !! bit is a minor sideshow to this, afaics. It just came up when
discussing the specifics of the fixed macros and some people expressed a
clear preference for not using !!, so I fixed the occurrances I
introduced.

Andres

Andres Freund wrote:
> I plan to commit something like this, unless there's very loud protest
> from Peter's side.

I agree. Peter proposal can be considered in a separate thread.

Thanks.
--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Robert Haas wrote:
> On Wed, Mar 2, 2016 at 9:48 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> On 2/11/16 9:30 PM, Michael Paquier wrote: ...
>
> We need to decide what to do about this. I disagree with Peter: I
> think that regardless of stdbool, what we've got right now is sloppy
> coding - bad style if nothing else. Furthermore, I think that while C
> lets you use any non-zero value to represent true, our bool type is
> supposed to contain only one of those two values. Therefore, I think
> we should commit the full patch, back-patch it as far as somebody has
> the energy for, and move on. But regardless, this patch can't keep
> sitting in the CommitFest - we either have to take it or reject it,
> and soon.
>

I know that we are trying to do the right thing. But right now there is an
error only in ginStepRight. Maybe now the fix this place, and we will think
about "bool" then? The patch is attached (small and simple).

Thanks.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Fri, Mar 18, 2016 at 8:36 PM, Yury Zhuravlev
<u(dot)zhuravlev(at)postgrespro(dot)ru> wrote:
> Robert Haas wrote:
>>
>> On Wed, Mar 2, 2016 at 9:48 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>>>
>>> On 2/11/16 9:30 PM, Michael Paquier wrote: ...
>>
>>
>> We need to decide what to do about this. I disagree with Peter: I
>> think that regardless of stdbool, what we've got right now is sloppy
>> coding - bad style if nothing else. Furthermore, I think that while C
>> lets you use any non-zero value to represent true, our bool type is
>> supposed to contain only one of those two values. Therefore, I think
>> we should commit the full patch, back-patch it as far as somebody has
>> the energy for, and move on. But regardless, this patch can't keep
>> sitting in the CommitFest - we either have to take it or reject it,
>> and soon.
>>
>
> I know that we are trying to do the right thing. But right now there is an
> error only in ginStepRight. Maybe now the fix this place, and we will think
> about "bool" then? The patch is attached (small and simple).

FWIW, when compiling with MS 2015 using the set of perl scripts I am
not seeing this compilation error... We may want to understand first
what kind of dependency is involved when doing the cmake build
compared to what is done with src/tools/msvc.
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> FWIW, when compiling with MS 2015 using the set of perl scripts I am
> not seeing this compilation error... We may want to understand first
> what kind of dependency is involved when doing the cmake build
> compared to what is done with src/tools/msvc.

That is strange ... unless maybe cmake is supplying a different set
of warning-enabling switches to the compiler?

regards, tom lane

On Thu, Mar 10, 2016 at 11:00 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-03-11 04:50:45 +0100, Michael Paquier wrote:
>> On Thu, Mar 10, 2016 at 11:40 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> > We need to decide what to do about this. I disagree with Peter: I
>> > think that regardless of stdbool, what we've got right now is sloppy
>> > coding - bad style if nothing else. Furthermore, I think that while C
>> > lets you use any non-zero value to represent true, our bool type is
>> > supposed to contain only one of those two values. Therefore, I think
>> > we should commit the full patch, back-patch it as far as somebody has
>> > the energy for, and move on. But regardless, this patch can't keep
>> > sitting in the CommitFest - we either have to take it or reject it,
>> > and soon.
>
> I plan to commit something like this, unless there's very loud protest
> from Peter's side.

So, can we get on with that, then?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Michael Paquier wrote:
> FWIW, when compiling with MS 2015 using the set of perl scripts I am
> not seeing this compilation error...

This error not in build stage but in GIN regresion tests. CMake nothing to
do with.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Sat, Mar 19, 2016 at 12:08 AM, Yury Zhuravlev
<u(dot)zhuravlev(at)postgrespro(dot)ru> wrote:
> Michael Paquier wrote:
>>
>> FWIW, when compiling with MS 2015 using the set of perl scripts I am
>> not seeing this compilation error...
>
> This error not in build stage but in GIN regresion tests. CMake nothing to
> do with.

Bingo: "right sibling of GIN page is of different type", and gin is
not the only test to fail, there are 4 tests failing including gin,
and all the failures are caused by that. So yes, we had better fix it.
--
Michael

On 2016-03-18 14:36:23 +0300, Yury Zhuravlev wrote:
> Robert Haas wrote:
> >On Wed, Mar 2, 2016 at 9:48 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> >>On 2/11/16 9:30 PM, Michael Paquier wrote: ...
> >
> >We need to decide what to do about this. I disagree with Peter: I
> >think that regardless of stdbool, what we've got right now is sloppy
> >coding - bad style if nothing else. Furthermore, I think that while C
> >lets you use any non-zero value to represent true, our bool type is
> >supposed to contain only one of those two values. Therefore, I think
> >we should commit the full patch, back-patch it as far as somebody has
> >the energy for, and move on. But regardless, this patch can't keep
> >sitting in the CommitFest - we either have to take it or reject it,
> >and soon.
> >
>
> I know that we are trying to do the right thing. But right now there is an
> error only in ginStepRight. Maybe now the fix this place, and we will think
> about "bool" then? The patch is attached (small and simple).
>
> Thanks.
>
>
> --
> Yury Zhuravlev
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company

> diff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c
> index 06ba9cb..30113d0 100644
> --- a/src/backend/access/gin/ginbtree.c
> +++ b/src/backend/access/gin/ginbtree.c
> @@ -162,8 +162,8 @@ ginStepRight(Buffer buffer, Relation index, int lockmode)
> {
> Buffer nextbuffer;
> Page page = BufferGetPage(buffer);
> - bool isLeaf = GinPageIsLeaf(page);
> - bool isData = GinPageIsData(page);
> + uint8 isLeaf = GinPageIsLeaf(page);
> + uint8 isData = GinPageIsData(page);
> BlockNumber blkno = GinPageGetOpaque(page)->rightlink;
>
> nextbuffer = ReadBuffer(index, blkno);

I've pushed the gin specific stuff (although I fixed the macros instead
of the callsites) to all branches. I plan to commit the larger patch
(which has grown since last posting it) after the minor releases; it's
somewhat large and has a lot of conflicts. This way at least the
confirmed issue is fixed in the next set of minor releases.

Greetings,

Andres Freund