Обсуждение: [HACKERS] taking stdbool.h into use
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
- 0001-Fix-bool-int-type-confusion.patch
- 0002-Change-TRUE-FALSE-to-true-false.patch
- 0003-Remove-TRUE-and-FALSE.patch
- 0004-Remove-BoolPtr-type.patch
- 0005-Make-casting-between-bool-and-GinTernaryValue-more-r.patch
- 0006-Add-bool8-typedef-for-system-catalog-structs.patch
- 0007-Avoid-use-of-bool-in-thread_test.c.patch
- 0008-Use-stdbool.h-if-available.patch
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: intbc = FALSE;
src/interfaces/ecpg/pgtypeslib/dt_common.c: intis2digits = FALSE;
src/interfaces/ecpg/pgtypeslib/dt_common.c: inthaveTextMonth = FALSE;
src/interfaces/ecpg/pgtypeslib/dt_common.c: intis2digits = FALSE;
src/interfaces/ecpg/pgtypeslib/dt_common.c: intbc = 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
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_PPCPower 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
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
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
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_testUSING 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
twocode values are chosen to be* compatible with the usual usage of bool isNull flags.** GIN_CAT_EMPTY_QUERY is never
storedin 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
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
- v2-0001-Fix-incorrect-use-of-bool.patch
- v2-0002-ecpg-Remove-useless-return-values.patch
- v2-0003-ecpg-Use-bool-instead-of-int.patch
- v2-0004-Change-TRUE-FALSE-to-true-false.patch
- v2-0005-Change-TRUE-FALSE-to-true-false-in-comments.patch
- v2-0006-Remove-TRUE-and-FALSE.patch
- v2-0007-Add-bool8-typedef-for-system-catalog-structs.patch
- v2-0008-Make-casting-between-bool-and-GinTernaryValue-mor.patch
- v2-0009-Use-stdbool.h-if-available.patch
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
- v3-0001-Fix-incorrect-use-of-bool.patch
- v3-0002-ecpg-Remove-useless-return-values.patch
- v3-0003-ecpg-Use-bool-instead-of-int.patch
- v3-0004-Change-TRUE-FALSE-to-true-false.patch
- v3-0005-Change-TRUE-FALSE-to-true-false-in-comments.patch
- v3-0006-Remove-TRUE-and-FALSE.patch
- v3-0007-Add-bool8-typedef-for-system-catalog-structs.patch
- v3-0008-Make-casting-between-bool-and-GinTernaryValue-mor.patch
- v3-0009-Use-stdbool.h-if-available.patch
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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
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
Вложения
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
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
Вложения
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
Вложения
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
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
Вложения
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
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
Вложения
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
Вложения
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
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
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
Вложения
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
Вложения
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
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
Вложения
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
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
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
Вложения
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
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
Вложения
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
Вложения
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
Вложения
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
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
Вложения
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
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
Вложения
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
Вложения
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
Вложения
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
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
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
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
Вложения
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.
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
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
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
Вложения
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
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
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