Re: 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 <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.