Re: Towards easier AMs: Cleaning up inappropriate use of name "relkind"

Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com> writes:
> On Jul 10, 2020, at 11:00 PM, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> I am not really a fan of this patch. I could see a benefit in
>> switching to an enum so as for places where we use a switch/case
>> without a default we would be warned if a new relkind gets added or if
>> a value is not covered, but then we should not really need
>> RELKIND_NULL, no?

> There are code paths where relkind is sometimes '\0' under normal, non-exceptional conditions. This happens in

> allpaths.c: set_append_rel_size
> rewriteHandler.c: view_query_is_auto_updatable
> lockcmds.c: LockViewRecurse_walker
> pg_depend.c: getOwnedSequences_internal

> Doesn't this justify having RELKIND_NULL in the enum?

I'd say no. I think including an intentionally invalid value in such
an enum is horrid, mainly because it will force a lot of places to cover
that value when they shouldn't (or else draw "enum value not handled in
switch" warnings). The confusion factor about whether it maybe *is*
a valid value is not to be discounted, either.

If we can't readily get rid of the use of '\0' in these code paths,
maybe trying to convert to an enum isn't going to be a win after all.

> Getting the compiler to warn when a new relkind is added to the
> enumeration but not handled in a switch is difficult.

We already have a project policy about how to do that.

regards, tom lane