Thread

Commits

Use stdbool.h if suitable
- 9a95a77d9d5d 11.0 landed
Add configure tests for stdbool.h and sizeof bool
- f20b3285340c 11.0 landed
Remove useless use of bit-masking macros
- d91da5ecedc8 11.0 landed
Change various Gin*Is* macros to return 0/1.
- af4472bcb88a 9.6.0 cited
Fix several possibly non-portable gaffs in record_image_ops.
- 4cbb646334b3 9.4.0 cited

taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-08-16T04:36:09Z

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 have done a fair amount of testing on this, including a hand-rigged
setup where _Bool is not 1 byte.  But obviously, more and wider testing
would be very useful.

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

Re: taking stdbool.h into use

Thomas Munro <thomas.munro@enterprisedb.com> — 2017-08-16T06:32:47Z

On Wed, Aug 16, 2017 at 4:36 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> 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 have done a fair amount of testing on this, including a hand-rigged
> setup where _Bool is not 1 byte.  But obviously, more and wider testing
> would be very useful.

Getting out of the way of C99 "bool" makes sense.  It is old enough to
vote.  On my system the tests pass at top level and tcl, plperl,
plpython after each patch is applied, when configured with:

  --enable-debug --enable-depend --enable-cassert --with-icu --with-python
  --with-perl --with-tcl --with-icu --with-ldap

However my system has sizeof(bool) == 1 and so do all the systems I
have access to (x86 + POWER).  Where can we find a computer with
sizeof(bool) == 4?  According to the intertubes OSX on POWER and
Windows 32 bit systems had that in ancient prehistory but they don't
now.

> 0001-Fix-bool-int-type-confusion.patch

Looks good.

> 0002-Change-TRUE-FALSE-to-true-false.patch

Looks good.  What about these?

src/backend/port/win32/crashdump.c:             ExInfo.ClientPointers = FALSE;
src/backend/port/win32/signal.c:        pgwin32_signal_event =
CreateEvent(NULL, TRUE, FALSE, NULL);
src/backend/port/win32/signal.c:
SleepEx(500, FALSE);
src/backend/port/win32/signal.c:        return FALSE;
src/backend/port/win32/socket.c:                waitevent =
CreateEvent(NULL, TRUE, FALSE, NULL);
src/backend/port/win32/socket.c:                        r =
WaitForMultipleObjectsEx(2, events, FALSE, 100, TRUE);
src/backend/port/win32/socket.c:                r =
WaitForMultipleObjectsEx(2, events, FALSE, timeout, TRUE);
src/backend/port/win32/socket.c:        r =
WaitForMultipleObjectsEx(numevents + 1, events, FALSE, timeoutval,
TRUE);
src/backend/port/win32/timer.c:         r =
WaitForSingleObjectEx(timerCommArea.event, waittime, FALSE);
src/backend/port/win32/timer.c:         timerCommArea.event =
CreateEvent(NULL, TRUE, FALSE, NULL);
src/backend/port/win32_sema.c:          rc =
WaitForMultipleObjectsEx(2, wh, FALSE, INFINITE, TRUE);
src/backend/port/win32_shmem.c: hmap = OpenFileMapping(FILE_MAP_READ,
FALSE, szShareMem);
src/backend/storage/ipc/dsm_impl.c:
                    FALSE,       /* do not inherit the name */
src/backend/storage/ipc/dsm_impl.c:
                                  PostmasterHandle, &hmap, 0, FALSE,
src/backend/storage/ipc/dsm_impl.c:
                                  NULL, NULL, 0, FALSE,
src/backend/storage/ipc/latch.c:        latch->event =
CreateEvent(NULL, TRUE, FALSE, NULL);
src/backend/storage/ipc/latch.c:        latch->event =
CreateEvent(&sa, TRUE, FALSE, NULL);
src/interfaces/ecpg/ecpglib/misc.c:                             return FALSE;
src/interfaces/ecpg/ecpglib/misc.c:                     return FALSE;
src/interfaces/ecpg/ecpglib/misc.c:                     mutex->handle
= CreateMutex(NULL, FALSE, NULL);
src/interfaces/ecpg/pgtypeslib/datetime.c:      bool
EuroDates = FALSE;
src/interfaces/ecpg/pgtypeslib/datetime.c:      bool
EuroDates = FALSE;
src/interfaces/ecpg/pgtypeslib/dt_common.c:     int
 bc = FALSE;
src/interfaces/ecpg/pgtypeslib/dt_common.c:     int
 is2digits = FALSE;
src/interfaces/ecpg/pgtypeslib/dt_common.c:     int
 haveTextMonth = FALSE;
src/interfaces/ecpg/pgtypeslib/dt_common.c:     int
 is2digits = FALSE;
src/interfaces/ecpg/pgtypeslib/dt_common.c:     int
 bc = FALSE;
src/interfaces/ecpg/pgtypeslib/interval.c:      bool
is_before = FALSE;
src/interfaces/ecpg/pgtypeslib/interval.c:      *is_zero = FALSE;
src/interfaces/ecpg/pgtypeslib/interval.c:      *is_zero = FALSE;
src/interfaces/ecpg/pgtypeslib/interval.c:      bool
is_before = FALSE;
src/interfaces/ecpg/pgtypeslib/interval.c:
 is_zero = FALSE;
