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

> On Jul 10, 2020, at 11:00 PM, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Wed, Jul 01, 2020 at 05:04:19PM -0700, Mark Dilger wrote:
>> Most of the work in this patch is mechanical replacement of if/else
>> if/else statements which hinge on relkind to switch statements on
>> relkind. The patch is not philosophically very interesting, but it
>> is fairly long. Reviewers might start by scrolling down the patch
>> to the changes in src/include/catalog/pg_class.h
>>
>> There are no intentional behavioral changes in this patch.
>
> Please note that 0002 does not apply anymore, there are conflicts in
> heap.c and tablecmds.c, and that there are noise diffs, likely because
> you ran pgindent and included places unrelated to this patch.

I can resubmit, but should like to address your second point before bothering...

> Anyway,
> 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?

It is not the purpose of this patch to change the behavior of the code. This is just a structural patch, using an enum and switches rather than char and if/else if/else blocks.

Subsequent patches could build on this work, such as changing the behavior when code encounters a relkind value outside the code's expected set of relkind values. Whether those patches would add Assert()s, elog()s, or ereport()s is not something I'd like to have to debate as part of this patch submission. Assert()s have the advantage of costing nothing in production builds, but elog()s have the advantage of protecting against corrupt relkind values at runtime in production.

Getting the compiler to warn when a new relkind is added to the enumeration but not handled in a switch is difficult. One strategy is to add -Wswitch-enum, but that would require refactoring switches over all enums, not just over the RelKind enum, and for some enums, that would require a large number of extra lines to be added to the code. Another strategy is to remove the default label from switches over RelKind, but that removes protections against invalid relkinds being encountered.

Do you have a preference about which directions I should pursue? Or do you think the patch idea itself is dead?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company