Re: some validate_relation_kind() tidying

From: Junwang Zhao <zhjwpku(at)gmail(dot)com>
To: Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>
Cc: Peter Eisentraut <peter(at)eisentraut(dot)org>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: some validate_relation_kind() tidying
Date: 2026-02-20 12:49:03
Message-ID: CAEG8a3+=OXdJ2x8Q=2QcgMFp8OH119L4MR2FHXsYwjXJY6u+kQ@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, Feb 19, 2026 at 4:48 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> Hi Peter,
>
> On Wed, Feb 18, 2026 at 8:45 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> >
> > We have three different functions called validate_relation_kind(), namely in
> >
> > src/backend/access/index/indexam.c
> > src/backend/access/sequence/sequence.c
> > src/backend/access/table/table.c
> >
> > These all check which relkinds are permitted by index_open(),
> > sequence_open(), and table_open(), respectively, which are each wrappers
> > around relation_open() (which accepts any relkind).
> >
>
> It's better to use a different name for each of them as done in
> attached 0003. Same names can confuse code browsing tools like cscope
> and human reader alike.
>
> > I always found the one in table.c a little too mysterious, because it
> > just checks which relkinds it does *not* want, and so if you want to
> > know whether a particular relkind is suitable for table_open(), you need
> > to do additional research and check what all the possible relkinds are
> > and so on, and there is no real explanation why those choices were made.
> > I think it would be clearer and more robust and also more consistent
> > with the other ones if we flipped that around and listed the ones that
> > are acceptable and why.
> >
>
> The || should be &&. The bug shows up as an initdb failure

Yes, I noticed the same issue, using || will always evaluate to true,
which means it will always error out.

> running bootstrap script ... 2026-02-19 14:06:43.411 IST [197482]
> FATAL: cannot open relation "pg_type"
> 2026-02-19 14:06:43.411 IST [197482] DETAIL: This operation is not
> supported for tables.
>
> I think this is more future-proof. If a relkind gets added and needs
> to be in this list, we will notice it from the error. I think we
> should avoid mentioning specific relkinds in the comment as well since
> that list will need to be updated as the set of relkinds changes. Just
> mentioning the criteria should be enough. I have slightly improved the
> comment in the attached 0003.

The renaming looks reasonable to me.

>
> > Secondly, the sequence.c one was probably copied from the table.c one,
> > but I think we can make the error message a bit more direct by just
> > saying "... is not a sequence" instead of "cannot open relation".
> >
>
> +1.
>
> > These are the two attached patches. This is just something I found
> > while working on something else nearby.
>
> Attached are your two patches + bug fix in 0002 + my suggestions in 0003.
>
> --
> Best Wishes,
> Ashutosh Bapat

--
Regards
Junwang Zhao

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Matheus Alcantara 2026-02-20 12:58:45 Re: Show comments in \dRp+, \dRs+, and \dX+ psql meta-commands
Previous Message Ashutosh Sharma 2026-02-20 12:46:53 Re: Skipping schema changes in publication