Re: Warnings around booleans

* Andres Freund (andres(at)anarazel(dot)de) wrote:
> 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.

Yeah, neither do I, so I've added Heikki. Heikki, any idea as to the
impact of this?

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

Fair enough.

> > > 3) The 'bypassrls' boolean in AlterUser() is queried strangely. There are two
> > > complaints:
[...]
> > 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...

Agreed. My role attributes patch had actually changed how that all was
done, but once the discussion moved on to default roles instead of a
bunch of additional role attributes, it felt like it would be
unnecessary code churn to change that function. Perhaps we should do so
anyway though.

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

We certainly do have tests around BYPASSRLS, but not one which changes
an independent role attribute and then re-tests. I'm fine with adding
that test (or other tests, in general), but that won't help when another
role attribute is added and someone makes a similar mistake, which I
believe the test I'm thinking of will. Even if I'm unable to make that
work though, someone grep'ing through our tests for places to add their
new role attribute would likely catch a test that's checking all of the
other ones while they might not consider what's done for just one
attribute to be a generally applicable case.

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

Agreed.

Thanks!

Stephen