> On Jul 10, 2020, at 11:00 PM, Michael Paquier <mich...@paquier.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
