From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Peter Eisentraut <peter_e(at)gmx(dot)net> |
Cc: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, Yury Zhuravlev <u(dot)zhuravlev(at)postgrespro(dot)ru>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>, Teodor Sigaev <teodor(at)sigaev(dot)ru> |
Subject: | Re: GinPageIs* don't actually return a boolean |
Date: | 2016-03-11 03:59:51 |
Message-ID: | 20160311035951.us7p2cbcjzcyjo63@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2016-03-11 04:00:42 | Re: GinPageIs* don't actually return a boolean |
Previous Message | Michael Paquier | 2016-03-11 03:50:45 | Re: GinPageIs* don't actually return a boolean |