Horton

Thread

Commits

  1. Sort out table_open vs. relation_open in rewriter

  2. Rename validate_relation_kind()

  3. Flip logic in table validate_relation_kind

  4. Change error message for sequence validate_relation_kind()

  1. some validate_relation_kind() tidying

    Peter Eisentraut <peter@eisentraut.org> — 2026-02-18T15:15:09Z

    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).

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.

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".

These are the two attached patches.  This is just something I found 
while working on something else nearby.

  2. Re: some validate_relation_kind() tidying

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-02-19T08:48:22Z

    Hi Peter,

On Wed, Feb 18, 2026 at 8:45 PM Peter Eisentraut <peter@eisentraut.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
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.

> 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

  3. Re: some validate_relation_kind() tidying

    Junwang Zhao <zhjwpku@gmail.com> — 2026-02-20T12:49:03Z

    On Thu, Feb 19, 2026 at 4:48 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Hi Peter,
>
> On Wed, Feb 18, 2026 at 8:45 PM Peter Eisentraut <peter@eisentraut.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

  4. Re: some validate_relation_kind() tidying

    Peter Eisentraut <peter@eisentraut.org> — 2026-02-23T16:44:07Z

    On 19.02.26 09:48, Ashutosh Bapat wrote:
> The || should be &&. The bug shows up as an initdb failure
> 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.
> 
>> 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.

Thanks, committed with your fixes.  (I fine-tuned the comment in patch 
0003 a bit further, since arguably you can "reference" a composite type 
in a query.)

  5. Re: some validate_relation_kind() tidying

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-02-24T01:30:21Z

    On Mon, Feb 23, 2026 at 10:14 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 19.02.26 09:48, Ashutosh Bapat wrote:
> > The || should be &&. The bug shows up as an initdb failure
> > 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.
> >
> >> 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.
>
> Thanks, committed with your fixes.  (I fine-tuned the comment in patch
> 0003 a bit further, since arguably you can "reference" a composite type
> in a query.)
>

I actually wanted to write "referenced in FROM clause" but then target
relations are not part of FROM clause per say. The new comment is more
accurate.

AFAIK, composite type can be referenced in places where any other type
can be referenced. Is there a place where it can be referenced in
place of a regular table?

-- 
Best Wishes,
Ashutosh Bapat