Re: [HACKERS] taking stdbool.h into use

[Peter Eisentraut]

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

Re: [HACKERS] taking stdbool.h into use

[Peter Eisentraut]

On 3/21/18 07:48, Peter Eisentraut wrote:
> On 3/21/18 01:51, Tom Lane wrote:
>> Andres Freund  writes:
>>> On March 20, 2018 8:24:41 PM PDT, Michael Paquier  
>>> 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

[Tom Lane]

Michael Paquier  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 

Re: [HACKERS] taking stdbool.h into use

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: [HACKERS] taking stdbool.h into use

[Peter Eisentraut]

On 3/21/18 01:51, Tom Lane wrote:
> Andres Freund  writes:
>> On March 20, 2018 8:24:41 PM PDT, Michael Paquier  
>> 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

[Tom Lane]

Andres Freund  writes:
> On March 20, 2018 8:24:41 PM PDT, Michael Paquier  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

[Andres Freund]

On March 20, 2018 8:24:41 PM PDT, Michael Paquier  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

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: [HACKERS] taking stdbool.h into use

[Peter Eisentraut]

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

[Tom Lane]

Michael Paquier  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

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: [HACKERS] taking stdbool.h into use

[Peter Eisentraut]

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
>From daedc3ea6d044dde18cc0c630085a7f55e764794 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 19 Dec 2017 13:54:05 -0500
Subject: [PATCH v6 1/2] Add static assertions about size of GinTernaryValue vs
 bool

We need these to be the same because some code casts between pointers to
them.
---
 src/backend/utils/adt/tsginidx.c | 4 +++-
 src/include/access/gin.h | 4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/backend/utils/adt/tsginidx.c b/src/backend/utils/adt/tsginidx.c
index de59e6417e..00e32b2570 100644
--- a/src/backend/utils/adt/tsginidx.c
+++ b/src/backend/utils/adt/tsginidx.c
@@ -309,7 +309,9 @@ gin_tsquery_consistent(PG_FUNCTION_ARGS)
 * query.
 */
gcv.first_item = GETQUERY(query);
-   gcv.check = check;
+   StaticAssertStmt(sizeof(GinTernaryValue) == sizeof(bool),
+"sizes of GinTernaryValue and 
bool are not equal");
+   gcv.check = (GinTernaryValue *) check;
gcv.map_item_operand = (int *) (extra_data[0]);
gcv.need_recheck = recheck;
 
diff --git a/src/include/access/gin.h b/src/include/access/gin.h
index 0acdb88241..3d8a130b69 100644
--- a/src/include/access/gin.h
+++ b/src/include/access/gin.h
@@ -51,8 +51,8 @@ typedef struct GinStatsData
 /*
  * A ternary value used by tri-consistent functions.
  *
- * For convenience, this is compatible with booleans. A boolean can be
- * safely cast to a GinTernaryValue.
+ * This must be of the same size as a bool because some code will cast a
+ * pointer to a bool to a pointer to a GinTernaryValue.
  */
 typedef char GinTernaryValue;
 

base-commit: 17bb62501787c56e0518e61db13a523d47afd724
-- 
2.16.2

>From 67f32f4195632fd1068bf3ad0bf93c12a865c7d4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 19 Dec 2017 14:24:43 -0500
Subject: [PATCH v6 2/2] Use stdbool.h if suitable

Using the standard bool type provided by C allows some recent compilers
and debuggers to give better diagnostics.  Also, some extension code and
third-party headers are increasingly pulling in stdbool.h, so it's
probably saner if everyone uses the same definition.

But PostgreSQL code is not prepared to handle bool of a size other than
1, so we keep our own old definition if we encounter a stdbool.h with a
bool of a different size.  (Apparently, that includes Power CPUs and
some very old compilers that declared bool to be an enum.)  We have
static assertions in critical places that check that bool is of the
right size.
---
 configure  | 213 -
 configure.in   |   7 ++
 src/include/c.h|  14 ++-
 src/include/pg_config.h.in |   9 ++
 src/pl/plperl/plperl.h |  10 +--
 5 files changed, 205 insertions(+), 48 deletions(-)

diff --git a/configure b/configure
index 3943711283..c670becd16 100755
--- a/configure
+++ b/configure
@@ -1999,116 +1999,116 @@ $as_echo "$ac_res" >&6; }
 
 } # ac_fn_c_check_func
 
