Re: Warnings around booleans

Andres,

* Andres Freund (andres(at)anarazel(dot)de) wrote:
> Forcing our bool to be stdbool.h shows up a bunch of errors and
> warnings:

Wow.

> 1) gin stores/queries some bools as GinTernaryValue.
>
> Part of this is easy to fix, just adjust GinScanKeyData->entryRes to
> be a GinTernaryValue (it's actually is compared against MAYBE).
>
> What I find slightly worrysome is that in gin_tsquery_consistent()
> checkcondition_gin (returning GinTernaryValue) is passed as a
> callback that's expected to return bool. And the field
> checkcondition_gin is returning (GinChkVal->check[i]) actually is a
> ternary.

Is there a potential corruption issue from that..?

> 2) help_config.c has a member named 'bool' in a local struct. That
> doesn't work well if bool actually is a macro. Which mean that whole
> #ifndef bool patch in c.h wasn't exercised since 2003 when
> help_config.c was created...
>
> => rename field to bool_ (or _bool but that looks a wee close to C's _Bool)

I don't particularly like just renaming it to bool_, for my part, but
certainly need to do something.

> 3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two
> complaints:

I assume you mean AlterRole() above?

> bool bypassrls = -1;
>
> it's a boolean.
>
> else if (authform->rolbypassrls || bypassrls >= 0)
> {
> if (!superuser())
> ereport(ERROR,
> (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> errmsg("must be superuser to change bypassrls attribute")));
> }
> ...
> if (bypassrls >= 0)
> {
> new_record[Anum_pg_authid_rolbypassrls - 1] = BoolGetDatum(bypassrls > 0);
> new_record_repl[Anum_pg_authid_rolbypassrls - 1] = true;
> }
>
> if it's a boolean, that's always true.
>
>
> It's not entirely clear to me why this was written that way. Stephen?

I specifically recall fixing a similar issue in this area but clearly
didn't realize that it was a bool when, as I recall, I was changing it
to match what we do for all of the other role attributes. In those
other cases the value is an int, not a bool though. Changing it to an
int here should make it line up with the rest of AlterRole().

I'll do that and add regression tests for it and any others which don't
get tested. My thinking on the test is to independently change each
value and then poll for all role attributes set and verify that the only
change made was the change expected.

> 3) vacuumdb.c stores -1's in a bool (result) in vacuum_one_database().
>
> => convert to an int

Perhaps I'm missing something, but it appears to only ever be set to 0
or -1, so wouldn't it make sense to just correct it to use bool properly
instead?

As for what's happening, it appears that we'll happily continue to go
through all of the databases even on an error condition, right? I seem
to recall seeing that happen and being a bit surprised at it, but wasn't
in a position at the time to debug it.

> 4) _tableInfo->relreplident is a bool, should be a char

This results in pg_dump always failing to dump out the necessary
ALTER TABLE ONLY ... REPLICA IDENTITY ... when it's actually required,
right? Is that something we can add a regression test to cover which
will be exercised through the pg_upgrade path?

Thanks!

Stephen