Re: taking stdbool.h into use

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> Some not so long time ago, it was discussed to look into taking
> stdbool.h into use. The reason was that third-party libraries (perl?,
> ldap?, postgis?) are increasingly doing so, and having incompatible
> definitions of bool could/does create a mess.

> Here is a patch set that aims to accomplish that. On the way there, it
> cleans up various loose and weird uses of bool and proposes a way to
> ensure that the system catalog structs get a 1-byte bool even if the
> "standard" bool is not.

I played around with this on my dinosaur machines.

On gaur, every source file gives me a complaint like this:

In file included from ../../src/include/postgres.h:47,
from username.c:16:
../../src/include/c.h:207: warning: `SIZEOF__BOOL' redefined
../../src/include/pg_config.h:801: warning: this is the location of the previous definition

pg_config.h contains:

/* The size of `_Bool', as computed by sizeof. */
#define SIZEOF__BOOL 0

c.h then attempts to override that, which would be bad style in any case.
I think you should make configure take care of such situations and emit
the correct SIZEOF__BOOL value to begin with. Perhaps the script could
check for a zero result and change it to 1.

Note that even though this platform has a <stdbool.h>, configure rejects
it because the name "bool" is not a macro. Dunno if we care to change
that; I see we're using a standard autoconf macro, so messing with that
is likely more trouble than it's worth.

After temporarily commenting out the bogus SIZEOF__BOOL definition,
I got a clean compile and clean regression tests. That's not too
surprising though because without use of <stdbool.h> it's effectively
equivalent to the old code.

BTW, I also wonder why 0008 is doing

AC_CHECK_SIZEOF(_Bool)
and then
#define SIZEOF_BOOL SIZEOF__BOOL

rather than just

AC_CHECK_SIZEOF(bool)

I should think that not touching _Bool when we don't have to is a
good thing.

On prairiedog, configure seems to detect things correctly:

$ grep BOOL src/include/pg_config.h
#define HAVE_STDBOOL_H 1
#define HAVE__BOOL 1
#define SIZEOF__BOOL 4

It builds without warnings, but the regression tests crash:

2017-08-24 16:53:42.621 EDT [24029] LOG: server process (PID 24311) was terminated by signal 10: Bus error
2017-08-24 16:53:42.621 EDT [24029] DETAIL: Failed process was running: CREATE INDEX textarrayidx ON array_index_op_test USING gin (t);

The core dump is a bit confused, but it seems to be trying to dereference
a null pointer in bttextcmp. I'm pretty sure the underlying problem is
that you've not done anything with GinNullCategory:

/*
* Category codes to distinguish placeholder nulls from ordinary NULL keys.
* Note that the datatype size and the first two code values are chosen to be
* compatible with the usual usage of bool isNull flags.
*
* GIN_CAT_EMPTY_QUERY is never stored in the index; and notice that it is
* chosen to sort before not after regular key values.
*/
typedef signed char GinNullCategory;

Overlaying that with "bool" is just Not Gonna Work. It also ain't gonna
work to do "typedef bool GinNullCategory", so I'm not very sure how to
resolve that. Maybe we could hack it like

#if SIZEOF__BOOL == 1
typedef signed char GinNullCategory;
#elif SIZEOF__BOOL == 4
typedef int32 GinNullCategory;
#else
#error "unsupported sizeof(bool)"
#endif

However, the quoted comment implies that we store GinNullCategory values
on-disk, which might mean that some additional hacks are needed to
preserve index compatibility.

> I have done a fair amount of testing on this, including a hand-rigged
> setup where _Bool is not 1 byte.

I'm not very sure how you got through regression tests despite this issue.
Possibly it's got something to do with prairiedog being an alignment-picky
machine ... but the bus error is *not* at a spot where a bool or
GinNullCategory value is being accessed, so the problem seems like it
should manifest with jury-rigged _Bool on non-picky hardware as well.

regards, tom lane