src/interfaces/ecpg/pgtypeslib/numeric.c:       bool            have_dp = FALSE;
src/interfaces/ecpg/pgtypeslib/timestamp.c:             return FALSE;
src/interfaces/libpq/fe-secure.c: * The caller should say got_epipe =
FALSE if it is certain that it
src/pl/plperl/plperl.c:
eval_pv("PostgreSQL::InServer::SPI::bootstrap()", FALSE);
src/pl/plperl/plperl.c: eval_pv(PLC_TRUSTED, FALSE);
src/pl/plperl/plperl.c: eval_pv("my $a=chr(0x100); return $a =~
/\\xa9/i", FALSE);
src/pl/plperl/plperl.c:         eval_pv(plperl_on_plperl_init, FALSE);
src/pl/plperl/plperl.c:         eval_pv(plperl_on_plperlu_init, FALSE);
src/pl/plperl/plperl.c:         SV                **svp = av_fetch(av,
i, FALSE);
src/pl/plperl/plperl.c:                 while ((svp = av_fetch(rav, i,
FALSE)) != NULL)
src/pl/plperl/ppport.h:#  define ERRSV
get_sv("@",FALSE)
src/pl/plperl/ppport.h: utilize(!(flags & PERL_LOADMOD_DENY),
start_subparse(FALSE, 0),
src/pl/plperl/ppport.h:                 start_subparse(FALSE, 0),
src/pl/plperl/ppport.h: SV *my_cxt_sv = get_sv(MY_CXT_KEY, FALSE)
src/pl/plperl/ppport.h:    return FALSE;
src/pl/plperl/ppport.h:    bool overflowed = FALSE;
src/pl/plperl/ppport.h:    bool overflowed = FALSE;
src/pl/plperl/ppport.h:    bool overflowed = FALSE;
src/pl/plpgsql/src/pl_comp.c: * if not recognized, fill in *word and
return FALSE.
src/port/kill.c:                if ((prochandle =
OpenProcess(PROCESS_TERMINATE, FALSE, (DWORD) pid)) == NULL)
src/port/pgsleep.c:             SleepEx((microsec < 500 ? 1 :
(microsec + 500) / 1000), FALSE);
src/test/regress/pg_regress.c:          r =
WaitForMultipleObjects(tests_left, active_pids, FALSE, INFINITE);

> 0003-Remove-TRUE-and-FALSE.patch

Looks good.  What about these?

src/interfaces/ecpg/include/ecpglib.h:#define FALSE     0
src/interfaces/ecpg/pgtypeslib/extern.h:#define FALSE   0

> 0004-Remove-BoolPtr-type.patch

Looks good.

> 0005-Make-casting-between-bool-and-GinTernaryValue-more-r.patch

- * For convenience, this is compatible with booleans. A boolean can be
+ * For convenience, this is compatible with bools. A bool can be

Maybe "compatible with bool" ?

+#elif SIZEOF_BOOL == 4
+typedef int GinTernaryValue;
+#else

Maybe int32?  I understand that PostgreSQL doesn't actually work if
int isn't of that size, but still.

> 0006-Add-bool8-typedef-for-system-catalog-structs.patch

Make sense.

> 0007-Avoid-use-of-bool-in-thread_test.c.patch

Looks good.

> 0008-Use-stdbool.h-if-available.patch

I'm afraid this autoconf stuff is gibberish, and I can't personally
tell if it's the right gibberish so no opinion on this one.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: taking stdbool.h into use

Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-16T14:20:11Z

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> However my system has sizeof(bool) == 1 and so do all the systems I
> have access to (x86 + POWER).  Where can we find a computer with
> sizeof(bool) == 4?  According to the intertubes OSX on POWER and
> Windows 32 bit systems had that in ancient prehistory but they don't
> now.

prairiedog and (I assume) locust.  Maybe coypu --- I imagine OSX
got that choice from upstream BSD someplace.

cube:~ tgl$ uname -a
Darwin cube.sss.pgh.pa.us 8.11.0 Darwin Kernel Version 8.11.0: Wed Oct 10 18:26:00 PDT 2007; root:xnu-792.24.17~1/RELEASE_PPC Power Macintosh powerpc
cube:~ tgl$ cat foo.c
#include <stdio.h>
#include <stdbool.h>

int main()
{
  printf("sizeof(bool) = %zu\n", sizeof(bool));
  return 0;
}
cube:~ tgl$ gcc -O -Wall foo.c
cube:~ tgl$ ./a.out
sizeof(bool) = 4

Don't know how far back you need to go to find Windows machines
with 4-byte bool, but we have some pretty long-in-the-tooth
buildfarm critters in that lineage, too.

gaur/pademelon isn't booted up right now, but it might provide
an example of a system that lacks <stdbool.h> altogether.
(If it doesn't, I'd be willing to concede that we need not
consider that scenario anymore.)

			regards, tom lane

Re: taking stdbool.h into use

Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-16T21:31:00Z

I wrote:
> gaur/pademelon isn't booted up right now, but it might provide
> an example of a system that lacks <stdbool.h> altogether.
> (If it doesn't, I'd be willing to concede that we need not
> consider that scenario anymore.)

For the record --- pademelon (vendor cc on that box) doesn't have
<stdbool.h> at all.  gaur (user-installed gcc) has such a header,
but it contains

typedef enum
  {
    false = 0,
    true = 1
  } bool;

which unsurprisingly results in
sizeof(bool) = 4

What's possibly more relevant to Peter's patch, this represents
a platform on which "#include <stdbool.h>" succeeds, but
(a) there is no typedef _Bool, and (b) "bool" is not a macro.
Obviously pre-C99 ...

			regards, tom lane

Re: taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2017-08-17T02:31:06Z

On Wed, Aug 16, 2017 at 11:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Don't know how far back you need to go to find Windows machines
> with 4-byte bool, but we have some pretty long-in-the-tooth
> buildfarm critters in that lineage, too.

From VS 2003 and upwards the size has always been 1:
https://msdn.microsoft.com/en-us/library/s3f49ktz.aspx
So this gives some margin as the buildfarm uses VS 2008 and newer versions.
-- 
Michael

Re: taking stdbool.h into use

Tom Lane <tgl@sss.pgh.pa.us> — 2017-08-24T21:32:20Z

Peter Eisentraut <peter.eisentraut@2ndquadrant.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

Re: taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-09-14T18:56:22Z

On 8/16/17 02:32, Thomas Munro wrote:
>> 0001-Fix-bool-int-type-confusion.patch
> 
> Looks good.

Committed that one.

>> 0002-Change-TRUE-FALSE-to-true-false.patch
> 
> Looks good.  What about these?

OK, did more digging.  Windows, ICU, and Perl define their own
TRUE/FALSE, so in those uses we don't have to make any changes.  ECPG
also has its own definitions.  In ECPG I also found some obsolete uses
for bools that have been fixed in the equivalent backend code a long
time ago, so I added patches to fix that.  And another use in
src/backend/port/dynloader/darwin.c was probably wrong to begin with as
well.

I also went through all the comments to make equivalent changes.  I kept
that in a separate patch for easier viewing, but it should probably be
committed together.

Using a case-insensitive diff mechanism one can verify that no logic was
changed here.

>> 0003-Remove-TRUE-and-FALSE.patch
> 
> Looks good.  What about these?
> 
> src/interfaces/ecpg/include/ecpglib.h:#define FALSE     0
> src/interfaces/ecpg/pgtypeslib/extern.h:#define FALSE   0

Not sure about that.  These are available for use by ecpg applications,
and it's perhaps not worth disrupting that.

>> 0004-Remove-BoolPtr-type.patch
> 
> Looks good.

Committed that one.

>> 0007-Avoid-use-of-bool-in-thread_test.c.patch
>
> Looks good.

and that one

>> 0005-Make-casting-between-bool-and-GinTernaryValue-more-r.patch
>> 0008-Use-stdbool.h-if-available.patch

These need some more work based on Tom's feedback.

Attached is a new patch set.  Based on the discussion so far, 0001
through 0007 might be ready; the other two need some more work.

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

Re: taking stdbool.h into use

Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-15T02:35:18Z

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> 0005-Make-casting-between-bool-and-GinTernaryValue-more-r.patch
> 0008-Use-stdbool.h-if-available.patch

> These need some more work based on Tom's feedback.

> Attached is a new patch set.  Based on the discussion so far, 0001
> through 0007 might be ready; the other two need some more work.

Do you need me to do another round of tests on prairiedog/gaur/pademelon?

			regards, tom lane

Re: taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-09-16T12:44:29Z

On 9/14/17 22:35, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> 0005-Make-casting-between-bool-and-GinTernaryValue-more-r.patch
>> 0008-Use-stdbool.h-if-available.patch
> 
>> These need some more work based on Tom's feedback.
> 
>> Attached is a new patch set.  Based on the discussion so far, 0001
>> through 0007 might be ready; the other two need some more work.
> 
> Do you need me to do another round of tests on prairiedog/gaur/pademelon?

No, I haven't done any further work on those portability-critical pieces
yet.

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

Re: taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-10-26T16:25:14Z

Here is an updated patch set.  This is just a rebase of the previous
set, no substantial changes.  Based on the discussion so far, I'm
proposing that 0001 through 0007 could be ready to commit after review,
whereas the remaining two need more work at some later time.

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

Re: taking stdbool.h into use

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-10-26T17:51:08Z

Peter Eisentraut wrote:
> Here is an updated patch set.  This is just a rebase of the previous
> set, no substantial changes.  Based on the discussion so far, I'm
> proposing that 0001 through 0007 could be ready to commit after review,
> whereas the remaining two need more work at some later time.

I gave this a quick run, to see if my compiler would complain for things
like this:

   bool        isprimary = flags & INDEX_CREATE_IS_PRIMARY;

(taken from the first patch at
https://postgr.es/m/20171023161503.ohkybquxrlech7d7@alvherre.pgsql )

which is assigning a value other than 1/0 to a bool variable without an
explicit cast.  I thought it would provoke a warning, but it does not.
Is that expected?  Is my compiler too old/new?

config.log says
gcc version 6.3.0 20170516 (Debian 6.3.0-18) 

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

Re: taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2017-10-26T20:53:56Z

On Thu, Oct 26, 2017 at 10:51 AM, Alvaro Herrera
<alvherre@alvh.no-ip.org> wrote:
> I gave this a quick run, to see if my compiler would complain for things
> like this:
>
>    bool        isprimary = flags & INDEX_CREATE_IS_PRIMARY;
>
> (taken from the first patch at
> https://postgr.es/m/20171023161503.ohkybquxrlech7d7@alvherre.pgsql )
>
> which is assigning a value other than 1/0 to a bool variable without an
> explicit cast.  I thought it would provoke a warning, but it does not.
> Is that expected?  Is my compiler too old/new?

It seems to me that this proves the point of the proposed patch. You
had better use a zero-equality comparison for such bitwise operation,
and so you ought to do that:
bool        isprimary = (flags & INDEX_CREATE_IS_PRIMARY) != 0;
-- 
Michael

Re: taking stdbool.h into use

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-10-26T22:48:33Z

Michael Paquier wrote:
> On Thu, Oct 26, 2017 at 10:51 AM, Alvaro Herrera
> <alvherre@alvh.no-ip.org> wrote:
> > I gave this a quick run, to see if my compiler would complain for things
> > like this:
> >
> >    bool        isprimary = flags & INDEX_CREATE_IS_PRIMARY;
> >
> > (taken from the first patch at
> > https://postgr.es/m/20171023161503.ohkybquxrlech7d7@alvherre.pgsql )
> >
> > which is assigning a value other than 1/0 to a bool variable without an
> > explicit cast.  I thought it would provoke a warning, but it does not.
> > Is that expected?  Is my compiler too old/new?
> 
> It seems to me that this proves the point of the proposed patch. You
> had better use a zero-equality comparison for such bitwise operation,
> and so you ought to do that:
> bool        isprimary = (flags & INDEX_CREATE_IS_PRIMARY) != 0;

Right, exactly.  But my point is that with the whole patch series
applied I didn't get any warnings.

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

Re: taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2017-10-26T23:10:28Z

On Thu, Oct 26, 2017 at 3:48 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Right, exactly.  But my point is that with the whole patch series
> applied I didn't get any warnings.

Sorry, I misread your message. You use Linux I suppose, what's your compiler?
-- 
Michael

Re: taking stdbool.h into use

Tom Lane <tgl@sss.pgh.pa.us> — 2017-10-27T00:41:44Z

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Michael Paquier wrote:
>> It seems to me that this proves the point of the proposed patch. You
>> had better use a zero-equality comparison for such bitwise operation,
>> and so you ought to do that:
>> bool        isprimary = (flags & INDEX_CREATE_IS_PRIMARY) != 0;

> Right, exactly.  But my point is that with the whole patch series
> applied I didn't get any warnings.

While warnings for this would be lovely, I don't see how we can expect to
get any.  This is perfectly correct C code no matter whether isprimary
is C99 bool or is typedef'd to char ... you just end up with different
values of isprimary, should the RHS produce something other than 1/0.
The compiler has no way to know that assigning, say, 4 in the char
variable case is not quite your intent.  Maybe you could hope for a
warning if the bit value were far enough left to actually not fit into
"char", but otherwise there's nothing wrong.

			regards, tom lane

Re: taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2017-10-28T23:11:34Z

On Thu, Oct 26, 2017 at 5:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> While warnings for this would be lovely, I don't see how we can expect to
> get any.  This is perfectly correct C code no matter whether isprimary
> is C99 bool or is typedef'd to char ... you just end up with different
> values of isprimary, should the RHS produce something other than 1/0.
> The compiler has no way to know that assigning, say, 4 in the char
> variable case is not quite your intent.  Maybe you could hope for a
> warning if the bit value were far enough left to actually not fit into
> "char", but otherwise there's nothing wrong.

This reminded me of
https://www.postgresql.org/message-id/20160212144735.7zkg5527i3un3254%40alap3.anarazel.de
which has caused commit af4472bc when using stdbool.h for MSVC
2013/2015 builds. So I would really assume that there are places where
we could see warnings.
-- 
Michael

Re: taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2017-10-29T12:50:57Z

On Thu, Oct 26, 2017 at 9:25 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Here is an updated patch set.  This is just a rebase of the previous
> set, no substantial changes.  Based on the discussion so far, I'm
> proposing that 0001 through 0007 could be ready to commit after review,
> whereas the remaining two need more work at some later time.

I had a look at this patch series. Patches 1, 2 (macos headers indeed
show that NSUNLINKMODULE_OPTION_NONE is set to 0x0), 3 to 7 look fine
to me.

I spotted a couple of other things while looking at your patches and
the code tree.

-   return (ginCompareItemPointers(&btree->itemptr, iptr) > 0) ? TRUE : FALSE;
+   return (ginCompareItemPointers(&btree->itemptr, iptr) > 0) ? true : false;
You could simplify that at the same time by removing such things. The
"false : true" pattern is less frequent than the "true : false"
pattern.

Some comments in the code still mention FALSE or TRUE:
- hashsearch.c uses FALSE in some comments.
- In execExpr.c, ExecCheck mentions TRUE.
- AggStateIsShared mentions TRUE and FALSE.
- In arrayfuncs.c, ReadArrayStr, CopyArrayEls and ReadArrayBinary use TRUE.

0009 looks like a good idea. It would make the code less confusing for
the reader to mention HAVE_STDBOOL_H in the #endif of c.h that you are
complicating to make the end of the block clear. I am lacking energy
for 0008, so I have no opinions to offer. Sorry :p
-- 
Michael

Re: taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-11-04T16:21:24Z

On 10/29/17 08:50, Michael Paquier wrote:
> On Thu, Oct 26, 2017 at 9:25 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> Here is an updated patch set.  This is just a rebase of the previous
>> set, no substantial changes.  Based on the discussion so far, I'm
>> proposing that 0001 through 0007 could be ready to commit after review,
>> whereas the remaining two need more work at some later time.
> 
> I had a look at this patch series. Patches 1, 2 (macos headers indeed
> show that NSUNLINKMODULE_OPTION_NONE is set to 0x0), 3 to 7 look fine
> to me.

Committed 1, 2, 3; will work on the rest later and incorporate your
findings.

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

Re: taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-11-08T16:46:45Z

On 10/29/17 08:50, Michael Paquier wrote:
> I had a look at this patch series. Patches 1, 2 (macos headers indeed
> show that NSUNLINKMODULE_OPTION_NONE is set to 0x0), 3 to 7 look fine
> to me.

Committed 4 and 5 together.

> I spotted a couple of other things while looking at your patches and
> the code tree.
> 
> -   return (ginCompareItemPointers(&btree->itemptr, iptr) > 0) ? TRUE : FALSE;
> +   return (ginCompareItemPointers(&btree->itemptr, iptr) > 0) ? true : false;
> You could simplify that at the same time by removing such things. The
> "false : true" pattern is less frequent than the "true : false"
> pattern.

I have found many more instances like that.  It might be worth
simplifying a bit, but that sounds like a separate undertaking.

> Some comments in the code still mention FALSE or TRUE:
> - hashsearch.c uses FALSE in some comments.
> - In execExpr.c, ExecCheck mentions TRUE.

That one is an SQL TRUE, so I left it.

> - AggStateIsShared mentions TRUE and FALSE.
> - In arrayfuncs.c, ReadArrayStr, CopyArrayEls and ReadArrayBinary use TRUE.

Fixed the other ones.

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

Re: taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2017-11-08T23:19:52Z

On Thu, Nov 9, 2017 at 1:46 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 10/29/17 08:50, Michael Paquier wrote:
>> I spotted a couple of other things while looking at your patches and
>> the code tree.
>>
>> -   return (ginCompareItemPointers(&btree->itemptr, iptr) > 0) ? TRUE : FALSE;
>> +   return (ginCompareItemPointers(&btree->itemptr, iptr) > 0) ? true : false;
>> You could simplify that at the same time by removing such things. The
>> "false : true" pattern is less frequent than the "true : false"
>> pattern.
>
> I have found many more instances like that.  It might be worth
> simplifying a bit, but that sounds like a separate undertaking.

Yeah, I just mentioned one for reference. And I can see 66 instances.
It may be not worth changing either to simplify back-patching.

>> Some comments in the code still mention FALSE or TRUE:
>> - hashsearch.c uses FALSE in some comments.
>> - In execExpr.c, ExecCheck mentions TRUE.
>
> That one is an SQL TRUE, so I left it.

Oops. You are right. I tried to be careful with what was referring to SQL and C.
-- 
Michael

Re: [HACKERS] taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-11-15T20:13:38Z

I'm going to put this patch set as Returned With Feedback for now.  The
GinNullCategory issues look like they will need quite a bit of work.
But it will be worth picking this up some time.

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

Re: [HACKERS] taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-12-20T16:02:41Z

On 11/15/17 15:13, Peter Eisentraut wrote:
> I'm going to put this patch set as Returned With Feedback for now.  The
> GinNullCategory issues look like they will need quite a bit of work.
> But it will be worth picking this up some time.

I think the issue with GinNullCategory is practically unfixable.  This
is on-disk data that needs to be castable to an array of bool.  So
tolerating a bool of size other than 1 would either require a disk
format change or extensive code changes, neither of which seem
worthwhile at this point.

So here is a minimal patch set to perhaps wrap this up for the time
being.  I have added static assertions that check the sizes of
GinNullCategory and GinTernaryValue, which I think are the two critical
places that require compatibility with bool.  And then we include
<stdbool.h> only if its bool has size 1.

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

Re: [HACKERS] taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2017-12-25T05:32:28Z

On Thu, Dec 21, 2017 at 1:02 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 11/15/17 15:13, Peter Eisentraut wrote:
>> I'm going to put this patch set as Returned With Feedback for now.  The
>> GinNullCategory issues look like they will need quite a bit of work.
>> But it will be worth picking this up some time.
>
> I think the issue with GinNullCategory is practically unfixable.  This
> is on-disk data that needs to be castable to an array of bool.  So
> tolerating a bool of size other than 1 would either require a disk
> format change or extensive code changes, neither of which seem
> worthwhile at this point.
>
> So here is a minimal patch set to perhaps wrap this up for the time
> being.  I have added static assertions that check the sizes of
> GinNullCategory and GinTernaryValue, which I think are the two critical
> places that require compatibility with bool.  And then we include
> <stdbool.h> only if its bool has size 1.

+   /*
+    * now we can use the nullFlags as category codes
+    */
+   StaticAssertStmt(sizeof(GinNullCategory) == sizeof(bool),
+                    "sizes of GinNullCategory and bool are not equal");
    *categories = (GinNullCategory *) nullFlags;

Hm. So on powerpc compilation is going to fail with this patch as
sizeof(char) is 1, no? Wouldn't it be better to just allocate an array
for GinNullCategory entries and then just fill in the values one by
one with GIN_CAT_NORM_KEY or GIN_CAT_NULL_KEY by scanning nullFlags?
-- 
Michael

Re: [HACKERS] taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-12-26T19:00:47Z

On 12/25/17 00:32, Michael Paquier wrote:
>> So here is a minimal patch set to perhaps wrap this up for the time
>> being.  I have added static assertions that check the sizes of
>> GinNullCategory and GinTernaryValue, which I think are the two critical
>> places that require compatibility with bool.  And then we include
>> <stdbool.h> only if its bool has size 1.
> 
> +   /*
> +    * now we can use the nullFlags as category codes
> +    */
> +   StaticAssertStmt(sizeof(GinNullCategory) == sizeof(bool),
> +                    "sizes of GinNullCategory and bool are not equal");
>     *categories = (GinNullCategory *) nullFlags;
> 
> Hm. So on powerpc compilation is going to fail with this patch as
> sizeof(char) is 1, no?

Yes, but right now it would (probably) just fail in mysterious ways, so
the static assertion adds safety.

> Wouldn't it be better to just allocate an array
> for GinNullCategory entries and then just fill in the values one by
> one with GIN_CAT_NORM_KEY or GIN_CAT_NULL_KEY by scanning nullFlags?

Yeah, initially I though making another loop through the array would be
adding more overhead.  But reading the code again, we already loop
through the array anyway to set the nullFlags to the right bit patterns.
 So I think we can just drop that and build a proper GinNullCategory
array instead.  I think that would be much cleaner.  Patch attached.

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

Re: [HACKERS] taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2017-12-27T04:10:06Z

On Tue, Dec 26, 2017 at 02:00:47PM -0500, Peter Eisentraut wrote:
> On 12/25/17 00:32, Michael Paquier wrote:
> >> So here is a minimal patch set to perhaps wrap this up for the time
> >> being.  I have added static assertions that check the sizes of
> >> GinNullCategory and GinTernaryValue, which I think are the two critical
> >> places that require compatibility with bool.  And then we include
> >> <stdbool.h> only if its bool has size 1.
> > 
> > +   /*
> > +    * now we can use the nullFlags as category codes
> > +    */
> > +   StaticAssertStmt(sizeof(GinNullCategory) == sizeof(bool),
> > +                    "sizes of GinNullCategory and bool are not equal");
> >     *categories = (GinNullCategory *) nullFlags;
> > 
> > Hm. So on powerpc compilation is going to fail with this patch as
> > sizeof(char) is 1, no?
> 
> Yes, but right now it would (probably) just fail in mysterious ways, so
> the static assertion adds safety.
> 
> > Wouldn't it be better to just allocate an array
> > for GinNullCategory entries and then just fill in the values one by
> > one with GIN_CAT_NORM_KEY or GIN_CAT_NULL_KEY by scanning nullFlags?
> 
> Yeah, initially I though making another loop through the array would be
> adding more overhead.  But reading the code again, we already loop
> through the array anyway to set the nullFlags to the right bit patterns.
>  So I think we can just drop that and build a proper GinNullCategory
> array instead.  I think that would be much cleaner.  Patch attached.

Thanks for the updated patch. This looks saner to me.

It would be nice to do something like that for GinTernaryValue in
tsginidx.c by mapping directly to GIN_FALSE and GIN_TRUE depending on
the input coming in gin_tsquery_consistent. The fix is more trivial
there. Do you think that those two things should be back-patched?
--
Michael

Re: [HACKERS] taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-12-27T17:52:55Z

On 12/26/17 23:10, Michael Paquier wrote:
> It would be nice to do something like that for GinTernaryValue in
> tsginidx.c by mapping directly to GIN_FALSE and GIN_TRUE depending on
> the input coming in gin_tsquery_consistent. The fix is more trivial
> there.

For GinTernaryValue, I think it's easier to just make it the same size
as bool, since it doesn't go onto disk.  My earlier patch did that.  I'm
not sure it's worth adding more code to copy the array around.

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

Re: [HACKERS] taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2017-12-28T00:47:52Z

On Wed, Dec 27, 2017 at 12:52:55PM -0500, Peter Eisentraut wrote:
> On 12/26/17 23:10, Michael Paquier wrote:
> > It would be nice to do something like that for GinTernaryValue in
> > tsginidx.c by mapping directly to GIN_FALSE and GIN_TRUE depending on
> > the input coming in gin_tsquery_consistent. The fix is more trivial
> > there.
> 
> For GinTernaryValue, I think it's easier to just make it the same size
> as bool, since it doesn't go onto disk.  My earlier patch did that.  I'm
> not sure it's worth adding more code to copy the array around.

But on prairiedog the sizeof bool and char are different, so compilation
would fail, no? checkcondition_gin is used only by
gin_tsquery_consistent so I think that it is possible to get advantage
of that by using a secondary type of GinChkVal which uses directly a
bool array and converts the check value for the operand to a
GinTernaryValue value on-the-fly. I agree that this would make the code
more complex though for not much gain on modern platform.
--
Michael

Re: [HACKERS] taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-12-28T15:15:58Z

On 12/27/17 19:47, Michael Paquier wrote:
>> For GinTernaryValue, I think it's easier to just make it the same size
>> as bool, since it doesn't go onto disk.  My earlier patch did that.  I'm
>> not sure it's worth adding more code to copy the array around.
> But on prairiedog the sizeof bool and char are different, so compilation
> would fail, no? 

An earlier patch I posted defines GinTernaryValue to be the same size as
bool, accounting for different possible sizes of bool.

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

Re: [HACKERS] taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-12-28T18:45:54Z

Here is a new patch set that picks up the best pieces of the recent
discussions:  We use stdbool.h if available, use bool8 in the system
catalogs, define GinTernaryValue to be the same size as bool, and
rewrite the GinNullCategory code to work without casting.

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

Re: [HACKERS] taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2017-12-29T08:48:54Z

On Thu, Dec 28, 2017 at 01:45:54PM -0500, Peter Eisentraut wrote:
> An earlier patch I posted defines GinTernaryValue to be the same size as
> bool, accounting for different possible sizes of bool.

Doh. I forgot this one. Yes that approach is fine.

> Here is a new patch set that picks up the best pieces of the recent
> discussions:  We use stdbool.h if available, use bool8 in the system
> catalogs, define GinTernaryValue to be the same size as bool, and
> rewrite the GinNullCategory code to work without casting.

I have looked at 0002 and 0003. Those look good to ship for me.
--
Michael

Re: [HACKERS] taking stdbool.h into use

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-12-29T12:43:54Z

Michael Paquier wrote:
> On Thu, Dec 28, 2017 at 01:45:54PM -0500, Peter Eisentraut wrote:

> > Here is a new patch set that picks up the best pieces of the recent
> > discussions:  We use stdbool.h if available, use bool8 in the system
> > catalogs, define GinTernaryValue to be the same size as bool, and
> > rewrite the GinNullCategory code to work without casting.
> 
> I have looked at 0002 and 0003. Those look good to ship for me.

Yeah, I'd vote to push those right away to see what buildfarm has to
say.  That way you can push 0001 shortly after the dust settles (if
any), which will have an effect on the bootstrap data format patch.

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

Re: [HACKERS] taking stdbool.h into use

Tom Lane <tgl@sss.pgh.pa.us> — 2017-12-29T17:33:24Z

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Michael Paquier wrote:
>> I have looked at 0002 and 0003. Those look good to ship for me.

> Yeah, I'd vote to push those right away to see what buildfarm has to
> say.  That way you can push 0001 shortly after the dust settles (if
> any), which will have an effect on the bootstrap data format patch.

Yeah, I think all of this is at the point where the next thing to do
is see what the buildfarm has to say.  I could test it manually on
prairiedog, but I'd just as soon let the buildfarm script do the work.

It does make sense, probably, to push 0001-0003 first and see if
anything turns up from that, then 0004.

			regards, tom lane

Re: [HACKERS] taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2017-12-29T23:29:09Z

On Fri, Dec 29, 2017 at 12:33:24PM -0500, Tom Lane wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > Michael Paquier wrote:
> >> I have looked at 0002 and 0003. Those look good to ship for me.
> 
> > Yeah, I'd vote to push those right away to see what buildfarm has to
> > say.  That way you can push 0001 shortly after the dust settles (if
> > any), which will have an effect on the bootstrap data format patch.
> 
> Yeah, I think all of this is at the point where the next thing to do
> is see what the buildfarm has to say.  I could test it manually on
> prairiedog, but I'd just as soon let the buildfarm script do the work.
> 
> It does make sense, probably, to push 0001-0003 first and see if
> anything turns up from that, then 0004.

I have not looked at 0001 in details yet, which was going to be my next
step. If you could wait for at least two days that would be nice to give
me some room.

0002 and 0003 are independent on the rest, which is why I looked at them
first. Would we want to actually backpatch them at some point? Perhaps
not per the lack of complains in this area.
--
Michael

Re: [HACKERS] taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2017-12-30T13:08:22Z

On Sat, Dec 30, 2017 at 08:29:09AM +0900, Michael Paquier wrote:
> On Fri, Dec 29, 2017 at 12:33:24PM -0500, Tom Lane wrote:
>> It does make sense, probably, to push 0001-0003 first and see if
>> anything turns up from that, then 0004.
> 
> I have not looked at 0001 in details yet, which was going to be my next
> step. If you could wait for at least two days that would be nice to give
> me some room.

So, looking at 0001 now... Shouldn't there be a DatumGetBool8(), with
the existing DatumGetBool() which should depend on the size of bool? I
can see that all the catalogs are correctly updated with bool8 in the
patch.
--
Michael

Re: [HACKERS] taking stdbool.h into use

Tom Lane <tgl@sss.pgh.pa.us> — 2017-12-30T15:31:45Z

Michael Paquier <michael.paquier@gmail.com> writes:
> So, looking at 0001 now... Shouldn't there be a DatumGetBool8(), with
> the existing DatumGetBool() which should depend on the size of bool? I
> can see that all the catalogs are correctly updated with bool8 in the
> patch.

Surely bool and bool8 should have identical Datum representations,
so I'm not sure they need different DatumGet/GetDatum macros.

Although this opens up another point: just above those macros,
postgres.h says

 * When a type narrower than Datum is stored in a Datum, we place it in the
 * low-order bits and are careful that the DatumGetXXX macro for it discards
 * the unused high-order bits (as opposed to, say, assuming they are zero).
 * This is needed to support old-style user-defined functions, since depending
 * on architecture and compiler, the return value of a function returning char
 * or short may contain garbage when called as if it returned Datum.

Since we flushed support for V0 functions, the stated rationale doesn't
apply anymore.  I wonder whether there is anything to be gained by
changing DatumGetXXX and XXXGetDatum to be simple casts (as, if memory
serves, they once were until we noticed the stated hazard).  Or, if
there's still a reason to keep the masking steps in place, we'd better
update this comment.

			regards, tom lane

Re: [HACKERS] taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2018-01-09T05:17:53Z

On Sat, Dec 30, 2017 at 10:31:45AM -0500, Tom Lane wrote:
> Surely bool and bool8 should have identical Datum representations,
> so I'm not sure they need different DatumGet/GetDatum macros.

[... refresh update ...]
Yeah sure. I am a bit disturbed by the fact that DatumGetBool() enforces
a case to bool though.

> Although this opens up another point: just above those macros,
> postgres.h says
> 
>  * When a type narrower than Datum is stored in a Datum, we place it in the
>  * low-order bits and are careful that the DatumGetXXX macro for it discards
>  * the unused high-order bits (as opposed to, say, assuming they are zero).
>  * This is needed to support old-style user-defined functions, since depending
>  * on architecture and compiler, the return value of a function returning char
>  * or short may contain garbage when called as if it returned Datum.
> 
> Since we flushed support for V0 functions, the stated rationale doesn't
> apply anymore.  I wonder whether there is anything to be gained by
> changing DatumGetXXX and XXXGetDatum to be simple casts (as, if memory
> serves, they once were until we noticed the stated hazard).  Or, if
> there's still a reason to keep the masking steps in place, we'd better
> update this comment.

23a41573 did not do it rightly visibly, and 23b09e1 got it better, still
GET_1_BYTE() was getting used only because of V0 functions being used in
contrib/seg and because of the way MSVC 2015 handles boolean evaluation
as per thread [1].

In short, your argument for a switch to simple casts makes sense for me.

[1]: https://www.postgresql.org/message-id/E1atdPs-0005zu-Re@gemulon.postgresql.org
--
Michael

Re: [HACKERS] taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-01-11T22:01:55Z

On 1/9/18 00:17, Michael Paquier wrote:
>>  * When a type narrower than Datum is stored in a Datum, we place it in the
>>  * low-order bits and are careful that the DatumGetXXX macro for it discards
>>  * the unused high-order bits (as opposed to, say, assuming they are zero).
>>  * This is needed to support old-style user-defined functions, since depending
>>  * on architecture and compiler, the return value of a function returning char
>>  * or short may contain garbage when called as if it returned Datum.
>>
>> Since we flushed support for V0 functions, the stated rationale doesn't
>> apply anymore.  I wonder whether there is anything to be gained by
>> changing DatumGetXXX and XXXGetDatum to be simple casts (as, if memory
>> serves, they once were until we noticed the stated hazard).  Or, if
>> there's still a reason to keep the masking steps in place, we'd better
>> update this comment.

Yeah, we should remove those bit-masking calls if they are no longer
necessary.

Looking around where else they are used, the uses in numeric.c sure seem
like noops:

#if SIZEOF_DATUM == 8
#define NumericAbbrevGetDatum(X) ((Datum) SET_8_BYTES(X))
#define DatumGetNumericAbbrev(X) ((int64) GET_8_BYTES(X))
#define NUMERIC_ABBREV_NAN      NumericAbbrevGetDatum(PG_INT64_MIN)
#else
#define NumericAbbrevGetDatum(X) ((Datum) SET_4_BYTES(X))
#define DatumGetNumericAbbrev(X) ((int32) GET_4_BYTES(X))
#define NUMERIC_ABBREV_NAN      NumericAbbrevGetDatum(PG_INT32_MIN)
#endif

We can just replace these by straight casts, too.

That leaves the uses in rowtypes.c.  Those were introduced as a
portability fix by commit 4cbb646334b.  I'm curious why these are
necessary.  The Datums they operate come from heap_deform_tuple(), which
gets them from fetchatt(), which does run all pass-by-value values
through the very same GET_X_BYTES() macros (until now).  I don't see
where those dirty upper bits would be coming from.

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

Re: [HACKERS] taking stdbool.h into use

Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-11T23:40:05Z

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> That leaves the uses in rowtypes.c.  Those were introduced as a
> portability fix by commit 4cbb646334b.  I'm curious why these are
> necessary.  The Datums they operate come from heap_deform_tuple(), which
> gets them from fetchatt(), which does run all pass-by-value values
> through the very same GET_X_BYTES() macros (until now).  I don't see
> where those dirty upper bits would be coming from.

I don't see it either.  I think the actually useful parts of that patch
were to declare record_image_cmp's result correctly as int rather than
bool, and to cope with varlena fields of unequal size.  Unfortunately
there seems to be no contemporaneous mailing list discussion, so
it's not clear what Kevin based this change on.

Want to try reverting the GET_X_BYTES() parts of it and see if the
buildfarm complains?

Note if that if we simplify the GetDatum macros, it's possible that
record_image_cmp would change behavior, since it might now see signed not
unsigned values depending on whether the casts do sign extension or not.
Not sure if that'd be a problem.

			regards, tom lane

Re: [HACKERS] taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2018-01-16T06:14:14Z

On Thu, Jan 11, 2018 at 06:40:05PM -0500, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> That leaves the uses in rowtypes.c.  Those were introduced as a
>> portability fix by commit 4cbb646334b.  I'm curious why these are
>> necessary.  The Datums they operate come from heap_deform_tuple(), which
>> gets them from fetchatt(), which does run all pass-by-value values
>> through the very same GET_X_BYTES() macros (until now).  I don't see
>> where those dirty upper bits would be coming from.
> 
> I don't see it either.  I think the actually useful parts of that patch
> were to declare record_image_cmp's result correctly as int rather than
> bool, and to cope with varlena fields of unequal size.  Unfortunately
> there seems to be no contemporaneous mailing list discussion, so
> it's not clear what Kevin based this change on.

This was a hotfix after a buildfarm breakage, the base commit being
f566515.

> Want to try reverting the GET_X_BYTES() parts of it and see if the
> buildfarm complains?

So, Peter, are you planning to do so?

> Note if that if we simplify the GetDatum macros, it's possible that
> record_image_cmp would change behavior, since it might now see signed not
> unsigned values depending on whether the casts do sign extension or not.
> Not sure if that'd be a problem.

Based on the patch series in
https://www.postgresql.org/message-id/d86ec1f4-941c-e702-b05a-748ea2e59e9c@2ndquadrant.com,
the next thing that could be shipped is 0003 in my opinion, as 0002 has
already been pushed.
--
Michael

Re: [HACKERS] taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-01-16T23:12:29Z

On 1/11/18 17:01, Peter Eisentraut wrote:
> Looking around where else they are used, the uses in numeric.c sure seem
> like noops:
> 
> #if SIZEOF_DATUM == 8
> #define NumericAbbrevGetDatum(X) ((Datum) SET_8_BYTES(X))
> #define DatumGetNumericAbbrev(X) ((int64) GET_8_BYTES(X))
> #define NUMERIC_ABBREV_NAN      NumericAbbrevGetDatum(PG_INT64_MIN)
> #else
> #define NumericAbbrevGetDatum(X) ((Datum) SET_4_BYTES(X))
> #define DatumGetNumericAbbrev(X) ((int32) GET_4_BYTES(X))
> #define NUMERIC_ABBREV_NAN      NumericAbbrevGetDatum(PG_INT32_MIN)
> #endif
> 
> We can just replace these by straight casts, too.

I have committed a change for this.  I'll work through the other details
later.

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

Re: [HACKERS] taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-01-23T16:33:56Z

On 1/16/18 01:14, Michael Paquier wrote:
>> I don't see it either.  I think the actually useful parts of that patch
>> were to declare record_image_cmp's result correctly as int rather than
>> bool, and to cope with varlena fields of unequal size.  Unfortunately
>> there seems to be no contemporaneous mailing list discussion, so
>> it's not clear what Kevin based this change on.
> 
> This was a hotfix after a buildfarm breakage, the base commit being
> f566515.
> 
>> Want to try reverting the GET_X_BYTES() parts of it and see if the
>> buildfarm complains?
> 
> So, Peter, are you planning to do so?

Here is a proposed patch set to clean this up.  First, add some test
coverage for record_image_cmp.  (There wasn't any, only for
record_image_eq as part of MV testing.)  Then, remove the GET_ macros
from record_image_{eq,cmp}.  And finally remove all that byte-masking
stuff from postgres.h.

>> Note if that if we simplify the GetDatum macros, it's possible that
>> record_image_cmp would change behavior, since it might now see signed not
>> unsigned values depending on whether the casts do sign extension or not.
>> Not sure if that'd be a problem.

I wasn't able to construct such a case.  Everything still works unsigned
end-to-end, I think.  But if you can think of a case, we can throw it
into the tests.  The tests already contain cases of comparing positive
and negative integers.

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

Re: [HACKERS] taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2018-01-24T06:44:04Z

On Tue, Jan 23, 2018 at 11:33:56AM -0500, Peter Eisentraut wrote:
> Here is a proposed patch set to clean this up.  First, add some test
> coverage for record_image_cmp.  (There wasn't any, only for
> record_image_eq as part of MV testing.)  Then, remove the GET_ macros
> from record_image_{eq,cmp}.  And finally remove all that byte-masking
> stuff from postgres.h.

Good catch. Coverage reports mark those areas as empty! Similarly the
functions for record_* are mostly not used. Some tests could be added
for them at the same time. The four error paths of those functions are
tested as well, which is cool to see. Even if the buildfarm explodes
after 0002 and 0003, 0001 is still a good addition. The set looks good
to me by the way.
--
Michael

Re: [HACKERS] taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2018-02-01T06:47:00Z

On Wed, Jan 24, 2018 at 03:44:04PM +0900, Michael Paquier wrote:
> Good catch. Coverage reports mark those areas as empty! Similarly the
> functions for record_* are mostly not used. Some tests could be added
> for them at the same time. The four error paths of those functions are
> tested as well, which is cool to see. Even if the buildfarm explodes
> after 0002 and 0003, 0001 is still a good addition. The set looks good
> to me by the way.

OK, so those have been committed as a61116d, 0b5e33f6 and a6ef00b5 for
the record.  The last steps would be to:
- Introduce bool8 for catalogs.
- Address GinTernaryValue.
- Integrate stdbool.h.
Peter, are you planning to work on that for the next CF?
--
Michael

Re: [HACKERS] taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-02-01T14:04:57Z

On 2/1/18 01:47, Michael Paquier wrote:
> On Wed, Jan 24, 2018 at 03:44:04PM +0900, Michael Paquier wrote:
>> Good catch. Coverage reports mark those areas as empty! Similarly the
>> functions for record_* are mostly not used. Some tests could be added
>> for them at the same time. The four error paths of those functions are
>> tested as well, which is cool to see. Even if the buildfarm explodes
>> after 0002 and 0003, 0001 is still a good addition. The set looks good
>> to me by the way.
> 
> OK, so those have been committed as a61116d, 0b5e33f6 and a6ef00b5 for
> the record.  The last steps would be to:
> - Introduce bool8 for catalogs.
> - Address GinTernaryValue.
> - Integrate stdbool.h.
> Peter, are you planning to work on that for the next CF?

I've been testing this a bit further and during a test setup with 4-byte
bools I still got regression test failures related to GIN, so it doesn't
seem quite ready.  I'll keep working on it.

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

Re: [HACKERS] taking stdbool.h into use

Michael Paquier <michael.paquier@gmail.com> — 2018-02-02T04:16:22Z

On Thu, Feb 01, 2018 at 09:04:57AM -0500, Peter Eisentraut wrote:
> I've been testing this a bit further and during a test setup with 4-byte
> bools I still got regression test failures related to GIN, so it doesn't
> seem quite ready.  I'll keep working on it.

Cool.  Thanks for the update.
--
Michael

Re: [HACKERS] taking stdbool.h into use

Andres Freund <andres@anarazel.de> — 2018-03-01T10:28:54Z

On 2018-02-01 09:04:57 -0500, Peter Eisentraut wrote:
> I've been testing this a bit further and during a test setup with 4-byte
> bools I still got regression test failures related to GIN, so it doesn't
> seem quite ready.  I'll keep working on it.

Are you planning to get this into v11? The CF entry
https://commitfest.postgresql.org/17/1444/
lists this as "ready for committer" which doesn't seem to jive with the
above description?

Greetings,

Andres Freund

Re: [HACKERS] taking stdbool.h into use

Michael Paquier <michael@paquier.xyz> — 2018-03-02T04:34:35Z

On Thu, Mar 01, 2018 at 02:28:54AM -0800, Andres Freund wrote:
> On 2018-02-01 09:04:57 -0500, Peter Eisentraut wrote:
>> I've been testing this a bit further and during a test setup with 4-byte
>> bools I still got regression test failures related to GIN, so it doesn't
>> seem quite ready.  I'll keep working on it.
> 
> Are you planning to get this into v11? The CF entry
> https://commitfest.postgresql.org/17/1444/
> lists this as "ready for committer" which doesn't seem to jive with the
> above description?

Indeed, this is wrong.  Peter, why did you switch suddendly this patch
as ready for committer?  The patch is waiting for your input as you
mentioned that the GIN portion of this patch series is not completely
baked yet.  So I have switched the patch in this state.
--
Michael

Re: [HACKERS] taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-13T19:25:39Z

On 3/1/18 23:34, Michael Paquier wrote:
> Indeed, this is wrong.  Peter, why did you switch suddendly this patch
> as ready for committer?  The patch is waiting for your input as you
> mentioned that the GIN portion of this patch series is not completely
> baked yet.  So I have switched the patch in this state.

After more digging, there are more problems with having a bool that is
not 1 byte.  For example, pg_control has a bool field, so with a
different bool size, pg_control would be laid out differently.  That
would require changing all the mentions of bool to bool8 where the end
up on disk somehow, as I had already done for the system catalog
structures, but we don't know all the other places that would be affected.

So I'm going back to my proposal from December, to just use stdbool.h
when sizeof(bool) == 1, and add a static assertion to prevent other
configurations.

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

Re: [HACKERS] taking stdbool.h into use

Michael Paquier <michael@paquier.xyz> — 2018-03-20T06:13:05Z

On Tue, Mar 13, 2018 at 03:25:39PM -0400, Peter Eisentraut wrote:
> After more digging, there are more problems with having a bool that is
> not 1 byte.  For example, pg_control has a bool field, so with a
> different bool size, pg_control would be laid out differently.  That
> would require changing all the mentions of bool to bool8 where the end
> up on disk somehow, as I had already done for the system catalog
> structures, but we don't know all the other places that would be affected.
> 
> So I'm going back to my proposal from December, to just use stdbool.h
> when sizeof(bool) == 1, and add a static assertion to prevent other
> configurations.

So, on one side of the ring, we have more complicated patches to include
so as support for sizeof(bool) == 4 becomes possible in the backend
code, and on the opposite side one patch which restrains the use of
stdbool.h only when the size is 1.  A size of 4 bytes for bool is
defined in stdbool.h on a small set of platforms, so it could be
tempting to use what is proposed here, still that feels like a
halk-baked integration.  Thoughts from others?
--
Michael

Re: [HACKERS] taking stdbool.h into use

Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-20T06:18:54Z

Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Mar 13, 2018 at 03:25:39PM -0400, Peter Eisentraut wrote:
>> So I'm going back to my proposal from December, to just use stdbool.h
>> when sizeof(bool) == 1, and add a static assertion to prevent other
>> configurations.

> So, on one side of the ring, we have more complicated patches to include
> so as support for sizeof(bool) == 4 becomes possible in the backend
> code, and on the opposite side one patch which restrains the use of
> stdbool.h only when the size is 1.  A size of 4 bytes for bool is
> defined in stdbool.h on a small set of platforms, so it could be
> tempting to use what is proposed here, still that feels like a
> halk-baked integration.  Thoughts from others?

I think it'd be worth identifying exactly which platforms have
sizeof(bool) different from 1.  Are any of them things that anyone
cares about going forward?  The point of this patch is to ease
future development of extensions, but it's unlikely any extension
authors care about portability onto, say, macOS 10.4 (prairiedog).

			regards, tom lane

Re: [HACKERS] taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-20T21:04:22Z

On 3/20/18 02:18, Tom Lane wrote:
> I think it'd be worth identifying exactly which platforms have
> sizeof(bool) different from 1.  Are any of them things that anyone
> cares about going forward?  The point of this patch is to ease
> future development of extensions, but it's unlikely any extension
> authors care about portability onto, say, macOS 10.4 (prairiedog).

I'm not sure I follow.  Say we commit configure tests and discover that
platforms A, B, and C are affected.  What would we do with that
information?  I don't think we are saying we'd just break A, B, and C.

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

Re: [HACKERS] taking stdbool.h into use

Andres Freund <andres@anarazel.de> — 2018-03-20T21:14:23Z

Hi,

On 2018-03-20 17:04:22 -0400, Peter Eisentraut wrote:
> On 3/20/18 02:18, Tom Lane wrote:
> > I think it'd be worth identifying exactly which platforms have
> > sizeof(bool) different from 1.  Are any of them things that anyone
> > cares about going forward?  The point of this patch is to ease
> > future development of extensions, but it's unlikely any extension
> > authors care about portability onto, say, macOS 10.4 (prairiedog).
> 
> I'm not sure I follow.  Say we commit configure tests and discover that
> platforms A, B, and C are affected.  What would we do with that
> information?  I don't think we are saying we'd just break A, B, and C.

If those are only older platforms we could just not use stdbool for
those. The likelihood of getting into conflicts with $library stdbool
uses is lower...

Greetings,

Andres Freund

Re: [HACKERS] taking stdbool.h into use

Michael Paquier <michael@paquier.xyz> — 2018-03-21T03:24:41Z

On Tue, Mar 20, 2018 at 02:14:23PM -0700, Andres Freund wrote:
> On 2018-03-20 17:04:22 -0400, Peter Eisentraut wrote:
> > On 3/20/18 02:18, Tom Lane wrote:
> > > I think it'd be worth identifying exactly which platforms have
> > > sizeof(bool) different from 1.  Are any of them things that anyone
> > > cares about going forward?  The point of this patch is to ease
> > > future development of extensions, but it's unlikely any extension
> > > authors care about portability onto, say, macOS 10.4 (prairiedog).
> > 
> > I'm not sure I follow.  Say we commit configure tests and discover that
> > platforms A, B, and C are affected.  What would we do with that
> > information?  I don't think we are saying we'd just break A, B, and C.
> 
> If those are only older platforms we could just not use stdbool for
> those. The likelihood of getting into conflicts with $library stdbool
> uses is lower...

Yeah, I agree with that.  Just not using stdbool.h in those cases ought
to be fine.  Any platforms where sizeof(bool) is 4 involves macos older
than 10.5 and Windows platforms using MSVC versions older than 2003
(didn't look further down either).
--
Michael

Re: [HACKERS] taking stdbool.h into use

Andres Freund <andres@anarazel.de> — 2018-03-21T03:43:16Z


On March 20, 2018 8:24:41 PM PDT, Michael Paquier <michael@paquier.xyz> wrote:
>On Tue, Mar 20, 2018 at 02:14:23PM -0700, Andres Freund wrote:
>> On 2018-03-20 17:04:22 -0400, Peter Eisentraut wrote:
>> > On 3/20/18 02:18, Tom Lane wrote:
>> > > I think it'd be worth identifying exactly which platforms have
>> > > sizeof(bool) different from 1.  Are any of them things that
>anyone
>> > > cares about going forward?  The point of this patch is to ease
>> > > future development of extensions, but it's unlikely any extension
>> > > authors care about portability onto, say, macOS 10.4
>(prairiedog).
>> > 
>> > I'm not sure I follow.  Say we commit configure tests and discover
>that
>> > platforms A, B, and C are affected.  What would we do with that
>> > information?  I don't think we are saying we'd just break A, B, and
>C.
>> 
>> If those are only older platforms we could just not use stdbool for
>> those. The likelihood of getting into conflicts with $library stdbool
>> uses is lower...
>
>Yeah, I agree with that.  Just not using stdbool.h in those cases ought
>to be fine.  Any platforms where sizeof(bool) is 4 involves macos older
>than 10.5 and Windows platforms using MSVC versions older than 2003
>(didn't look further down either).

Aren't there some somewhat modern architectures where that's still the case, for performance reasons? PPC or such?

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [HACKERS] taking stdbool.h into use

Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-21T05:51:01Z

Andres Freund <andres@anarazel.de> writes:
> On March 20, 2018 8:24:41 PM PDT, Michael Paquier <michael@paquier.xyz> wrote:
>> Yeah, I agree with that.  Just not using stdbool.h in those cases ought
>> to be fine.  Any platforms where sizeof(bool) is 4 involves macos older
>> than 10.5 and Windows platforms using MSVC versions older than 2003
>> (didn't look further down either).

> Aren't there some somewhat modern architectures where that's still the case, for performance reasons? PPC or such?

Well, hydra (F16 on ppc64) has sizeof(bool) = 1.  Don't have any other
good datapoints handy.  Presumably we'd set up configure to report
what it found out, so it wouldn't take long to survey the buildfarm.

			regards, tom lane

Re: [HACKERS] taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-21T11:48:02Z

On 3/21/18 01:51, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On March 20, 2018 8:24:41 PM PDT, Michael Paquier <michael@paquier.xyz> wrote:
>>> Yeah, I agree with that.  Just not using stdbool.h in those cases ought
>>> to be fine.  Any platforms where sizeof(bool) is 4 involves macos older
>>> than 10.5 and Windows platforms using MSVC versions older than 2003
>>> (didn't look further down either).
> 
>> Aren't there some somewhat modern architectures where that's still the case, for performance reasons? PPC or such?
> 
> Well, hydra (F16 on ppc64) has sizeof(bool) = 1.  Don't have any other
> good datapoints handy.  Presumably we'd set up configure to report
> what it found out, so it wouldn't take long to survey the buildfarm.

I've pushed the configure tests.  Let's see what they say.

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

Re: [HACKERS] taking stdbool.h into use

Michael Paquier <michael@paquier.xyz> — 2018-03-22T01:42:20Z

On Wed, Mar 21, 2018 at 07:48:02AM -0400, Peter Eisentraut wrote:
> I've pushed the configure tests.  Let's see what they say.

At least the buildfarm is green.  Browsing the buildfarm logs manually
is kind of annoying for any normal human being.  I guess that you have
access to the database holding all this data, which could make that
easier to look.  What are those telling about SIZEOF_BOOL in
pg_config.h?
--
Michael

Re: [HACKERS] taking stdbool.h into use

Tom Lane <tgl@sss.pgh.pa.us> — 2018-03-22T02:07:30Z

Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Mar 21, 2018 at 07:48:02AM -0400, Peter Eisentraut wrote:
>> I've pushed the configure tests.  Let's see what they say.

> At least the buildfarm is green.  Browsing the buildfarm logs manually
> is kind of annoying for any normal human being.  I guess that you have
> access to the database holding all this data, which could make that
> easier to look.  What are those telling about SIZEOF_BOOL in
> pg_config.h?

AFAIK, the pginfra team will give access to the buildfarm database
to anyone reasonably well known in the community, so if you want
to poke through that, just ask.  In the meantime, here's what
I just got from

select distinct sysname, l from
   (select sysname, unnest(string_to_array(log_text, E'\n')) as l from build_status_log
    where log_stage = 'configure.log' and snapshot > '2018-03-20') ss
where l ~ 'bool'
order by 1

    sysname    |                                      l                                      
---------------+-----------------------------------------------------------------------------
 aholehole     | checking for stdbool.h that conforms to C99... yes
 aholehole     | checking size of bool... 1
 anole         | checking for stdbool.h that conforms to C99... yes
 anole         | checking size of bool... 1
 caiman        | checking for stdbool.h that conforms to C99... yes
 caiman        | checking size of bool... 1
 calliphoridae | checking for stdbool.h that conforms to C99... (cached) yes
 calliphoridae | checking for stdbool.h that conforms to C99... yes
 calliphoridae | checking size of bool... 1
 calliphoridae | checking size of bool... (cached) 1
 capybara      | checking for stdbool.h that conforms to C99... yes
 capybara      | checking size of bool... 1
 castoroides   | checking for stdbool.h that conforms to C99... no
 castoroides   | checking size of bool... 0
 chub          | checking for stdbool.h that conforms to C99... yes
 chub          | checking size of bool... 1
 clam          | checking for stdbool.h that conforms to C99... yes
 clam          | checking size of bool... 1
 conchuela     | checking for stdbool.h that conforms to C99... yes
 conchuela     | checking size of bool... 1
 coypu         | checking for stdbool.h that conforms to C99... yes
 coypu         | checking size of bool... 1
 crake         | Mar 21 09:15:52 checking for stdbool.h that conforms to C99... yes
 crake         | Mar 21 09:15:59 checking size of bool... 1
 crake         | Mar 21 09:37:25 checking for stdbool.h that conforms to C99... (cached) yes
 crake         | Mar 21 09:37:25 checking size of bool... (cached) 1
 crake         | Mar 21 11:46:41 checking for stdbool.h that conforms to C99... (cached) yes
 crake         | Mar 21 11:46:41 checking size of bool... (cached) 1
 crake         | Mar 21 12:38:09 checking for stdbool.h that conforms to C99... (cached) yes
 crake         | Mar 21 12:38:09 checking size of bool... (cached) 1
 crake         | Mar 21 15:38:50 checking for stdbool.h that conforms to C99... (cached) yes
 crake         | Mar 21 15:38:50 checking size of bool... (cached) 1
 crake         | Mar 21 18:37:24 checking for stdbool.h that conforms to C99... (cached) yes
 crake         | Mar 21 18:37:24 checking size of bool... (cached) 1
 culicidae     | checking for stdbool.h that conforms to C99... (cached) yes
 culicidae     | checking for stdbool.h that conforms to C99... yes
 culicidae     | checking size of bool... 1
 culicidae     | checking size of bool... (cached) 1
 curculio      | checking for stdbool.h that conforms to C99... yes
 curculio      | checking size of bool... 1
 dangomushi    | checking for stdbool.h that conforms to C99... yes
 dangomushi    | checking size of bool... 1
 dromedary     | checking for stdbool.h that conforms to C99... (cached) yes
 dromedary     | checking for stdbool.h that conforms to C99... yes
 dromedary     | checking size of bool... 1
 dromedary     | checking size of bool... (cached) 1
 dunlin        | checking for stdbool.h that conforms to C99... yes
 dunlin        | checking size of bool... 1
 francolin     | checking for stdbool.h that conforms to C99... (cached) yes
 francolin     | checking for stdbool.h that conforms to C99... yes
 francolin     | checking size of bool... 1
 francolin     | checking size of bool... (cached) 1
 fulmar        | checking for stdbool.h that conforms to C99... yes
 fulmar        | checking size of bool... 1
 gharial       | checking for stdbool.h that conforms to C99... yes
 gharial       | checking size of bool... 1
 grison        | checking for stdbool.h that conforms to C99... yes
 grison        | checking size of bool... 1
 guaibasaurus  | checking for stdbool.h that conforms to C99... yes
 guaibasaurus  | checking size of bool... 1
 handfish      | checking for stdbool.h that conforms to C99... yes
 handfish      | checking size of bool... 1
 hornet        | checking for stdbool.h that conforms to C99... yes
 hornet        | checking size of bool... 1
 jacana        | Mar 21 12:51:12 checking for stdbool.h that conforms to C99... yes
 jacana        | Mar 21 12:53:25 checking size of bool... 1
 katydid       | checking for stdbool.h that conforms to C99... yes
 katydid       | checking size of bool... 1
 lapwing       | checking for stdbool.h that conforms to C99... yes
 lapwing       | checking size of bool... 1
 loach         | checking for stdbool.h that conforms to C99... yes
 loach         | checking size of bool... 1
 locust        | checking for stdbool.h that conforms to C99... (cached) yes
 locust        | checking for stdbool.h that conforms to C99... yes
 locust        | checking size of bool... 4
 locust        | checking size of bool... (cached) 4
 longfin       | checking for stdbool.h that conforms to C99... (cached) yes
 longfin       | checking for stdbool.h that conforms to C99... yes
 longfin       | checking size of bool... 1
 longfin       | checking size of bool... (cached) 1
 lorikeet      | checking for stdbool.h that conforms to C99... (cached) yes
 lorikeet      | checking for stdbool.h that conforms to C99... yes
 lorikeet      | checking size of bool... 1
 lorikeet      | checking size of bool... (cached) 1
 lousyjack     | checking for stdbool.h that conforms to C99... (cached) yes
 lousyjack     | checking for stdbool.h that conforms to C99... yes
 lousyjack     | checking size of bool... 1
 lousyjack     | checking size of bool... (cached) 1
 macaque       | checking for stdbool.h that conforms to C99... yes
 macaque       | checking size of bool... 1
 magpie        | checking for stdbool.h that conforms to C99... yes
 magpie        | checking size of bool... 1
 mantid        | checking for stdbool.h that conforms to C99... yes
 mantid        | checking size of bool... 1
 moonjelly     | checking for stdbool.h that conforms to C99... (cached) yes
 moonjelly     | checking for stdbool.h that conforms to C99... yes
 moonjelly     | checking size of bool... 1
 moonjelly     | checking size of bool... (cached) 1
 mule          | checking for stdbool.h that conforms to C99... (cached) yes
 mule          | checking for stdbool.h that conforms to C99... yes
 mule          | checking size of bool... 1
 mule          | checking size of bool... (cached) 1
 mylodon       | checking for stdbool.h that conforms to C99... (cached) yes
 mylodon       | checking for stdbool.h that conforms to C99... yes
 mylodon       | checking size of bool... 1
 mylodon       | checking size of bool... (cached) 1
 nightjar      | checking for stdbool.h that conforms to C99... (cached) yes
 nightjar      | checking for stdbool.h that conforms to C99... yes
 nightjar      | checking size of bool... 1
 nightjar      | checking size of bool... (cached) 1
 okapi         | checking for stdbool.h that conforms to C99... yes
 okapi         | checking size of bool... 1
 piculet       | checking for stdbool.h that conforms to C99... (cached) yes
 piculet       | checking for stdbool.h that conforms to C99... yes
 piculet       | checking size of bool... 1
 piculet       | checking size of bool... (cached) 1
 prairiedog    | checking for stdbool.h that conforms to C99... (cached) yes
 prairiedog    | checking for stdbool.h that conforms to C99... yes
 prairiedog    | checking size of bool... 4
 prairiedog    | checking size of bool... (cached) 4
 prion         | checking for stdbool.h that conforms to C99... (cached) yes
 prion         | checking for stdbool.h that conforms to C99... yes
 prion         | checking size of bool... 1
 prion         | checking size of bool... (cached) 1
 protosciurus  | checking for stdbool.h that conforms to C99... no
 protosciurus  | checking size of bool... 0
 quokka        | checking for stdbool.h that conforms to C99... yes
 quokka        | checking size of bool... 1
 rhinoceros    | checking for stdbool.h that conforms to C99... (cached) yes
 rhinoceros    | checking for stdbool.h that conforms to C99... yes
 rhinoceros    | checking size of bool... 1
 rhinoceros    | checking size of bool... (cached) 1
 rover_firefly | checking for stdbool.h that conforms to C99... yes
 rover_firefly | checking size of bool... 1
 seawasp       | checking for stdbool.h that conforms to C99... (cached) yes
 seawasp       | checking for stdbool.h that conforms to C99... yes
 seawasp       | checking size of bool... 1
 seawasp       | checking size of bool... (cached) 1
 sidewinder    | checking for stdbool.h that conforms to C99... yes
 sidewinder    | checking size of bool... 1
 spurfowl      | checking for stdbool.h that conforms to C99... yes
 spurfowl      | checking size of bool... 1
 sungazer      | checking for stdbool.h that conforms to C99... yes
 sungazer      | checking size of bool... 1
 termite       | checking for stdbool.h that conforms to C99... yes
 termite       | checking size of bool... 1
 tern          | checking for stdbool.h that conforms to C99... yes
 tern          | checking size of bool... 1
 treepie       | checking for stdbool.h that conforms to C99... yes
 treepie       | checking size of bool... 1
(150 rows)

I don't think all of the slower buildfarm members have checked in
yet, but so far it's looking like ancient-macOS is the only platform
with sizeof(bool) different from 1.

			regards, tom lane

Re: [HACKERS] taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-23T02:17:29Z

On 3/21/18 07:48, Peter Eisentraut wrote:
> On 3/21/18 01:51, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> On March 20, 2018 8:24:41 PM PDT, Michael Paquier <michael@paquier.xyz> wrote:
>>>> Yeah, I agree with that.  Just not using stdbool.h in those cases ought
>>>> to be fine.  Any platforms where sizeof(bool) is 4 involves macos older
>>>> than 10.5 and Windows platforms using MSVC versions older than 2003
>>>> (didn't look further down either).
>>
>>> Aren't there some somewhat modern architectures where that's still the case, for performance reasons? PPC or such?
>>
>> Well, hydra (F16 on ppc64) has sizeof(bool) = 1.  Don't have any other
>> good datapoints handy.  Presumably we'd set up configure to report
>> what it found out, so it wouldn't take long to survey the buildfarm.
> 
> I've pushed the configure tests.  Let's see what they say.

So after a day, only the old macOS PowerPC systems have sizeof(bool) == 4.

I have committed the rest of this patch now.

Windows could use some manual adjustments in pg_config.h.win32 if anyone
cares.  (That file also needs some more updates.)

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

Re: [HACKERS] taking stdbool.h into use

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-23T02:23:49Z

On 3/22/18 22:17, Peter Eisentraut wrote:
> So after a day, only the old macOS PowerPC systems have sizeof(bool) == 4.

And this is causing some problems in PL/Perl.  I'll look into it.

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