-# ac_fn_c_check_member LINENO AGGR MEMBER VAR INCLUDES
-# 
-# Tries to find if the field MEMBER exists in type AGGR, after including
-# INCLUDES, setting cache variable VAR accordingly.
-ac_fn_c_check_member ()
+# ac_fn_c_check_type LINENO TYPE VAR INCLUDES
+# ---
+# Tests whether TYPE exists after having included INCLUDES, setting cache
+# variable VAR accordingly.
+ac_fn_c_check_type ()
 {
   as_lineno=${as_lineno-"$1"} as_lineno_stack=as_lineno_stack=$as_lineno_stack
-  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for $2.$3" >&5
-$as_echo_n "checking for $2.$3... " >&6; }
-if eval \${$4+:} false; then :
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for $2" >&5
+$as_echo_n "checking for $2... " >&6; }
+if eval \${$3+:} false; then :
   $as_echo_n "(cached) " 

Re: [HACKERS] taking stdbool.h into use

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: [HACKERS] taking stdbool.h into use

[Andres Freund]

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]

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


signature.asc
Description: PGP signature

Re: [HACKERS] taking stdbool.h into use

[Peter Eisentraut]

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]

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


signature.asc
Description: PGP signature

Re: [HACKERS] taking stdbool.h into use

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: [HACKERS] taking stdbool.h into use

[Peter Eisentraut]

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
From 686b2a6f2c0a455dccbecf07d163af5d6f9c9e88 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 23 Jan 2018 10:13:45 -0500
Subject: [PATCH 1/3] Add tests for record_image_eq and record_image_cmp

record_image_eq was covered a bit by the materialized view code that it
is meant to support, but record_image_cmp was not tested at all.
---
 src/test/regress/expected/rowtypes.out | 161 +
 src/test/regress/sql/rowtypes.sql  |  53 +++
 2 files changed, 214 insertions(+)

diff --git a/src/test/regress/expected/rowtypes.out 
b/src/test/regress/expected/rowtypes.out
index a4bac8e3b5..e3c23a41cd 100644
--- a/src/test/regress/expected/rowtypes.out
+++ b/src/test/regress/expected/rowtypes.out
@@ -369,6 +369,167 @@ LINE 1: select * from cc order by f1;
   ^
 HINT:  Use an explicit ordering operator or modify the query.
 --
+-- Tests for record_image_{eq,cmp}
+--
+create type testtype1 as (a int, b int);
+-- all true
+select row(1, 2)::testtype1 *< row(1, 3)::testtype1;
+ ?column? 
+--
+ t
+(1 row)
+
+select row(1, 2)::testtype1 *<= row(1, 3)::testtype1;
+ ?column? 
+--
+ t
+(1 row)
+
+select row(1, 2)::testtype1 *= row(1, 2)::testtype1;
+ ?column? 
+--
+ t
+(1 row)
+
+select row(1, 2)::testtype1 *<> row(1, 3)::testtype1;
+ ?column? 
+--
+ t
+(1 row)
+
+select row(1, 3)::testtype1 *>= row(1, 2)::testtype1;
+ ?column? 
+--
+ t
+(1 row)
+
+select row(1, 3)::testtype1 *> row(1, 2)::testtype1;
+ ?column? 
+--
+ t
+(1 row)
+
+-- all false
+select row(1, -2)::testtype1 *< row(1, -3)::testtype1;
+ ?column? 
+--
+ f
+(1 row)
+
+select row(1, -2)::testtype1 *<= row(1, -3)::testtype1;
+ ?column? 
+--
+ f
+(1 row)
+
+select row(1, -2)::testtype1 *= row(1, -3)::testtype1;
+ ?column? 
+--
+ f
+(1 row)
+
+select row(1, -2)::testtype1 *<> row(1, -2)::testtype1;
+ ?column? 
+--
+ f
+(1 row)
+
+select row(1, -3)::testtype1 *>= row(1, -2)::testtype1;
+ ?column? 
+--
+ f
+(1 row)
+
+select row(1, -3)::testtype1 *> row(1, -2)::testtype1;
+ ?column? 
+--
+ f
+(1 row)
+
+-- This returns the "wrong" order because record_image_cmp works on
+-- unsigned datums without knowing about the actual data type.
+select row(1, -2)::testtype1 *< row(1, 3)::testtype1;
+ ?column? 
+--
+ f
+(1 row)
+
+-- other types
+create type testtype2 as (a smallint, b bool);  -- byval different sizes
+select row(1, true)::testtype2 *< row(2, true)::testtype2;
+ ?column? 
+--
+ t
+(1 row)
+
+select row(-2, true)::testtype2 *< row(-1, true)::testtype2;
+ ?column? 
+--
+ t
+(1 row)
+
+select row(0, false)::testtype2 *< row(0, true)::testtype2;
+ ?column? 
+--
+ t
+(1 row)
+
+select row(0, false)::testtype2 *<> row(0, true)::testtype2;
+ ?column? 
+--
+ t
+(1 row)
+
+create type testtype3 as (a int, b text);  -- variable length
+select row(1, 'abc')::testtype3 *< row(1, 'abd')::testtype3;
+ ?column? 
+--
+ t
+(1 row)
+
+select row(1, 'abc')::testtype3 *< row(1, 'abcd')::testtype3;
+ ?column? 
+--
+ t
+(1 row)
+
+select row(1, 'abc')::testtype3 *> row(1, 'abd')::testtype3;
+ ?column? 
+--
+ f
+(1 row)
+
+select row(1, 'abc')::testtype3 *<> row(1, 'abd')::testtype3;
+ ?column? 
+--
+ t
+(1 row)
+
+create type testtype4 as (a int, b point);  -- by ref, fixed length
+select row(1, '(1,2)')::testtype4 *< row(1, 

Re: [HACKERS] taking stdbool.h into use

[Peter Eisentraut]

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

[Tom Lane]

Peter Eisentraut  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

[Peter Eisentraut]

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

[Michael Paquier]

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...@gemulon.postgresql.org
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] taking stdbool.h into use

