Re: Warnings around booleans - Mailing list pgsql-hackers

On 2015-08-12 08:16:09 -0400, Stephen Frost wrote:
> > 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..?

I honestly don't understand the gin code well enough to answer 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.

There's alread a _enum in the struct, so that doesn't bother my much.

> > 3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two
> > complaints:
> 
> I assume you mean AlterRole() above?

Oops.

> >     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 find that a somewhat ugly coding pattern, but since the rest of the
function is written that way...

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

Do that if you like, but what I really think we should have is a test
that tries to bypass rls and fails, then the user gets changes and it
succeeds, and then it gets disabled and fails again. This really seems
test-worthy behaviour to me.

> > 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?

Yea, you're right.

> > 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?

With our current boolean definition this doesn't fail at all. All the
values fall into both char and unsigned char range.

WRT tests: It'd probably be a good idea to not drop the tables at the
end of replica_identity.sql.