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

> On Jul 12, 2020, at 4:59 AM, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Sat, Jul 11, 2020 at 03:32:55PM -0400, Tom Lane wrote:
>> Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com> writes:
>>> 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
>
> There are more code paths than what's mentioned upthread when it comes
> to relkinds and \0. For example, I can quickly grep for acl.c that
> relies on get_rel_relkind() returning \0 when the relkind cannot be
> found. And we do that for get_typtype() as well in the syscache.

I was thinking about places in the code that test a relkind variable against a list of values, rather than places that return a relkind to callers, though certainly those two things are related. It's possible that I've missed some places in the code where \0 might be encountered, but I've added Asserts against unexpected values in v3.

I left get_rel_relkind() as is. There does not seem to be anything wrong with it returning \0 as long as all callers are prepared to deal with that result.

>
>>> 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.
>
> I agree here that the situation could be improved because we never
> store this value in the catalogs. Perhaps there would be a benefit in
> switching to an enum in the long run, I am not sure. But if we do so,
> RELKIND_NULL should not be around.

In the v3 patch, I have removed RELKIND_NULL from the enum, and also removed default: labels from switches over RelKind. The patch is also rebased.