[Tom Lane]

Michael Paquier  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]

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


signature.asc
Description: PGP signature

Re: [HACKERS] taking stdbool.h into use

[Michael Paquier]

On Fri, Dec 29, 2017 at 12:33:24PM -0500, Tom Lane wrote:
> Alvaro Herrera  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


signature.asc
Description: PGP signature

Re: [HACKERS] taking stdbool.h into use

[Tom Lane]

Alvaro Herrera  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

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] taking stdbool.h into use

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: [HACKERS] taking stdbool.h into use

[Peter Eisentraut]

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
>From 1cc402987b4f296c2ce9138d0e5741a16cae3aca Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 16 Aug 2017 00:22:32 -0400
Subject: [PATCH v5 1/4] Add bool8 typedef for system catalog structs

For system catalog structs, we require that the C bool type is 1 byte in
size, the same as the SQL type.  But if stdbool.h is used, then this is
not guaranteed.  To address that, define a separate type bool8 that is
guaranteed to be 1 byte, and use that in the system catalog structs.
Some tweaking in Catalog.pm ensures that bool is not accidentally used
where bool8 should be used.
---
 src/backend/catalog/Catalog.pm|  2 ++
 src/include/c.h   |  9 +
 src/include/catalog/pg_aggregate.h|  4 ++--
 src/include/catalog/pg_attribute.h| 10 +-
 src/include/catalog/pg_auth_members.h |  2 +-
 src/include/catalog/pg_authid.h   | 14 +++---
 src/include/catalog/pg_class.h| 22 +++---
 src/include/catalog/pg_constraint.h   | 10 +-
 src/include/catalog/pg_conversion.h   |  2 +-
 src/include/catalog/pg_database.h |  4 ++--
 src/include/catalog/pg_extension.h|  2 +-
 src/include/catalog/pg_index.h| 20 ++--
 src/include/catalog/pg_language.h |  4 ++--
 src/include/catalog/pg_opclass.h  |  2 +-
 src/include/catalog/pg_operator.h |  4 ++--
 src/include/catalog/pg_pltemplate.h   |  4 ++--
 src/include/catalog/pg_policy.h   |  2 +-
 src/include/catalog/pg_proc.h | 12 ++--
 src/include/catalog/pg_publication.h  |  8 
 src/include/catalog/pg_rewrite.h  |  2 +-
 src/include/catalog/pg_sequence.h |  2 +-
 src/include/catalog/pg_statistic.h|  2 +-
 src/include/catalog/pg_subscription.h |  2 +-
 src/include/catalog/pg_trigger.h  |  6 +++---
 src/include/catalog/pg_type.h |  8 
 src/include/commands/sequence.h   |  2 +-
 26 files changed, 86 insertions(+), 75 deletions(-)

diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index 80bd9771f1..2602c68867 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -26,6 +26,8 @@ sub Catalogs
# There are a few types which are given one name in the C source, but a
# different name at the SQL level.  These are enumerated here.
my %RENAME_ATTTYPE = (
+   'bool'  => 'INVALID',  # use bool8 instead
+   'bool8' => 'bool',
'int16' => 'int2',
'int32' => 'int4',
'int64' => 'int8',
diff --git a/src/include/c.h b/src/include/c.h
index 22535a7deb..20c15a0d9d 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -267,6 +267,15 @@ typedef char bool;
 
 #endif /* not C++ */
 
+/*
+ * bool8
+ *
+ * A bool type that is guaranteed to be 8 bits/1 byte, mainly for use in
+ * system catalog definitions.  (stdbool.h's bool is not 1 byte on all
+ * platforms.)
+ */
+typedef char bool8;
+
 
 /* 
  * Section 3:  standard system types
diff --git a/src/include/catalog/pg_aggregate.h 
b/src/include/catalog/pg_aggregate.h
index 13f1bce5af..a8894b9aad 100644
--- a/src/include/catalog/pg_aggregate.h
+++ b/src/include/catalog/pg_aggregate.h
@@ -65,8 +65,8 @@ CATALOG(pg_aggregate,2600) BKI_WITHOUT_OIDS
regproc aggmtransfn;
regproc aggminvtransfn;
regproc aggmfinalfn;
-   boolaggfinalextra;
-   boolaggmfinalextra;
+   bool8   aggfinalextra;
+   bool8   aggmfinalextra;
charaggfinalmodify;
charaggmfinalmodify;
Oid aggsortop;
diff --git a/src/include/catalog/pg_attribute.h 
b/src/include/catalog/pg_attribute.h
index bcf28e8f04..a2007560ae 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -104,7 +104,7 @@ CATALOG(pg_attribute,1249) BKI_BOOTSTRAP BKI_WITHOUT_OIDS 
BKI_ROWTYPE_OID(75) BK
 * attbyval is a copy of the typbyval field from pg_type for this
 * attribute.  See atttypid comments above.
 */
-   boolattbyval;
+   bool8   attbyval;
 
/*--
 * attstorage tells for VARLENA attributes, what the heap access
@@ -128,16 +128,16 @@ CATALOG(pg_attribute,1249) BKI_BOOTSTRAP BKI_WITHOUT_OIDS 
BKI_ROWTYPE_OID(75) BK

Re: [HACKERS] taking stdbool.h into use

[Peter Eisentraut]

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]

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]

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
> >>  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


signature.asc
Description: PGP signature

Re: [HACKERS] taking stdbool.h into use

[Peter Eisentraut]

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
>>  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
From dc47c84286345eae48582df42bd977e369b4f341 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 26 Dec 2017 13:47:18 -0500
Subject: [PATCH] Don't cast between GinNullCategory and bool

The original idea was that we could use a isNull-style bool array
directly as a GinNullCategory array.  However, the existing code already
acknowledges that that doesn't really work, because of the possibility
that bool as currently defined can have arbitrary bit patterns for true
values.  So it has to loop through the nullFlags array to set each bool
value to and acceptable value.  But if we are looping through the whole
array anyway, we might as well build a proper GinNullCategory array
instead and abandon the type casting.  That makes the code much safer in
case bool is ever changed to something else.
---
 src/backend/access/gin/ginscan.c | 19 ---
 src/backend/access/gin/ginutil.c | 18 --
 src/include/access/ginblock.h|  7 +--
 3 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/src/backend/access/gin/ginscan.c b/src/backend/access/gin/ginscan.c
index 7ceea7a741..77c0c577b5 100644
--- a/src/backend/access/gin/ginscan.c
+++ b/src/backend/access/gin/ginscan.c
@@ -295,6 +295,7 @@ ginNewScanKey(IndexScanDesc scan)
bool   *partial_matches = NULL;
Pointer*extra_data = NULL;
bool   *nullFlags = NULL;
+   GinNullCategory *categories;
int32   searchMode = GIN_SEARCH_MODE_DEFAULT;
 
/*
@@ -346,15 +347,12 @@ ginNewScanKey(IndexScanDesc scan)
}
 
/*
-* If the extractQueryFn didn't create a nullFlags array, 
create one,
-* assuming that everything's non-null.  Otherwise, run through 
the
-* array and make sure each value is exactly 0 or 1; this 
ensures
-* binary compatibility with the GinNullCategory 
representation. While
-* at it, detect whether any null keys are present.
+* Create GinNullCategory representation.  If the extractQueryFn
+* didn't create a nullFlags array, we assume everything is 
non-null.
+* While at it, detect whether any null keys are present.
 */
-   if (nullFlags == NULL)
-   nullFlags = (bool *) palloc0(nQueryValues * 
sizeof(bool));
-   else
+   categories = (GinNullCategory *) palloc0(nQueryValues * 
sizeof(GinNullCategory));
+   if (nullFlags)
{
int32   j;
 
@@ -362,17 +360,16 @@ ginNewScanKey(IndexScanDesc scan)
{
if (nullFlags[j])
{
-   nullFlags[j] = true;/* not any 
other nonzero value */
+   categories[j] = GIN_CAT_NULL_KEY;
hasNullQuery = true;
}
}
}
-   /* now we can use the nullFlags as category codes */
 
ginFillScanKey(so, skey->sk_attno,
   skey->sk_strategy, searchMode,
   skey->sk_argument, nQueryValues,
-  queryValues, 

Re: [HACKERS] taking stdbool.h into use

[Michael Paquier]

On Thu, Dec 21, 2017 at 1:02 AM, Peter Eisentraut
 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
>  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]

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
 only if its bool has size 1.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From c9b5eb5024fcfc48bee93d621778dcacba8dd575 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 15 Dec 2017 17:01:03 -0500
Subject: [PATCH v4 1/3] Add static assertions about size of GinNullCategory vs
 bool

We need these to be the same because some code uses an array of one as
the other.  If bool were a different size (e.g., because stdbool.h is
used, which could make sizeof(bool) == 4 in some configurations), then
this would silently break.  Since GinNullCategory is signed char and
bool is a char of unspecified signedness, there need to be explicit
casts between the two, otherwise the compiler would complain.  But those
casts also silently hide any size mismatches.
---
 src/backend/access/gin/ginscan.c | 6 +-
 src/backend/access/gin/ginutil.c | 7 ++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/backend/access/gin/ginscan.c b/src/backend/access/gin/ginscan.c
index 7ceea7a741..eaf4ae6fdf 100644
--- a/src/backend/access/gin/ginscan.c
+++ b/src/backend/access/gin/ginscan.c
@@ -367,8 +367,12 @@ ginNewScanKey(IndexScanDesc scan)
}
}
}
-   /* now we can use the nullFlags as category codes */
 
+   /*
+* now we can use the nullFlags as category codes
+*/
+   StaticAssertStmt(sizeof(GinNullCategory) == sizeof(bool),
+"sizes of GinNullCategory and 
bool are not equal");
ginFillScanKey(so, skey->sk_attno,
   skey->sk_strategy, searchMode,
   skey->sk_argument, nQueryValues,
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index d9c6483437..28912e416a 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -540,7 +540,12 @@ ginExtractEntries(GinState *ginstate, OffsetNumber attnum,
for (i = 0; i < *nentries; i++)
nullFlags[i] = (nullFlags[i] ? true : false);
}
-   /* now we can use the nullFlags as category codes */
+
+   /*
+* 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;
 
/*

base-commit: 7d3583ad9ae54b44119973a9d6d731c9cc74c86e
-- 
2.15.1

From b1b89da6ccae2778c4d2561ffd3ba522c56586a4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 19 Dec 2017 13:54:05 -0500
Subject: [PATCH v4 2/3] Add static assertions about size of GinTernaryValue vs
 bool

We need these to be the same because some code casts between pointers to
them.
---
 src/backend/utils/adt/tsginidx.c | 4 +++-
 src/include/access/gin.h | 4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/backend/utils/adt/tsginidx.c b/src/backend/utils/adt/tsginidx.c
index aba456ed88..23b98a3d50 100644
--- a/src/backend/utils/adt/tsginidx.c
+++ b/src/backend/utils/adt/tsginidx.c
@@ -309,7 +309,9 @@ gin_tsquery_consistent(PG_FUNCTION_ARGS)
 * query.
 */
gcv.first_item = GETQUERY(query);
-   gcv.check = check;
+   StaticAssertStmt(sizeof(GinTernaryValue) == sizeof(bool),
+"sizes of GinTernaryValue and 
bool are not equal");
+   gcv.check = (GinTernaryValue *) check;
gcv.map_item_operand = (int *) (extra_data[0]);
gcv.need_recheck = recheck;
 
diff --git a/src/include/access/gin.h b/src/include/access/gin.h
index ec83058095..19fdf132b4 100644
--- a/src/include/access/gin.h
+++ b/src/include/access/gin.h
@@ -51,8 +51,8 @@ typedef struct GinStatsData
 /*
  * A ternary value used by tri-consistent functions.
  *
- *