Hi,
On 2018-12-18 22:50:57 -0500, Tom Lane wrote:
> Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> > At Tue, 18 Dec 2018 17:13:08 -0800, Andres Freund <andres@anarazel.de> wrote in
<20181219011308.mopzyvc73nwjzdb6@alap3.anarazel.de>
> >> Right now there's no easy way to use the compiler to ensure that all
> >> places that need to deal with all kinds of relkinds check a new
> >> relkind. I think we should make that easier by moving RELKIND_* to an
> >> enum, with the existing letters as the value.
>
> > I think we cannot use enums having base-type, so it will work
> > unless we forget the cast within switch(). However, I don't think
> > it is not a reason not to do so.
> >
> > switch ((RelationRelkind) rel->rd_rel->relkind)
>
> Hm. This example doesn't seem very compelling. If we put a
> "default: elog(ERROR...)" into the switch, compilers will not warn
> us about omitted values. On the other hand, if we remove the default:
> then we lose robustness against corrupted relkind values coming from disk,
> which I think is going to be a net negative.
I don't quite see the from-disk bit being particularly critical, that
seems like a fairly minor safety net. Most of those switches are at
places considerably later than where on-disk corruption normally would
be apparent. If we really are concerned with that, it'd not be too hard
to add a relkind_as_enum() wrapper function that errored out on an
inpermissible value.
> Not to mention that we get no help at all unless we remember to add
> the cast to every such switch.
That doesn't seem too bad. A manual search and replace would probably
find the majority within an hour or two of effort.
> So, while I understand Andres' desire to make this more bulletproof,
> I'm not quite sure how this proposal helps in practice.
We've repeatedly forgotten to add new relkinds for checks that we
already have. Manually adding the cast won't be bulletproof, but it sure
would be more robust than what we have now. There's plenty switches
where we do know that we want the exhaustive list, adding the cast there
would already make things more robust, even if we miss other places.
It'd be nice if there were an easy way to write a switch() where the
compiler enforces that all enum values are checked, but still had the
possibility to have a 'default:' block for error checking... I can't
quite come up with a good approach to emulate that though.
Greetings,
Andres Freund