Re: relkind as an enum

Hi Alvaro,
I think this change will help to spot easily all the places where a
new relkind should be handled. Finding all the places where a new
relkind should be handled is a hard task right now.

On Mon, Feb 2, 2026 at 8:11 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre(at)kurilemu(dot)de> writes:
> > There have been mentions of turning Form_pg_class->relkind into an enum,
> > so that we can have compilers provide some more help with
> > switch(relkind) blocks. Here's a quick experiment with that.
> > ...
> > 0003 is the backend-side change. This looks generally reasonable,
> > though I'm annoyed that I couldn't find a way to coerce the compiler
> > into telling me if I had missed some spot.
>
> Yeah. On the whole I'm dubious that this is worth the code churn,
> because I doubt that it will move the needle at all on getting
> better compiler warnings. One reason why I think that is that
> I don't think it's okay to assume that what comes off of disk
> for the contents of pg_class.relkind is necessarily one of the
> valid values: catalog corruption would break that assumption.
> So even in places where it's reasonable to list every RELKIND_
> value in a switch (and in many of them it looks like that would
> be tedium), we'd still need to cater for a default: clause.
> And then we'd get no warnings, so what have we gained?
>
> regards, tom lane
>
>

At most of the places which have default case (or an all catching else
clause), it's hard to know whether the default case applies to valid
relkinds that are not covered by earlier cases/if else branches or it
applies to corrupted relkinds.

The places where we read pg_class.relkind from catalog are lesser than
the places where it's used. If we validate the relkind read at these
places, we could avoid leaking invalid relkind values all over the
code. Then there will be no reason to use default. There will be
places where we will need to list a handful of relkinds even if they
are not used in that place or can not be there. But I think, it will
also force the author to explain why those are not expected or used
there; the code will be better documented than it is today - where we
just act as if we don't know this relkind and do something default or
just ignore it.

There's a lot of code churn, I agree. But we don't have to do it all
at once. Start with some simple switch cases and then as and tidy the
code as and when it's touched.

Attached patchset has previous patches + 0004, which has code
modifications along these lines but enough to give an idea.

1. get_rel_relkind() - it currently returns '\0' when it doesn't find
the given reloid. The comments there don't tell why it doesn't raise
an error. I have changed it to raise an error.
2. Changes to AllocateRelationDesc() and RelationBuildLocalRelation()
create a relation descriptor. If rel->rd_rel->relkind is not a valid
relkind, AllocateRelationDesc() will throw an error. This will make
sure that every relcache entry will have a valid relkind.
RelationBuildLocalRelation() is handed down a relkind when creating a
new relation, so the relkind better be valid. Hence added an Assert.
This should take care of one case where we may read an invalid relkind
from the disk.
3. extractRelOptions() - accesses relkind directly from Form_pg_class
- but I guess the pg_class entry there is validated at a much higher
level. Hence added just an Assert.

I think the first two cover most of the places where a corrupt relkind
can make it into a running server. There may be more, like
extractRelOptions(), which access relkind directly from the catalog.
Those need to be fixed as well. With the above three changes, I ran
`meson test` and didn't see any failures. I maybe missing some reason
that led us to let the invalid relkind spread all over the code.

To check whether a relkind is valid or not, I added RelkindIsValid() -
which uses a switch case, but does not have a default. With Relkind as
an enum, this function will fail to compile if a new relkind is not
added to the switch case.

With such checks the places where an on-disk relkind is read, we could
remove the default case from many, if not all, places. I tried a few.

pg_overexplain.c - this is interesting since RangeTblEntry::relkind is
not exactly the same as pg_class::relkind. RangeTblEntry::relkind will
be 0 when RTE doesn't refer to a catalog object - i.e. when RTE::relid
= 0. The patch uses RelkindIsValid() to Assert that the relkind
transferred from pg_class to RangeTblEntry is valid for a pg_class
object in parse_relation.c, worker.c, pg_output.c and ri_triggers.c.
This prevents invalid relkind from infecting the system through
RTE::relkind. In turn this allows us to remove default from switch
(RTE::relkind) as done in pg_overexplain(). As a side note, I am
actually doubtful whether it's safe to print an unknown non-'\0'
relkind, since it could be an unprintable character. Further
everywhere RTE::relkind is used, we will need to additionally handle 0
either as an Assert or in the switch case.

Changes to get_relkind_objtype(), errdetail_relkind_not_supported()
show the easy cases where we can eliminate default cases by just
declaring relkind as enum.

After converting relkind to enum, we need to make sure that we write
only a single byte value to the catalogs instead of coercing a
four/eight byte value into a single byte. I think we will need to add
RelkindIsValid() somewhere in the heap_create() for that.

Adding RELKIND_*_STR macros seems awkward to me, but I don't have any
better ideas.

--
Best Wishes,
Ashutosh Bapat