Re: GinPageIs* don't actually return a boolean

On Wed, Mar 2, 2016 at 9:48 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> 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 <stdbool.h>
> 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 <anonymous>
> *)}' but argument is of type 'GinTernaryValue (*)(void *, QueryOperand
> *) {aka char (*)(void *, struct <anonymous> *)}'
>
> 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