Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2016-03-18 14:36:23 +0300, Yury Zhuravlev wrote: > Robert Haas wrote: > >On Wed, Mar 2, 2016 at 9:48 PM, Peter Eisentrautwrote: > >>On 2/11/16 9:30 PM, Michael Paquier wrote: ... > > > >We need to decide what to do about this. I disagree with Peter: I > >think that regardless of stdbool, what we've got right now is sloppy > >coding - bad style if nothing else. Furthermore, I think that while C > >lets you use any non-zero value to represent true, our bool type is > >supposed to contain only one of those two values. Therefore, I think > >we should commit the full patch, back-patch it as far as somebody has > >the energy for, and move on. But regardless, this patch can't keep > >sitting in the CommitFest - we either have to take it or reject it, > >and soon. > > > > I know that we are trying to do the right thing. But right now there is an > error only in ginStepRight. Maybe now the fix this place, and we will think > about "bool" then? The patch is attached (small and simple). > > Thanks. > > > -- > Yury Zhuravlev > Postgres Professional: http://www.postgrespro.com > The Russian Postgres Company > diff --git a/src/backend/access/gin/ginbtree.c > b/src/backend/access/gin/ginbtree.c > index 06ba9cb..30113d0 100644 > --- a/src/backend/access/gin/ginbtree.c > +++ b/src/backend/access/gin/ginbtree.c > @@ -162,8 +162,8 @@ ginStepRight(Buffer buffer, Relation index, int lockmode) > { > Buffer nextbuffer; > Pagepage = BufferGetPage(buffer); > - boolisLeaf = GinPageIsLeaf(page); > - boolisData = GinPageIsData(page); > + uint8 isLeaf = GinPageIsLeaf(page); > + uint8 isData = GinPageIsData(page); > BlockNumber blkno = GinPageGetOpaque(page)->rightlink; > > nextbuffer = ReadBuffer(index, blkno); I've pushed the gin specific stuff (although I fixed the macros instead of the callsites) to all branches. I plan to commit the larger patch (which has grown since last posting it) after the minor releases; it's somewhat large and has a lot of conflicts. This way at least the confirmed issue is fixed in the next set of minor releases. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Fri, Mar 18, 2016 at 8:36 PM, Yury Zhuravlevwrote: > Robert Haas wrote: >> >> On Wed, Mar 2, 2016 at 9:48 PM, Peter Eisentraut wrote: >>> >>> On 2/11/16 9:30 PM, Michael Paquier wrote: ... >> >> >> We need to decide what to do about this. I disagree with Peter: I >> think that regardless of stdbool, what we've got right now is sloppy >> coding - bad style if nothing else. Furthermore, I think that while C >> lets you use any non-zero value to represent true, our bool type is >> supposed to contain only one of those two values. Therefore, I think >> we should commit the full patch, back-patch it as far as somebody has >> the energy for, and move on. But regardless, this patch can't keep >> sitting in the CommitFest - we either have to take it or reject it, >> and soon. >> > > I know that we are trying to do the right thing. But right now there is an > error only in ginStepRight. Maybe now the fix this place, and we will think > about "bool" then? The patch is attached (small and simple). FWIW, when compiling with MS 2015 using the set of perl scripts I am not seeing this compilation error... We may want to understand first what kind of dependency is involved when doing the cmake build compared to what is done with src/tools/msvc. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Sat, Mar 19, 2016 at 12:08 AM, Yury Zhuravlevwrote: > Michael Paquier wrote: >> >> FWIW, when compiling with MS 2015 using the set of perl scripts I am >> not seeing this compilation error... > > This error not in build stage but in GIN regresion tests. CMake nothing to > do with. Bingo: "right sibling of GIN page is of different type", and gin is not the only test to fail, there are 4 tests failing including gin, and all the failures are caused by that. So yes, we had better fix it. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Thu, Mar 10, 2016 at 11:00 PM, Andres Freundwrote: > On 2016-03-11 04:50:45 +0100, Michael Paquier wrote: >> On Thu, Mar 10, 2016 at 11:40 PM, Robert Haas wrote: >> > We need to decide what to do about this. I disagree with Peter: I >> > think that regardless of stdbool, what we've got right now is sloppy >> > coding - bad style if nothing else. Furthermore, I think that while C >> > lets you use any non-zero value to represent true, our bool type is >> > supposed to contain only one of those two values. Therefore, I think >> > we should commit the full patch, back-patch it as far as somebody has >> > the energy for, and move on. But regardless, this patch can't keep >> > sitting in the CommitFest - we either have to take it or reject it, >> > and soon. > > I plan to commit something like this, unless there's very loud protest > from Peter's side. So, can we get on with that, then? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Robert Haas wrote: On Wed, Mar 2, 2016 at 9:48 PM, Peter Eisentrautwrote: On 2/11/16 9:30 PM, Michael Paquier wrote: ... We need to decide what to do about this. I disagree with Peter: I think that regardless of stdbool, what we've got right now is sloppy coding - bad style if nothing else. Furthermore, I think that while C lets you use any non-zero value to represent true, our bool type is supposed to contain only one of those two values. Therefore, I think we should commit the full patch, back-patch it as far as somebody has the energy for, and move on. But regardless, this patch can't keep sitting in the CommitFest - we either have to take it or reject it, and soon. I know that we are trying to do the right thing. But right now there is an error only in ginStepRight. Maybe now the fix this place, and we will think about "bool" then? The patch is attached (small and simple). Thanks. -- Yury Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Companydiff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c index 06ba9cb..30113d0 100644 --- a/src/backend/access/gin/ginbtree.c +++ b/src/backend/access/gin/ginbtree.c @@ -162,8 +162,8 @@ ginStepRight(Buffer buffer, Relation index, int lockmode) { Buffer nextbuffer; Pagepage = BufferGetPage(buffer); - boolisLeaf = GinPageIsLeaf(page); - boolisData = GinPageIsData(page); + uint8 isLeaf = GinPageIsLeaf(page); + uint8 isData = GinPageIsData(page); BlockNumber blkno = GinPageGetOpaque(page)->rightlink; nextbuffer = ReadBuffer(index, blkno); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Michael Paquierwrites: > FWIW, when compiling with MS 2015 using the set of perl scripts I am > not seeing this compilation error... We may want to understand first > what kind of dependency is involved when doing the cmake build > compared to what is done with src/tools/msvc. That is strange ... unless maybe cmake is supplying a different set of warning-enabling switches to the compiler? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Michael Paquier wrote: FWIW, when compiling with MS 2015 using the set of perl scripts I am not seeing this compilation error... This error not in build stage but in GIN regresion tests. CMake nothing to do with. -- Yury Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Andres Freund wrote: I plan to commit something like this, unless there's very loud protest from Peter's side. I agree. Peter proposal can be considered in a separate thread. Thanks. -- Yury Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2016-03-11 04:50:45 +0100, Michael Paquier wrote: > On Thu, Mar 10, 2016 at 11:40 PM, Robert Haaswrote: > > We need to decide what to do about this. I disagree with Peter: I > > think that regardless of stdbool, what we've got right now is sloppy > > coding - bad style if nothing else. Furthermore, I think that while C > > lets you use any non-zero value to represent true, our bool type is > > supposed to contain only one of those two values. Therefore, I think > > we should commit the full patch, back-patch it as far as somebody has > > the energy for, and move on. But regardless, this patch can't keep > > sitting in the CommitFest - we either have to take it or reject it, > > and soon. I plan to commit something like this, unless there's very loud protest from Peter's side. > +1, I would suggest to move ahead, !! is not really Postgres-like anyway. The !! bit is a minor sideshow to this, afaics. It just came up when discussing the specifics of the fixed macros and some people expressed a clear preference for not using !!, so I fixed the occurrances I introduced. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2016-03-02 21:48:16 -0500, Peter Eisentraut wrote: > After reviewing this thread and relevant internet lore, I think this > might be the wrong way to address this problem. It is in general not > guaranteed in C that a Boolean-sounding function or macro returns 0 or > 1. Prime examples are ctype.h functions such as isupper(). This is > normally not a problem because built-in conditionals such as if, &&, || > handle this just fine. So code like > > - Assert(!create || !!txn); > + Assert(!create || txn != NULL); > > is arguably silly either way. There is no risk in writing just > > Assert(!create || txn); Yes, obviously. I doubt that anyone misunderstood that. I personally find the !! easier to read when contrasting to a negated value (as in the above assert). > The problem only happens if you compare two "Boolean" values directly > with each other; That's not correct. It also happen if you compare an expression with a stored version, i.e. only one side is a 'proper bool'. > A quick look through the code based on the provided patch shows that > approximately the only place affected by this is > > if (isLeaf != GinPageIsLeaf(page) || isData != GinPageIsData(page)) > elog(ERROR, "right sibling of GIN page is of different type"); > > and that's not actually a problem because isLeaf and isData are earlier > populated by the same macros. It would still be worth fixing, but a > localized fix seems better. That's maybe the only place where we compare stored booleans to expressions, but it's definitely not the only place where some expression is stored in a boolean variable. And in all those cases there's an int32 (or whatever type the expression has) to bool (usually 1byte char). That's definitely problematic. > But the lore on the internet casts some doubt on that: There is no > guarantee that bool is 1 byte, that bool can be passed around like char, > or even that bool arrays are laid out like char arrays. Maybe this all > works out okay, just like it has worked out so far that int is 4 bytes, > but we don't know enough about it. We could probably add some configure > tests around that. So? > My proposal on this particular patch is to do nothing. The stdbool > issues should be looked into, for the sake of Windows and other > future-proofness. But that will likely be an entirely different patch. That seems to entirely miss the part of this discussion dealing with high bits set and such? Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Thu, Mar 10, 2016 at 11:40 PM, Robert Haaswrote: > We need to decide what to do about this. I disagree with Peter: I > think that regardless of stdbool, what we've got right now is sloppy > coding - bad style if nothing else. Furthermore, I think that while C > lets you use any non-zero value to represent true, our bool type is > supposed to contain only one of those two values. Therefore, I think > we should commit the full patch, back-patch it as far as somebody has > the energy for, and move on. But regardless, this patch can't keep > sitting in the CommitFest - we either have to take it or reject it, > and soon. +1, I would suggest to move ahead, !! is not really Postgres-like anyway. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Wed, Mar 2, 2016 at 9:48 PM, Peter Eisentrautwrote: > On 2/11/16 9:30 PM, Michael Paquier wrote: >>> Well, Yury was saying upthread that some MSVC versions have a problem >>> with the existing coding, which would be a reason to back-patch ... >>> but I'd like to see a failing buildfarm member first. Don't particularly >>> want to promise to support compilers not represented in the farm. >> >> Grmbl. Forgot to attach the rebased patch upthread. Here is it now. >> >> As of now the only complain has been related to MS2015 and MS2013. If >> we follow the pattern of cec8394b and [1], support to compile on newer >> versions of MSVC would be master and REL9_5_STABLE, but MS2013 is >> supported down to 9.3. Based on this reason, we would want to >> backpatch down to 9.3 the patch of this thread. > > After reviewing this thread and relevant internet lore, I think this > might be the wrong way to address this problem. It is in general not > guaranteed in C that a Boolean-sounding function or macro returns 0 or > 1. Prime examples are ctype.h functions such as isupper(). This is > normally not a problem because built-in conditionals such as if, &&, || > handle this just fine. So code like > > - Assert(!create || !!txn); > + Assert(!create || txn != NULL); > > is arguably silly either way. There is no risk in writing just > > Assert(!create || txn); > > The problem only happens if you compare two "Boolean" values directly > with each other; and so maybe you shouldn't do that, or at least place > the extra care there instead, instead of fighting a permanent battle > with the APIs you're using. (This isn't an outrageous requirement: You > can't compare floats or strings either without extra care.) > > A quick look through the code based on the provided patch shows that > approximately the only place affected by this is > > if (isLeaf != GinPageIsLeaf(page) || isData != GinPageIsData(page)) > elog(ERROR, "right sibling of GIN page is of different type"); > > and that's not actually a problem because isLeaf and isData are earlier > populated by the same macros. It would still be worth fixing, but a > localized fix seems better. > > > Now on the matter of stdbool, I tried putting an #include > near the top of c.h and compile that to see what would happen. This is > the first warning I see: > > ginlogic.c: In function 'shimTriConsistentFn': > ginlogic.c:171:24: error: comparison of constant '2' with boolean > expression is always false [-Werror=bool-compare] >if (key->entryRes[i] == GIN_MAYBE) > ^ > > and then later on something related: > > ../../../../src/include/tsearch/ts_utils.h:107:13: note: expected '_Bool > (*)(void *, QueryOperand *) {aka _Bool (*)(void *, struct > *)}' but argument is of type 'GinTernaryValue (*)(void *, QueryOperand > *) {aka char (*)(void *, struct *)}' > > So the compiler is actually potentially helpful, but as it stands, > PostgreSQL code is liable to break if you end up with stdbool.h somehow. > > (plperl also fails to compile because of a hot-potato game about who is > actually responsible for defining bool.) > > So one idea would be to actually get ahead of the game, include > stdbool.h if available, fix the mentioned issues, and maybe get more > robust code that way. > > But the lore on the internet casts some doubt on that: There is no > guarantee that bool is 1 byte, that bool can be passed around like char, > or even that bool arrays are laid out like char arrays. Maybe this all > works out okay, just like it has worked out so far that int is 4 bytes, > but we don't know enough about it. We could probably add some configure > tests around that. > > We could also go the other way and forcibly undefine an existing bool > type (since stdbool.h is supposed to use macros, not typedefs). But > that might not work well if a header that is included later pulls in > stdbool.h on top of that. > > > My proposal on this particular patch is to do nothing. The stdbool > issues should be looked into, for the sake of Windows and other > future-proofness. But that will likely be an entirely different patch. We need to decide what to do about this. I disagree with Peter: I think that regardless of stdbool, what we've got right now is sloppy coding - bad style if nothing else. Furthermore, I think that while C lets you use any non-zero value to represent true, our bool type is supposed to contain only one of those two values. Therefore, I think we should commit the full patch, back-patch it as far as somebody has the energy for, and move on. But regardless, this patch can't keep sitting in the CommitFest - we either have to take it or reject it, and soon. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription:
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2/11/16 9:30 PM, Michael Paquier wrote: >> Well, Yury was saying upthread that some MSVC versions have a problem >> with the existing coding, which would be a reason to back-patch ... >> but I'd like to see a failing buildfarm member first. Don't particularly >> want to promise to support compilers not represented in the farm. > > Grmbl. Forgot to attach the rebased patch upthread. Here is it now. > > As of now the only complain has been related to MS2015 and MS2013. If > we follow the pattern of cec8394b and [1], support to compile on newer > versions of MSVC would be master and REL9_5_STABLE, but MS2013 is > supported down to 9.3. Based on this reason, we would want to > backpatch down to 9.3 the patch of this thread. After reviewing this thread and relevant internet lore, I think this might be the wrong way to address this problem. It is in general not guaranteed in C that a Boolean-sounding function or macro returns 0 or 1. Prime examples are ctype.h functions such as isupper(). This is normally not a problem because built-in conditionals such as if, &&, || handle this just fine. So code like - Assert(!create || !!txn); + Assert(!create || txn != NULL); is arguably silly either way. There is no risk in writing just Assert(!create || txn); The problem only happens if you compare two "Boolean" values directly with each other; and so maybe you shouldn't do that, or at least place the extra care there instead, instead of fighting a permanent battle with the APIs you're using. (This isn't an outrageous requirement: You can't compare floats or strings either without extra care.) A quick look through the code based on the provided patch shows that approximately the only place affected by this is if (isLeaf != GinPageIsLeaf(page) || isData != GinPageIsData(page)) elog(ERROR, "right sibling of GIN page is of different type"); and that's not actually a problem because isLeaf and isData are earlier populated by the same macros. It would still be worth fixing, but a localized fix seems better. Now on the matter of stdbool, I tried putting an #include near the top of c.h and compile that to see what would happen. This is the first warning I see: ginlogic.c: In function 'shimTriConsistentFn': ginlogic.c:171:24: error: comparison of constant '2' with boolean expression is always false [-Werror=bool-compare] if (key->entryRes[i] == GIN_MAYBE) ^ and then later on something related: ../../../../src/include/tsearch/ts_utils.h:107:13: note: expected '_Bool (*)(void *, QueryOperand *) {aka _Bool (*)(void *, struct *)}' but argument is of type 'GinTernaryValue (*)(void *, QueryOperand *) {aka char (*)(void *, struct *)}' So the compiler is actually potentially helpful, but as it stands, PostgreSQL code is liable to break if you end up with stdbool.h somehow. (plperl also fails to compile because of a hot-potato game about who is actually responsible for defining bool.) So one idea would be to actually get ahead of the game, include stdbool.h if available, fix the mentioned issues, and maybe get more robust code that way. But the lore on the internet casts some doubt on that: There is no guarantee that bool is 1 byte, that bool can be passed around like char, or even that bool arrays are laid out like char arrays. Maybe this all works out okay, just like it has worked out so far that int is 4 bytes, but we don't know enough about it. We could probably add some configure tests around that. We could also go the other way and forcibly undefine an existing bool type (since stdbool.h is supposed to use macros, not typedefs). But that might not work well if a header that is included later pulls in stdbool.h on top of that. My proposal on this particular patch is to do nothing. The stdbool issues should be looked into, for the sake of Windows and other future-proofness. But that will likely be an entirely different patch. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Sat, Feb 13, 2016 at 1:48 AM, Tom Lanewrote: > Andres Freund writes: >> On February 12, 2016 5:29:44 PM GMT+01:00, Tom Lane >> wrote: >>> We should standardize on the "((var & FLAG) != 0)" >>> pattern, which works reliably in all cases. > >> That's what the second version of my patch, and I presume Michael's updated >> one as well, does. I think the only open question is how far to backpatch. >> While annoying work, I think we should go all the way. > > I don't object to that, if someone wants to do the work. A good argument > for it is that we'd otherwise be laying a nasty trap for future > back-patched bug fixes, which might well rely on the cleaned-up behavior. >From the MSVC-only perspective, that's down to 9.3, but it would definitely make sense to backpatch 2 versions further down to facilitate future bug fix integration, so +1 to get that down to 9.1. Andres, I guess that you are on that? That's your patch after all. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Yury Zhuravlev wrote: Robert Haas wrote: So, is it being pulled in indirectly by some other #include? I can double-check it tomorrow. I've found who include stdbool.h and think it is inevitable for MSVC2013 and MSVC2015. In port/win32.h we inlcude process.h. In process.h included corecrt_startup.h. In corecrt_startup.h included vcruntime_startup.h. In vcruntime_startup.h we included stdbool.h tadam! -- Yury Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Thu, Feb 11, 2016 at 9:30 PM, Michael Paquierwrote: > On Fri, Feb 12, 2016 at 3:45 AM, Tom Lane wrote: >> Andres Freund writes: >>> On 2016-02-11 13:37:17 -0500, Tom Lane wrote: Absolutely; they don't work safely for testing bits that aren't in the rightmost byte of a flag word, for instance. I'm on board with making these fixes, I'm just unconvinced that stdbool is a good reason for it. >> >>> Oh, ok. Interactions with stdbool was what made me looking into this, >>> that's primarily why I mentioned it. What's your thinking about >>> back-patching, independent of that then? >> >> Well, Yury was saying upthread that some MSVC versions have a problem >> with the existing coding, which would be a reason to back-patch ... >> but I'd like to see a failing buildfarm member first. Don't particularly >> want to promise to support compilers not represented in the farm. > > Grmbl. Forgot to attach the rebased patch upthread. Here is it now. > > As of now the only complain has been related to MS2015 and MS2013. If > we follow the pattern of cec8394b and [1], support to compile on newer > versions of MSVC would be master and REL9_5_STABLE, but MS2013 is > supported down to 9.3. Based on this reason, we would want to > backpatch down to 9.3 the patch of this thread. OK, that seems reasonable from here. What I'm still fuzzy about is why including stdbool.h causes a failure. Is it because it defines a type called "bool" that clashes with ours? That seems like the obvious explanation, but then how does that not cause the compiler to just straight-up error out? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Fri, Feb 12, 2016 at 9:39 AM, Tom Lanewrote: > Robert Haas writes: >> On Fri, Feb 12, 2016 at 8:48 AM, Andres Freund wrote: >>> E.g. if you include stdbool.h [ ginStepRight breaks ] > >> Ah-ha. OK, now I get it. So then I agree we should back-patch this >> at least as far as 9.3 where MSVC 2013 became a supported platform, > > Um, no, that does not follow. The unanswered question here is why, > when we *have not* included stdbool.h and *have* typedef'd bool as > just plain "char", we would get C99 bool behavior. There is something > happening there that should not be happening, and I'm not really satisfied > with the explanation "Microsoft is brain-dead as usual". I think we > should dig deeper, because whatever is going on there may have deeper > effects than we now realize. http://www.postgresql.org/message-id/d2106c2d-0f46-4cf9-af27-54f81ef6e...@postgrespro.ru seems to explain what happens pretty clearly. We #include something which #includes something which #includes something which #includes . It's not that surprising, is it? I mean, things with "std" in the name figure to be commonly-included. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
> OK, that seems reasonable from here. What I'm still fuzzy about is > why including stdbool.h causes a failure. Is it because it defines a > type called "bool" that clashes with ours? That seems like the > obvious explanation, but then how does that not cause the compiler to > just straight-up error out? stdbool.h defines the '_Bool' type. The standard says: C99 and C11 §6.3.1.2/1 “When any scalar value is converted to _Bool, the result is 0 if the value compares equal to 0; otherwise, the result is 1.” It seems that MSVC's bool (_Bool) assignment complies to the standard. -- Dmitry Ivanov Postgres Professional: http://www.postgrespro.com Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2016-02-12 07:59:06 -0500, Robert Haas wrote: > OK, that seems reasonable from here. What I'm still fuzzy about is > why including stdbool.h causes a failure. Is it because it defines a > type called "bool" that clashes with ours? That seems like the > obvious explanation, but then how does that not cause the compiler to > just straight-up error out? No, that's not the problem. Our own definition is actually conditional. The problem is that the standard's booleans behave differently. They only ever contain 0 or 1, even if you assign something outside of that range - essentially they store !!(right-hand-side). If you know compare a boolean with the values returned by one of these macros you'll get into problems. E.g. if you include stdbool.h: Buffer ginStepRight(Buffer buffer, Relation index, int lockmode) { boolisLeaf = GinPageIsLeaf(page); boolisData = GinPageIsData(page); ... if (isLeaf != GinPageIsLeaf(page) || isData != GinPageIsData(page)) elog(ERROR, "right sibling of GIN page is of different type"); doesn't work correctly because isLeaf/isData contain only 0/1 (due to the standard's bool behaviour), but the values they're compared to returns some bit set. Due to integer promotion rules isLeaf is promoted to an integer and compared... And thus the tests fail. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Robert Haaswrites: > On Fri, Feb 12, 2016 at 8:48 AM, Andres Freund wrote: >> E.g. if you include stdbool.h [ ginStepRight breaks ] > Ah-ha. OK, now I get it. So then I agree we should back-patch this > at least as far as 9.3 where MSVC 2013 became a supported platform, Um, no, that does not follow. The unanswered question here is why, when we *have not* included stdbool.h and *have* typedef'd bool as just plain "char", we would get C99 bool behavior. There is something happening there that should not be happening, and I'm not really satisfied with the explanation "Microsoft is brain-dead as usual". I think we should dig deeper, because whatever is going on there may have deeper effects than we now realize. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Fri, Feb 12, 2016 at 8:48 AM, Andres Freundwrote: > On 2016-02-12 07:59:06 -0500, Robert Haas wrote: >> OK, that seems reasonable from here. What I'm still fuzzy about is >> why including stdbool.h causes a failure. Is it because it defines a >> type called "bool" that clashes with ours? That seems like the >> obvious explanation, but then how does that not cause the compiler to >> just straight-up error out? > > No, that's not the problem. Our own definition is actually > conditional. The problem is that the standard's booleans behave > differently. They only ever contain 0 or 1, even if you assign something > outside of that range - essentially they store !!(right-hand-side). If > you know compare a boolean with the values returned by one of these > macros you'll get into problems. > > E.g. if you include stdbool.h: > Buffer > ginStepRight(Buffer buffer, Relation index, int lockmode) > { > boolisLeaf = GinPageIsLeaf(page); > boolisData = GinPageIsData(page); > ... > if (isLeaf != GinPageIsLeaf(page) || isData != GinPageIsData(page)) > elog(ERROR, "right sibling of GIN page is of different type"); > > doesn't work correctly because isLeaf/isData contain only 0/1 (due to > the standard's bool behaviour), but the values they're compared to > returns some bit set. Due to integer promotion rules isLeaf is promoted > to an integer and compared... And thus the tests fail. Ah-ha. OK, now I get it. So then I agree we should back-patch this at least as far as 9.3 where MSVC 2013 became a supported platform, per Michael's remarks, and maybe all the way. Do you want to do that? If not, I'll do it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2016-02-12 09:39:20 -0500, Tom Lane wrote: > Robert Haaswrites: > > On Fri, Feb 12, 2016 at 8:48 AM, Andres Freund wrote: > >> E.g. if you include stdbool.h [ ginStepRight breaks ] > > > Ah-ha. OK, now I get it. So then I agree we should back-patch this > > at least as far as 9.3 where MSVC 2013 became a supported platform, > > Um, no, that does not follow. Well, these headers are generally buggy, so ... > The unanswered question here is why, > when we *have not* included stdbool.h and *have* typedef'd bool as > just plain "char", we would get C99 bool behavior. There is something > happening there that should not be happening, and I'm not really satisfied > with the explanation "Microsoft is brain-dead as usual". I think we > should dig deeper, because whatever is going on there may have deeper > effects than we now realize. Well, http://archives.postgresql.org/message-id/d2106c2d-0f46-4cf9-af27-54f81ef6e20c%40postgrespro.ru outlines how stdbool.h gets included. That happens fairly at the begining of c.h. Later our definitions are guarded by ifdefs: #ifndef bool typedef char bool; #endif #ifndef true #define true((bool) 1) #endif #ifndef false #define false ((bool) 0) #endif So we can lament that MS standard libraries include stdbool.h instead of using _Bool. But I doubt that's going to buy us super much. Btw, there's a distinct advantage including stdbool: Compilers actually generate a fair bit better error messages/warnings in some cases. And the generated code sometimes is a bit better, too. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Robert Haaswrites: > On Fri, Feb 12, 2016 at 9:39 AM, Tom Lane wrote: >> Um, no, that does not follow. The unanswered question here is why, >> when we *have not* included stdbool.h and *have* typedef'd bool as >> just plain "char", we would get C99 bool behavior. > http://www.postgresql.org/message-id/d2106c2d-0f46-4cf9-af27-54f81ef6e...@postgrespro.ru > seems to explain what happens pretty clearly. We #include something > which #includes something which #includes something which #includes > . It's not that surprising, is it? Well, the thing that is scaring me here is allowing a platform-specific definition of "bool" to be adopted. If, for example, the compiler writer decided that that should be int width rather than char width, all hell would break loose. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Andres Freundwrites: > On February 12, 2016 5:29:44 PM GMT+01:00, Tom Lane > wrote: >> We should standardize on the "((var & FLAG) != 0)" >> pattern, which works reliably in all cases. > That's what the second version of my patch, and I presume Michael's updated > one as well, does. I think the only open question is how far to backpatch. > While annoying work, I think we should go all the way. I don't object to that, if someone wants to do the work. A good argument for it is that we'd otherwise be laying a nasty trap for future back-patched bug fixes, which might well rely on the cleaned-up behavior. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Andres Freund wrote: Did you read what I wrote? Read. That's not correct for char booleans, because the can have different bits set. Current code support this behavior. But to shoot his leg becomes easier. != 0 much better of course. Thanks. -- Yury Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Andres Freund wrote: Unless I am missing something major, that doesn't seem to achieve all that much. A cast to a char based bool wouldn't normalize this to 0 or 1. So you're still not guaranteed to be able to do somebool == anotherbool when either are set based on such a macro. In C99 cast to bool return 0 or 1 only. In older compilers nothing changes (Now the code is designed to "char == char"). I think this is a good option. But of course to write bool and use char strange. -- Yury Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On February 12, 2016 5:40:29 PM GMT+01:00, Yury Zhuravlevwrote: >Andres Freund wrote: >> Unless I am missing something major, that doesn't seem to >> achieve all that much. A cast to a char based bool wouldn't >> normalize this to 0 or 1. So you're still not guaranteed to be >> able to do somebool == anotherbool when either are set based on >> such a macro. >> > >In C99 cast to bool return 0 or 1 only. Don't you say. That's why I brought all this up. > In older compilers nothing >changes >(Now the code is designed to "char == char"). >I think this is a good option. But of course to write bool and use char > >strange. Did you read what I wrote? That's not correct for char booleans, because the can have different bits set. Andres --- Please excuse brevity and formatting - I am writing this on my mobile phone. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Fri, Feb 12, 2016 at 9:47 AM, Tom Lanewrote: > Robert Haas writes: >> On Fri, Feb 12, 2016 at 9:39 AM, Tom Lane wrote: >>> Um, no, that does not follow. The unanswered question here is why, >>> when we *have not* included stdbool.h and *have* typedef'd bool as >>> just plain "char", we would get C99 bool behavior. > >> http://www.postgresql.org/message-id/d2106c2d-0f46-4cf9-af27-54f81ef6e...@postgrespro.ru >> seems to explain what happens pretty clearly. We #include something >> which #includes something which #includes something which #includes >> . It's not that surprising, is it? > > Well, the thing that is scaring me here is allowing a platform-specific > definition of "bool" to be adopted. If, for example, the compiler > writer decided that that should be int width rather than char width, > all hell would break loose. That's true, but it doesn't really seem like a reason not to commit this patch. I mean, the coding here is (a) dangerous by your own admission and (b) actually breaks on platforms for which we allege support. If we find out that somebody has implemented an int-width bool we'll have some bigger decisions to make, but I don't see any particular reason why we've got to make those decisions now. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Teodor Sigaevwrites: > One more option for patch: > #define GinPageIsLeaf(page)((bool)(GinPageGetOpaque(page)->flags & > GIN_LEAF)) I think that's a seriously bad coding pattern to adopt, because it would work for some people but not others if the flag bit is to the left of the rightmost byte. We should standardize on the "((var & FLAG) != 0)" pattern, which works reliably in all cases. The pattern "(!!(var & FLAG))" would work too, but I dislike it because it is not "say what you mean" but more of a cute coding trick to save a keystroke or two. People who aren't longtime C coders would have to stop and think about what it does. (I'd expect reasonable compilers to generate pretty much the same code in any of these cases, so that aspect of it shouldn't be an issue.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On February 12, 2016 5:15:59 PM GMT+01:00, Teodor Sigaevwrote: >One more option for patch: > >#define GinPageIsLeaf(page)((bool)(GinPageGetOpaque(page)->flags & >GIN_LEAF)) > >Seems it will work on any platform with built-in bool. But I don't know >will it >work with 'typedef char bool' if high bit will be set. Unless I am missing something major, that doesn't seem to achieve all that much. A cast to a char based bool wouldn't normalize this to 0 or 1. So you're still not guaranteed to be able to do somebool == anotherbool when either are set based on such a macro. Andres --- Please excuse brevity and formatting - I am writing this on my mobile phone. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
One more option for patch: #define GinPageIsLeaf(page)((bool)(GinPageGetOpaque(page)->flags & GIN_LEAF)) Seems it will work on any platform with built-in bool. But I don't know will it work with 'typedef char bool' if high bit will be set. That's true, but it doesn't really seem like a reason not to commit this patch. I mean, the coding here is (a) dangerous by your own admission and (b) actually breaks on platforms for which we allege support. If we find out that somebody has implemented an int-width bool we'll have some bigger decisions to make, but I don't see any particular reason why we've got to make those decisions now. +1 -- Teodor Sigaev E-mail: teo...@sigaev.ru WWW: http://www.sigaev.ru/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2016-02-12 09:47:47 -0500, Tom Lane wrote: > Robert Haaswrites: > > On Fri, Feb 12, 2016 at 9:39 AM, Tom Lane wrote: > >> Um, no, that does not follow. The unanswered question here is why, > >> when we *have not* included stdbool.h and *have* typedef'd bool as > >> just plain "char", we would get C99 bool behavior. > > > http://www.postgresql.org/message-id/d2106c2d-0f46-4cf9-af27-54f81ef6e...@postgrespro.ru > > seems to explain what happens pretty clearly. We #include something > > which #includes something which #includes something which #includes > > . It's not that surprising, is it? > > Well, the thing that is scaring me here is allowing a platform-specific > definition of "bool" to be adopted. If, for example, the compiler > writer decided that that should be int width rather than char width, > all hell would break loose. Well, for some reason c.h has been written to allow for that for a long time. I think it's fairly unlikely that somebody writes a _Bool implementation where sizeof(_Bool) is bigger than sizeof(char). Although that'd be, by my reading of the standard. permissible. It just says 6.2.5-2: An object declared as type _Bool is large enough to store the values 0 and 1. 6.7.2.1: While the number of bits in a _Bool object is at least CHAR_BIT, the width (number of sign and value bits) of a _Bool may be just 1 bit. afaics that's pretty much all said about the size of _Bool, except some bitfield special cases. But we also only support e.g. CHAR_BIT = 8, so I'm not super concerned about _Bool being defined too wide. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On February 12, 2016 5:29:44 PM GMT+01:00, Tom Lanewrote: > We should standardize on the "((var & FLAG) != 0)" >pattern, which works reliably in all cases. That's what the second version of my patch, and I presume Michael's updated one as well, does. I think the only open question is how far to backpatch. While annoying work, I think we should go all the way. Andres --- Please excuse brevity and formatting - I am writing this on my mobile phone. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Thu, Feb 11, 2016 at 9:15 AM, Andres Freundwrote: > On 2016-02-11 08:50:41 -0500, Robert Haas wrote: >> Are we thinking to back-patch this? I would be disinclined to >> back-patch widespread changes like this. If there's a specific >> instance related to Gin where this is causing a tangible problem, we >> could back-patch just that part, with a clear description of that >> problem. Otherwise, I think this should be master-only. > > I'm not sure. It's pretty darn nasty that right now we fail in some > places in the code if stdbool.h is included previously. That's probably > going to become more and more common. On the other hand it's invasive, > as you say. Partially patching things doesn't seem like a really viable > approach to me, bugs caused by this are hard to find/trigger. I have never been quite clear on what you think is going to cause stdbool.h inclusions to become more common. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2016-02-11 09:51:26 -0500, Robert Haas wrote: > I have never been quite clear on what you think is going to cause > stdbool.h inclusions to become more common. Primarily because it's finally starting to be supported across the board, thanks to MS finally catching up. Then there's uglyness like: http://archives.postgresql.org/message-id/508bd3a0-b0a1-4f10-ad9c-2f86656f6e79%40postgrespro.ru -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Tue, Feb 9, 2016 at 11:53 PM, Michael Paquierwrote: > On Wed, Feb 10, 2016 at 1:17 AM, Yury Zhuravlev > wrote: >> I've just run into a problem with these macro. Function ginStepRight breaks >> completely when compiled using the MSVC2013 and MSVC2015 (since these >> releases use C99's bools but without stdbool.h like C++). >> I don't understand why the patch has not been commited yet. It seems to me >> not so important !! or ! = 0, the solution is all that matters. > > +1 for applying it. There were some conflicts in gist and lwlock, that > I fixed as per the attached. I have added as well an entry in next CF: > https://commitfest.postgresql.org/9/507/ > If a committer wants to pick up that before, feel free. For now it > won't fall in the void, that's better than nothing. Not actually attached. Are we thinking to back-patch this? I would be disinclined to back-patch widespread changes like this. If there's a specific instance related to Gin where this is causing a tangible problem, we could back-patch just that part, with a clear description of that problem. Otherwise, I think this should be master-only. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2016-02-11 08:50:41 -0500, Robert Haas wrote: > Are we thinking to back-patch this? I would be disinclined to > back-patch widespread changes like this. If there's a specific > instance related to Gin where this is causing a tangible problem, we > could back-patch just that part, with a clear description of that > problem. Otherwise, I think this should be master-only. I'm not sure. It's pretty darn nasty that right now we fail in some places in the code if stdbool.h is included previously. That's probably going to become more and more common. On the other hand it's invasive, as you say. Partially patching things doesn't seem like a really viable approach to me, bugs caused by this are hard to find/trigger. - Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Fri, Feb 12, 2016 at 3:45 AM, Tom Lanewrote: > Andres Freund writes: >> On 2016-02-11 13:37:17 -0500, Tom Lane wrote: >>> Absolutely; they don't work safely for testing bits that aren't in the >>> rightmost byte of a flag word, for instance. I'm on board with making >>> these fixes, I'm just unconvinced that stdbool is a good reason for it. > >> Oh, ok. Interactions with stdbool was what made me looking into this, >> that's primarily why I mentioned it. What's your thinking about >> back-patching, independent of that then? > > Well, Yury was saying upthread that some MSVC versions have a problem > with the existing coding, which would be a reason to back-patch ... > but I'd like to see a failing buildfarm member first. Don't particularly > want to promise to support compilers not represented in the farm. Grmbl. Forgot to attach the rebased patch upthread. Here is it now. As of now the only complain has been related to MS2015 and MS2013. If we follow the pattern of cec8394b and [1], support to compile on newer versions of MSVC would be master and REL9_5_STABLE, but MS2013 is supported down to 9.3. Based on this reason, we would want to backpatch down to 9.3 the patch of this thread. [1]: http://www.postgresql.org/message-id/529d05cc.7070...@gmx.de -- Michael diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index b0d5440..3bbb218 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -5329,7 +5329,8 @@ xact_redo_commit(xl_xact_parsed_commit *parsed, LWLockRelease(XidGenLock); } - Assert(!!(parsed->xinfo & XACT_XINFO_HAS_ORIGIN) == (origin_id != InvalidRepOriginId)); + Assert(((parsed->xinfo & XACT_XINFO_HAS_ORIGIN) != 0) == + (origin_id != InvalidRepOriginId)); if (parsed->xinfo & XACT_XINFO_HAS_ORIGIN) commit_time = parsed->origin_timestamp; diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index 78acced..024929a 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -575,7 +575,7 @@ ReorderBufferTXNByXid(ReorderBuffer *rb, TransactionId xid, bool create, if (is_new) *is_new = !found; - Assert(!create || !!txn); + Assert(!create || txn != NULL); return txn; } diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index d245857..ef42fad 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -205,11 +205,11 @@ PRINT_LWDEBUG(const char *where, LWLock *lock, LWLockMode mode) errmsg_internal("%d: %s(%s): excl %u shared %u haswaiters %u waiters %u rOK %d", MyProcPid, where, MainLWLockNames[id], - !!(state & LW_VAL_EXCLUSIVE), + (state & LW_VAL_EXCLUSIVE) != 0, state & LW_SHARED_MASK, - !!(state & LW_FLAG_HAS_WAITERS), + (state & LW_FLAG_HAS_WAITERS) != 0, pg_atomic_read_u32(>nwaiters), - !!(state & LW_FLAG_RELEASE_OK; + (state & LW_FLAG_RELEASE_OK) != 0))); else ereport(LOG, (errhidestmt(true), @@ -217,11 +217,11 @@ PRINT_LWDEBUG(const char *where, LWLock *lock, LWLockMode mode) errmsg_internal("%d: %s(%s %d): excl %u shared %u haswaiters %u waiters %u rOK %d", MyProcPid, where, T_NAME(lock), id, - !!(state & LW_VAL_EXCLUSIVE), + (state & LW_VAL_EXCLUSIVE) != 0, state & LW_SHARED_MASK, - !!(state & LW_FLAG_HAS_WAITERS), + (state & LW_FLAG_HAS_WAITERS) != 0, pg_atomic_read_u32(>nwaiters), - !!(state & LW_FLAG_RELEASE_OK; + (state & LW_FLAG_RELEASE_OK) != 0))); } } diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h index d2ea588..e212c9f 100644 --- a/src/include/access/gin_private.h +++ b/src/include/access/gin_private.h @@ -112,22 +112,22 @@ typedef struct GinMetaPageData */ #define GinPageGetOpaque(page) ( (GinPageOpaque) PageGetSpecialPointer(page) ) -#define GinPageIsLeaf(page)( GinPageGetOpaque(page)->flags & GIN_LEAF ) +#define GinPageIsLeaf(page)( (GinPageGetOpaque(page)->flags & GIN_LEAF) != 0 ) #define GinPageSetLeaf(page) ( GinPageGetOpaque(page)->flags |= GIN_LEAF ) #define GinPageSetNonLeaf(page)( GinPageGetOpaque(page)->flags &= ~GIN_LEAF ) -#define GinPageIsData(page)( GinPageGetOpaque(page)->flags & GIN_DATA ) +#define GinPageIsData(page)( (GinPageGetOpaque(page)->flags & GIN_DATA) != 0 ) #define GinPageSetData(page) ( GinPageGetOpaque(page)->flags |= GIN_DATA ) -#define GinPageIsList(page)( GinPageGetOpaque(page)->flags & GIN_LIST ) +#define GinPageIsList(page)( (GinPageGetOpaque(page)->flags & GIN_LIST) != 0 ) #define GinPageSetList(page) ( GinPageGetOpaque(page)->flags |= GIN_LIST ) -#define GinPageHasFullRow(page)( GinPageGetOpaque(page)->flags & GIN_LIST_FULLROW ) +#define GinPageHasFullRow(page)(
Re: [HACKERS] GinPageIs* don't actually return a boolean
Robert Haas wrote: So, is it being pulled in indirectly by some other #include? I can double-check it tomorrow. -- Yury Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Andres Freundwrites: > On 2016-02-11 09:51:26 -0500, Robert Haas wrote: >> I have never been quite clear on what you think is going to cause >> stdbool.h inclusions to become more common. > Primarily because it's finally starting to be supported across the > board, thanks to MS finally catching up. There are exactly 0 references to stdbool in our source tree. The only way it'd get pulled in is if some other system header does so. (Which seems possible, admittedly. I'm not sure whether the C standard allows or forbids such things.) It's worth worrying also about extensions that might want to include stdbool.h --- anything in our own headers that would conflict would be a problem. But I'm unconvinced that we need to make our .c files prepared for stdbool.h to be #included in them. By that argument, any random symbol in any system header in any platform on the planet is a hazard to us. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2016-02-11 13:06:14 -0500, Tom Lane wrote: > But I'm unconvinced that we need to make our .c files prepared for > stdbool.h to be #included in them. The fixes, besides two stylistic edits around !! use, are all in headers. The issue is that we return things meant to be used in a boolean context, that's not just 0/1 but some random set bit. Looking over the (outdated) patch attached to http://archives.postgresql.org/message-id/20150812161140.GD25424%40awork2.anarazel.de it's not completely outlandish that one wants to use one of these functions, but also something that uses stdbool.h. > By that argument, any random > symbol in any system header in any platform on the planet is a hazard > to us. Don't think that's really the same. C99's booleans are part of the standard, and have surprising behaviour that you can't get on the C level (they always contain 1/0 after assignment). That makes for more likely and more subtle bugs. And anyway, these macros are a potential issue even without stdbool.h style booleans. If you compare them using equality, you can get into trouble... Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Thu, Feb 11, 2016 at 9:54 AM, Andres Freundwrote: > On 2016-02-11 09:51:26 -0500, Robert Haas wrote: >> I have never been quite clear on what you think is going to cause >> stdbool.h inclusions to become more common. > > Primarily because it's finally starting to be supported across the > board, thanks to MS finally catching up. > > Then there's uglyness like: > http://archives.postgresql.org/message-id/508bd3a0-b0a1-4f10-ad9c-2f86656f6e79%40postgrespro.ru So, is it being pulled in indirectly by some other #include? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Andres Freundwrites: > And anyway, these macros are a potential issue even without stdbool.h > style booleans. Absolutely; they don't work safely for testing bits that aren't in the rightmost byte of a flag word, for instance. I'm on board with making these fixes, I'm just unconvinced that stdbool is a good reason for it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2016-02-11 13:37:17 -0500, Tom Lane wrote: > Andres Freundwrites: > > And anyway, these macros are a potential issue even without stdbool.h > > style booleans. > > Absolutely; they don't work safely for testing bits that aren't in the > rightmost byte of a flag word, for instance. I'm on board with making > these fixes, I'm just unconvinced that stdbool is a good reason for it. Oh, ok. Interactions with stdbool was what made me looking into this, that's primarily why I mentioned it. What's your thinking about back-patching, independent of that then? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Andres Freundwrites: > On 2016-02-11 13:37:17 -0500, Tom Lane wrote: >> Absolutely; they don't work safely for testing bits that aren't in the >> rightmost byte of a flag word, for instance. I'm on board with making >> these fixes, I'm just unconvinced that stdbool is a good reason for it. > Oh, ok. Interactions with stdbool was what made me looking into this, > that's primarily why I mentioned it. What's your thinking about > back-patching, independent of that then? Well, Yury was saying upthread that some MSVC versions have a problem with the existing coding, which would be a reason to back-patch ... but I'd like to see a failing buildfarm member first. Don't particularly want to promise to support compilers not represented in the farm. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On вторник, 29 сентября 2015 г. 19:02:59 MSK, Alvaro Herrera wrote: Andres Freund wrote: I went through all headers in src/include and checked for macros containing [^&]&[^&] and checked whether they have this hazard. Found a fair number. That patch also changes !! tests into != 0 style. I don't think you pushed this, did you? Hello hackers! I've just run into a problem with these macro. Function ginStepRight breaks completely when compiled using the MSVC2013 and MSVC2015 (since these releases use C99's bools but without stdbool.h like C++). I don't understand why the patch has not been commited yet. It seems to me not so important !! or ! = 0, the solution is all that matters. Thanks! -- Yury Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Wed, Feb 10, 2016 at 1:17 AM, Yury Zhuravlevwrote: > I've just run into a problem with these macro. Function ginStepRight breaks > completely when compiled using the MSVC2013 and MSVC2015 (since these > releases use C99's bools but without stdbool.h like C++). > I don't understand why the patch has not been commited yet. It seems to me > not so important !! or ! = 0, the solution is all that matters. +1 for applying it. There were some conflicts in gist and lwlock, that I fixed as per the attached. I have added as well an entry in next CF: https://commitfest.postgresql.org/9/507/ If a committer wants to pick up that before, feel free. For now it won't fall in the void, that's better than nothing. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Andres Freund wrote: > I went through all headers in src/include and checked for macros > containing [^&]&[^&] and checked whether they have this hazard. Found a > fair number. > > That patch also changes !! tests into != 0 style. I don't think you pushed this, did you? -- Álvaro Herrerahttp://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
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2015-08-12 18:52:59 -0400, Tom Lane wrote: Andres Freund and...@anarazel.de writes: I went through all headers in src/include and checked for macros containing [^][^] and checked whether they have this hazard. Found a fair number. That patch also changes !! tests into != 0 style. Looks OK to me, except I wonder why you did this #define TRIGGER_FIRED_FOR_ROW(event) \ - ((event) TRIGGER_EVENT_ROW) + (((event) TRIGGER_EVENT_ROW) == TRIGGER_EVENT_ROW) rather than != 0. That way doesn't look either more efficient or more readable. Purely consistency with the surrounding code. I was on the fence about that one... -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Andres Freund and...@anarazel.de writes: On 2015-08-12 18:52:59 -0400, Tom Lane wrote: Looks OK to me, except I wonder why you did this #define TRIGGER_FIRED_FOR_ROW(event) \ -((event) TRIGGER_EVENT_ROW) +(((event) TRIGGER_EVENT_ROW) == TRIGGER_EVENT_ROW) rather than != 0. That way doesn't look either more efficient or more readable. Purely consistency with the surrounding code. I was on the fence about that one... The adjacent code is doing something different than a bit-test, though: it's checking whether multibit fields have particular values. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2015-08-12 19:03:50 -0400, Tom Lane wrote: The adjacent code is doing something different than a bit-test, though: it's checking whether multibit fields have particular values. Yea, I know, that's why I was on the fence about it. Since you have an opinion and I couldn't really decide it's pretty clear which direction to go ;) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Andres Freund and...@anarazel.de writes: I went through all headers in src/include and checked for macros containing [^][^] and checked whether they have this hazard. Found a fair number. That patch also changes !! tests into != 0 style. Looks OK to me, except I wonder why you did this #define TRIGGER_FIRED_FOR_ROW(event) \ - ((event) TRIGGER_EVENT_ROW) + (((event) TRIGGER_EVENT_ROW) == TRIGGER_EVENT_ROW) rather than != 0. That way doesn't look either more efficient or more readable. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2015-08-11 13:49:19 -0300, Alvaro Herrera wrote: Andres Freund wrote: On 2015-08-11 12:43:03 -0400, Robert Haas wrote: On Tue, Aug 11, 2015 at 12:40 PM, Tom Lane t...@sss.pgh.pa.us wrote: We do not use !! elsewhere for this purpose, and I for one find it a pretty ugly locution. We do, actually, now, in contrib/pg_xlogdump/pg_xlogdump.c. I'd be in favor of getting rid of those. Those got already removed by Heikki's WAL format changes... And a bunch more places actually. Blame me. I'll come up with a patch fixing the macros and converting existing !! style ones. Actually there's one that's not yours ... I'm not touching sepgsql... ;) I went through all headers in src/include and checked for macros containing [^][^] and checked whether they have this hazard. Found a fair number. That patch also changes !! tests into != 0 style. Greetings, Andres Freund From 2c67e83b945ff9183dd3b5cdeea78f4bde5b8a74 Mon Sep 17 00:00:00 2001 From: Andres Freund and...@anarazel.de Date: Wed, 12 Aug 2015 16:05:35 +0200 Subject: [PATCH] Return only 0/1 from boolean macros. Previously some test macros returned the result of bit operations. That obviously works without problems when used as part of a boolean expression - but if e.g. the result is assigned to a boolean variable the result in some cases can be less than pretty. Depending on the source of the type definition for bool the result might be too wide or, if going through a pointer, the result can be an invalid value for a boolean (stdbool.h's bool e.g. only allows 0 and 1 values). The latter e.g. causes test failures with gcc when using a stdbool.h type boolean. As discussed also change some existing macros that forced a 0/1 return value by using !! to != 0 comparisons. I tried to find all relevant test macros in headers, there very well might be additional ones in C files themselves. Discussion: 20150811154237.gd17...@awork2.anarazel.de --- src/backend/access/transam/xact.c | 3 ++- src/backend/replication/logical/reorderbuffer.c | 2 +- src/backend/storage/lmgr/lwlock.c | 6 +++--- src/include/access/gin_private.h| 16 src/include/access/gist.h | 8 src/include/access/htup_details.h | 8 src/include/access/itup.h | 4 ++-- src/include/access/nbtree.h | 14 +++--- src/include/access/spgist_private.h | 10 +- src/include/access/xact.h | 4 ++-- src/include/catalog/pg_trigger.h| 10 +- src/include/commands/trigger.h | 2 +- src/include/regex/regguts.h | 2 +- src/include/storage/bufpage.h | 6 +++--- src/include/utils/jsonb.h | 6 +++--- 15 files changed, 51 insertions(+), 50 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index b53d95f..f5ec79f 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -5310,7 +5310,8 @@ xact_redo_commit(xl_xact_parsed_commit *parsed, LWLockRelease(XidGenLock); } - Assert(!!(parsed-xinfo XACT_XINFO_HAS_ORIGIN) == (origin_id != InvalidRepOriginId)); + Assert(((parsed-xinfo XACT_XINFO_HAS_ORIGIN) != 0) == + (origin_id != InvalidRepOriginId)); if (parsed-xinfo XACT_XINFO_HAS_ORIGIN) commit_time = parsed-origin_timestamp; diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index cdc7bd7..6664baa 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -575,7 +575,7 @@ ReorderBufferTXNByXid(ReorderBuffer *rb, TransactionId xid, bool create, if (is_new) *is_new = !found; - Assert(!create || !!txn); + Assert(!create || txn != NULL); return txn; } diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index 687ed63..99a1d3c 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -190,11 +190,11 @@ PRINT_LWDEBUG(const char *where, LWLock *lock, LWLockMode mode) errmsg(%d: %s(%s %d): excl %u shared %u haswaiters %u waiters %u rOK %d, MyProcPid, where, T_NAME(lock), T_ID(lock), - !!(state LW_VAL_EXCLUSIVE), + (state LW_VAL_EXCLUSIVE) != 0, state LW_SHARED_MASK, - !!(state LW_FLAG_HAS_WAITERS), + (state LW_FLAG_HAS_WAITERS) != 0, pg_atomic_read_u32(lock-nwaiters), - !!(state LW_FLAG_RELEASE_OK; + (state LW_FLAG_RELEASE_OK) != 0))); } } diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h index 5095fc1..1b63ead 100644 --- a/src/include/access/gin_private.h +++ b/src/include/access/gin_private.h @@ -112,22 +112,22 @@ typedef struct GinMetaPageData */ #define GinPageGetOpaque(page) (
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Tue, Aug 11, 2015 at 12:40 PM, Tom Lane t...@sss.pgh.pa.us wrote: We do not use !! elsewhere for this purpose, and I for one find it a pretty ugly locution. We do, actually, now, in contrib/pg_xlogdump/pg_xlogdump.c. I'd be in favor of getting rid of those. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Andres Freund wrote: On 2015-08-11 12:43:03 -0400, Robert Haas wrote: On Tue, Aug 11, 2015 at 12:40 PM, Tom Lane t...@sss.pgh.pa.us wrote: We do not use !! elsewhere for this purpose, and I for one find it a pretty ugly locution. We do, actually, now, in contrib/pg_xlogdump/pg_xlogdump.c. I'd be in favor of getting rid of those. And a bunch more places actually. Blame me. I'll come up with a patch fixing the macros and converting existing !! style ones. Actually there's one that's not yours ... alvin: master 0$ git grep '!!' -- \*.c | grep -v '!!!' contrib/isn/isn.c: * It's a helper function, not intended to be used!! contrib/sepgsql/uavc.c: sepgsql_audit_log(!!denied, src/backend/access/gist/gist.c: /* yes!!, found */ src/backend/access/gist/gist.c: * awful!!, we need search tree to find parent ... , but before we src/backend/access/gist/gistbuild.c:/* yes!!, found it */ src/backend/access/transam/xact.c: Assert(!!(parsed-xinfo XACT_XINFO_HAS_ORIGIN) == (origin_id != InvalidRepOriginId)); src/backend/executor/nodeModifyTable.c: * ctid!! */ src/backend/replication/logical/reorderbuffer.c:Assert(!create || !!txn); src/backend/storage/lmgr/lwlock.c: !!(state LW_VAL_EXCLUSIVE), src/backend/storage/lmgr/lwlock.c: !!(state LW_FLAG_HAS_WAITERS), src/backend/storage/lmgr/lwlock.c: !!(state LW_FLAG_RELEASE_OK; src/backend/tsearch/wparser_def.c: * order must be the same as in typedef enum {} TParserState!! src/backend/utils/adt/like.c: * A big hack of the regexp.c code!! Contributed by src/bin/pg_ctl/pg_ctl.c: * manager checkpoint, it's got nothing to do with database checkpoints!! src/interfaces/ecpg/preproc/c_keywords.c: * !!WARNING!!: This list must be sorted, because binary src/interfaces/ecpg/preproc/ecpg_keywords.c: * !!WARNING!!: This list must be sorted, because binary src/pl/plpgsql/src/pl_scanner.c: * !!WARNING!!: These lists must be sorted by ASCII name, because binary -- Álvaro Herrerahttp://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
Re: [HACKERS] GinPageIs* don't actually return a boolean
On Tue, Aug 11, 2015 at 11:42 AM, Andres Freund and...@anarazel.de wrote: #define GinPageIsLeaf(page)( GinPageGetOpaque(page)-flags GIN_LEAF ) #define GinPageIsData(page)( GinPageGetOpaque(page)-flags GIN_DATA ) #define GinPageIsList(page)( GinPageGetOpaque(page)-flags GIN_LIST ) ... These macros don't actually return a boolean that's comparable with our true/false. That doesn't strike me as a good idea. If there's actually a boolean type defined by some included header (in which case we don't overwrite it in c.h!) this actually can lead to tests failing. If e.g. stdbool.h is included in c.h the tests fail with gcc-4.9. !! is unknown to our codebase except where you've added it, and personally, I hate that idiom. I think we should write (val) != 0 instead of !!val. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2015-08-11 12:04:48 -0400, Robert Haas wrote: On Tue, Aug 11, 2015 at 11:42 AM, Andres Freund and...@anarazel.de wrote: #define GinPageIsLeaf(page)( GinPageGetOpaque(page)-flags GIN_LEAF ) #define GinPageIsData(page)( GinPageGetOpaque(page)-flags GIN_DATA ) #define GinPageIsList(page)( GinPageGetOpaque(page)-flags GIN_LIST ) ... These macros don't actually return a boolean that's comparable with our true/false. That doesn't strike me as a good idea. If there's actually a boolean type defined by some included header (in which case we don't overwrite it in c.h!) this actually can lead to tests failing. If e.g. stdbool.h is included in c.h the tests fail with gcc-4.9. !! is unknown to our codebase except where you've added it, and personally, I hate that idiom. I think we should write (val) != 0 instead of !!val. Hm. I find !! slightly simpler and it's pretty widely used in other projects, but I don't care much. As long as we fix the underlying issue, which != 0 certainly does... -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2015-08-11 12:43:03 -0400, Robert Haas wrote: On Tue, Aug 11, 2015 at 12:40 PM, Tom Lane t...@sss.pgh.pa.us wrote: We do not use !! elsewhere for this purpose, and I for one find it a pretty ugly locution. We do, actually, now, in contrib/pg_xlogdump/pg_xlogdump.c. I'd be in favor of getting rid of those. And a bunch more places actually. Blame me. I'll come up with a patch fixing the macros and converting existing !! style ones. - Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
On 2015-08-11 17:42:37 +0200, Andres Freund wrote: Hi, #define GinPageIsLeaf(page)( GinPageGetOpaque(page)-flags GIN_LEAF ) #define GinPageIsData(page)( GinPageGetOpaque(page)-flags GIN_DATA ) #define GinPageIsList(page)( GinPageGetOpaque(page)-flags GIN_LIST ) ... These macros don't actually return a boolean that's comparable with our true/false. That doesn't strike me as a good idea. If there's actually a boolean type defined by some included header (in which case we don't overwrite it in c.h!) this actually can lead to tests failing. If e.g. stdbool.h is included in c.h the tests fail with gcc-4.9. I guess the reason here is that if it's a boolean type known to the compiler it's free to assume that it only contains true/false... I think we should add a !! to these macros to make sure it's an actual boolean. This has been the case since gin's initial commit in 8a3631f8d86cdd9b0 . Even worse, we have that kind of macro all over. I thought e.g. HeapTupleHeaderIs* would be safe agains that, but that turns out not be the case. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GinPageIs* don't actually return a boolean
Andres Freund and...@anarazel.de writes: #define GinPageIsLeaf(page)( GinPageGetOpaque(page)-flags GIN_LEAF ) #define GinPageIsData(page)( GinPageGetOpaque(page)-flags GIN_DATA ) #define GinPageIsList(page)( GinPageGetOpaque(page)-flags GIN_LIST ) These macros don't actually return a boolean that's comparable with our true/false. That doesn't strike me as a good idea. Agreed, this is risky. For example, if the bit being tested is to the left of the lowest byte of flags, storing the result into a bool variable would do the wrong thing. I think we should add a !! to these macros to make sure it's an actual boolean. Please write it more like #define GinPageIsLeaf(page) ((GinPageGetOpaque(page)-flags GIN_LEAF) != 0) We do not use !! elsewhere for this purpose, and I for one find it a pretty ugly locution. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] GinPageIs* don't actually return a boolean
Hi, #define GinPageIsLeaf(page)( GinPageGetOpaque(page)-flags GIN_LEAF ) #define GinPageIsData(page)( GinPageGetOpaque(page)-flags GIN_DATA ) #define GinPageIsList(page)( GinPageGetOpaque(page)-flags GIN_LIST ) ... These macros don't actually return a boolean that's comparable with our true/false. That doesn't strike me as a good idea. If there's actually a boolean type defined by some included header (in which case we don't overwrite it in c.h!) this actually can lead to tests failing. If e.g. stdbool.h is included in c.h the tests fail with gcc-4.9. I think we should add a !! to these macros to make sure it's an actual boolean. This has been the case since gin's initial commit in 8a3631f8d86cdd9b0 . Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers