Re: Warnings around booleans

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.