some validate_relation_kind() tidying

Started by Peter Eisentraut16 days ago5 messages
Jump to latest
#1Peter Eisentraut
peter_e@gmx.net

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.

Attachments:

0001-Change-error-message-for-sequence-validate_relation_.patchtext/plain; charset=UTF-8; name=0001-Change-error-message-for-sequence-validate_relation_.patchDownload+4-7
0002-Flip-logic-in-table-validate_relation_kind.patchtext/plain; charset=UTF-8; name=0002-Flip-logic-in-table-validate_relation_kind.patchDownload+9-5
#2Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Peter Eisentraut (#1)
Re: some validate_relation_kind() tidying

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

Attachments:

v20260219-0001-Change-error-message-for-sequence-validate.patchtext/x-patch; charset=US-ASCII; name=v20260219-0001-Change-error-message-for-sequence-validate.patchDownload+4-7
v20260219-0002-Flip-logic-in-table-validate_relation_kind.patchtext/x-patch; charset=US-ASCII; name=v20260219-0002-Flip-logic-in-table-validate_relation_kind.patchDownload+9-5
v20260219-0003-Rename-validate_relation_kind.patchtext/x-patch; charset=US-ASCII; name=v20260219-0003-Rename-validate_relation_kind.patchDownload+18-19
#3Junwang Zhao
zhjwpku@gmail.com
In reply to: Ashutosh Bapat (#2)
Re: some validate_relation_kind() tidying

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

#4Peter Eisentraut
peter_e@gmx.net
In reply to: Ashutosh Bapat (#2)
Re: some validate_relation_kind() tidying

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

#5Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Peter Eisentraut (#4)
Re: some validate_relation_kind() tidying

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