Assertion failure in smgr.c when using pg_prewarm with partitioned tables

On Thu, May 15, 2025 at 2:22 PM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote:

Hi,

I encountered an assertion failure when a partitioned table is specified
as an argument to pg_prewarm. Below are the steps to reproduce the
issue:

$ pgbench -i -s 1 --partitions=3
$ psql <<EOF
CREATE EXTENSION pg_prewarm;
SELECT pg_prewarm('pgbench_accounts');
EOF

The following assertion failure occurs:

TRAP: failed Assert("RelFileNumberIsValid(rlocator.relNumber)"), File:
"smgr.c", Line: 246, PID: 1246282
postgres: ikeda postgres [local]
SELECT(ExceptionalCondition+0xbb)[0x55edd16725c1]
postgres: ikeda postgres [local] SELECT(smgropen+0x5e)[0x55edd145c1ff]

It looks like this may have been overlooked in commit 049ef33.
What do you think?

Yeah, this should be fixed, don't you think that instead of checking
the relnumber is valid, we shall check whether it's a partitioned rel
and give a separate error that prewarm is not supported for
partitioned tables? And in fact, we can think of supporting this for
the partitioned tables as well in the future, where we can loop
through all the partitions and do prewarm for each of them.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On 2025/05/15 18:20, Dilip Kumar wrote:

On Thu, May 15, 2025 at 2:22 PM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote:

Hi,

I encountered an assertion failure when a partitioned table is specified
as an argument to pg_prewarm. Below are the steps to reproduce the
issue:

Thanks for the report!

This assertion failure can also occur when pg_prewarm() is run on objects like
foreign tables, plain views, or other relations that don't have storage - not just
partitioned tables.

$ pgbench -i -s 1 --partitions=3
$ psql <<EOF
CREATE EXTENSION pg_prewarm;
SELECT pg_prewarm('pgbench_accounts');
EOF

The following assertion failure occurs:

TRAP: failed Assert("RelFileNumberIsValid(rlocator.relNumber)"), File:
"smgr.c", Line: 246, PID: 1246282
postgres: ikeda postgres [local]
SELECT(ExceptionalCondition+0xbb)[0x55edd16725c1]
postgres: ikeda postgres [local] SELECT(smgropen+0x5e)[0x55edd145c1ff]

It looks like this may have been overlooked in commit 049ef33.
What do you think?

Yeah, this should be fixed, don't you think that instead of checking
the relnumber is valid,

+1

How about adding a check to see whether the target relation has storage,
using something like RELKIND_HAS_STORAGE()?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Thu, May 15, 2025 at 3:14 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/05/15 18:20, Dilip Kumar wrote:

On Thu, May 15, 2025 at 2:22 PM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote:

Hi,

I encountered an assertion failure when a partitioned table is specified
as an argument to pg_prewarm. Below are the steps to reproduce the
issue:

Thanks for the report!

This assertion failure can also occur when pg_prewarm() is run on objects like
foreign tables, plain views, or other relations that don't have storage - not just
partitioned tables.

$ pgbench -i -s 1 --partitions=3
$ psql <<EOF
CREATE EXTENSION pg_prewarm;
SELECT pg_prewarm('pgbench_accounts');
EOF

The following assertion failure occurs:

TRAP: failed Assert("RelFileNumberIsValid(rlocator.relNumber)"), File:
"smgr.c", Line: 246, PID: 1246282
postgres: ikeda postgres [local]
SELECT(ExceptionalCondition+0xbb)[0x55edd16725c1]
postgres: ikeda postgres [local] SELECT(smgropen+0x5e)[0x55edd145c1ff]

It looks like this may have been overlooked in commit 049ef33.
What do you think?

Yeah, this should be fixed, don't you think that instead of checking
the relnumber is valid,

+1

How about adding a check to see whether the target relation has storage,
using something like RELKIND_HAS_STORAGE()?

Yeah, that makes more sense.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 15, 2025 at 6:50 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, May 15, 2025 at 3:14 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

How about adding a check to see whether the target relation has storage,
using something like RELKIND_HAS_STORAGE()?

Yeah, that makes more sense.

+1. FWIW, not long ago we fixed a similar Assert failure in
contrib/pg_freespacemap by verifying RELKIND_HAS_STORAGE() before
trying to access the storage (see 4623d7144). Wondering if there are
other similar issues elsewhere in contrib ...

Thanks
Richard

Thank you for your comments!

I updated the patch to use RELKIND_HAS_STORAGE() as done in
commit 4623d7144. Please see the v2-0001 patch for the changes.

On 2025-05-15 19:57, Richard Guo wrote:

+1. FWIW, not long ago we fixed a similar Assert failure in
contrib/pg_freespacemap by verifying RELKIND_HAS_STORAGE() before
trying to access the storage (see 4623d7144). Wondering if there are
other similar issues elsewhere in contrib ...

I tested all contrib functions that take regclass arguments (see
attached test.sql and test_result.txt). The result shows that only
pg_prewarm() can lead to the assertion failure.

However, I found that amcheck's error messages can be misleading
when run on partitioned indexes.

For example, on the master branch, amcheck (e.g., bt_index_check
function)
shows this error:

ERROR: expected "btree" index as targets for verification
DETAIL: Relation "pgbench_accounts_pkey" is a btree index.

This message says it expects a "btree" index, yet also states the
relation
is a "btree" index, which can seem contradictory. The actual issue is
that
the index is a btree partitioned index, but this detail is missing,
causing
possible confusion.

Therefore, I thought it would be better to update the detailed message
as shown in the v2-0002 patch:

* master with 0002 patch

ERROR: expected "btree" index as targets for verification
DETAIL: Relation "pgbench_accounts_pkey" is a btree partitioned

index.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

On Fri, May 16, 2025 at 11:51 AM Masahiro Ikeda
<ikedamsh@oss.nttdata.com> wrote:

Thank you for your comments!

I updated the patch to use RELKIND_HAS_STORAGE() as done in
commit 4623d7144. Please see the v2-0001 patch for the changes.

On 2025-05-15 19:57, Richard Guo wrote:

+1. FWIW, not long ago we fixed a similar Assert failure in
contrib/pg_freespacemap by verifying RELKIND_HAS_STORAGE() before
trying to access the storage (see 4623d7144). Wondering if there are
other similar issues elsewhere in contrib ...

I tested all contrib functions that take regclass arguments (see
attached test.sql and test_result.txt). The result shows that only
pg_prewarm() can lead to the assertion failure.

Right, even while I was searching, I see this is used in 3 contrib
modules, amcheck, autoprewarm, and pg_prewarm. amcheck is already
checking for AM type, and Autoprewarm is identifying the relation by
block, so there is no chance of getting the relation which do not have
storage.

However, I found that amcheck's error messages can be misleading
when run on partitioned indexes.

For example, on the master branch, amcheck (e.g., bt_index_check
function)
shows this error:

ERROR: expected "btree" index as targets for verification
DETAIL: Relation "pgbench_accounts_pkey" is a btree index.

This message says it expects a "btree" index, yet also states the
relation
is a "btree" index, which can seem contradictory. The actual issue is
that
the index is a btree partitioned index, but this detail is missing,
causing
possible confusion.

Yes, I found the error message confusing during testing as well, so it
makes sense to improve it.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On 2025/05/16 15:33, Dilip Kumar wrote:

On Fri, May 16, 2025 at 11:51 AM Masahiro Ikeda
<ikedamsh@oss.nttdata.com> wrote:

Thank you for your comments!

I updated the patch to use RELKIND_HAS_STORAGE() as done in
commit 4623d7144. Please see the v2-0001 patch for the changes.

Thanks for updating the patch!

You added a test for pg_prewarm() with a partitioned table in the TAP tests.
But, wouldn't it be simpler and easier to maintain if we added this
as a regular regression test (i.e., using .sql and .out files) instead of
TAP tests? That should be sufficient for this case?

Also, since the issue was introduced in v17, this patch should be
back-patched to v17, right?

On 2025-05-15 19:57, Richard Guo wrote:

+1. FWIW, not long ago we fixed a similar Assert failure in
contrib/pg_freespacemap by verifying RELKIND_HAS_STORAGE() before
trying to access the storage (see 4623d7144). Wondering if there are
other similar issues elsewhere in contrib ...

I tested all contrib functions that take regclass arguments (see
attached test.sql and test_result.txt). The result shows that only
pg_prewarm() can lead to the assertion failure.

Right, even while I was searching, I see this is used in 3 contrib
modules, amcheck, autoprewarm, and pg_prewarm. amcheck is already
checking for AM type, and Autoprewarm is identifying the relation by
block, so there is no chance of getting the relation which do not have
storage.

However, I found that amcheck's error messages can be misleading
when run on partitioned indexes.

For example, on the master branch, amcheck (e.g., bt_index_check
function)
shows this error:

ERROR: expected "btree" index as targets for verification
DETAIL: Relation "pgbench_accounts_pkey" is a btree index.

This message says it expects a "btree" index, yet also states the
relation
is a "btree" index, which can seem contradictory. The actual issue is
that
the index is a btree partitioned index, but this detail is missing,
causing
possible confusion.

Yes, I found the error message confusing during testing as well, so it
makes sense to improve it.

+1

Regarding the patch, how about also adding a regression test for
amcheck with a partitioned index?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Thanks for your work and feedback!

I've updated the patches and added regular regression tests for
both pg_prewarm and amcheck.

On 2025-05-16 21:01, Fujii Masao wrote:

Also, since the issue was introduced in v17, this patch should be
back-patched to v17, right?

I agree with back-patching v3-0001. I was able to reproduce the issue
on the REL_17_STABLE branch. One concern is that this patch changes
the error message in production:

* v17.5 (without --enable-cassert)

ERROR: fork "main" does not exist for this relation

* REL_17_STABLE with the v3-0001 patch (without --enable-cassert)

ERROR: relation "test" does not have storage

However, I think preventing the assertion failure should take priority.

As for v3-0002, I think it's fine to apply it to the master branch only,
since it just makes a cosmetic improvement to the error message.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

On Mon, May 19, 2025 at 5:18 PM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote:

Thanks for your work and feedback!

I've updated the patches and added regular regression tests for
both pg_prewarm and amcheck.

Thanks for updating the patches!

Regarding the 0001 patch:

+CREATE TABLE test_part1 PARTITION OF test FOR VALUES FROM (1) TO (1000);
+INSERT INTO test SELECT generate_series(1, 100);

These lines don't seem necessary for the test. How about removing them?
It would simplify the test and slightly reduce the execution time - though
the time savings are very minimal.

+-- To specify the relation which does not have storage should fail.

This comment could be clearer as:
"pg_prewarm() should fail if the target relation has no storage."

+ /* Check that the storage exists. */

Maybe rephrase to:
"Check that the relation has storage."?

Regarding the 0002 patch:

- errdetail("Relation \"%s\" is a %s index.",
-    RelationGetRelationName(rel), NameStr(((Form_pg_am)
GETSTRUCT(amtuprel))->amname))));
+ errdetail("Relation \"%s\" is a %s %sindex.",
+    RelationGetRelationName(rel), NameStr(((Form_pg_am)
GETSTRUCT(amtuprel))->amname),
+    (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX) ?
"partitioned " : "")));

Instead of manually building the message, how about using
errdetail_relkind_not_supported()?
It would be more consistent with similar error reporting elsewhere.

I agree with back-patching v3-0001. I was able to reproduce the issue
on the REL_17_STABLE branch.

Thanks for the confirmation.

One concern is that this patch changes
the error message in production:

* v17.5 (without --enable-cassert)

ERROR: fork "main" does not exist for this relation

* REL_17_STABLE with the v3-0001 patch (without --enable-cassert)

ERROR: relation "test" does not have storage

However, I think preventing the assertion failure should take priority.

Yes.

Regards,

--
Fujii Masao

<div dir='auto'><div><div><div class="elided-text">2025/05/21 12:54 Fujii Masao &lt;masao.fujii@gmail.com&gt;:<br type="attribution"><blockquote style="margin:0 0 0 0.8ex;border-left:1px #ccc solid;padding-left:1ex"><p dir="ltr">On Mon, May 19, 2025 at 5:18 PM Masahiro Ikeda &lt;ikedamsh@oss.nttdata.com&gt; wrote:
<br>
&gt;
<br>
&gt; Thanks for your work and feedback!
<br>
&gt;
<br>
&gt; I've updated the patches and added regular regression tests for
<br>
&gt; both pg_prewarm and amcheck.
<br>

<br>
Thanks for updating the patches!
<br>

<br>
Regarding the 0001 patch:
<br>

<br>
+CREATE TABLE test_part1 PARTITION OF test FOR VALUES FROM (1) TO (1000);
<br>
+INSERT INTO test SELECT generate_series(1, 100);
<br>

<br>
These lines don't seem necessary for the test. How about removing them?
<br>
It would simplify the test and slightly reduce the execution time - though
<br>
the time savings are very minimal.
<br>

<br>
+-- To specify the relation which does not have storage should fail.
<br>

<br>
This comment could be clearer as:
<br>
"pg_prewarm() should fail if the target relation has no storage."
<br>

<br>
+ /* Check that the storage exists. */
<br>

<br>
Maybe rephrase to:
<br>
"Check that the relation has storage." ?<br></p></blockquote></div></div></div><div dir="auto"><br></div><div dir="auto">Thanks! I will fix them.</div><div dir="auto"><br></div><div dir="auto"><div><div class="elided-text"><blockquote style="margin:0 0 0 0.8ex;border-left:1px #ccc solid;padding-left:1ex"><p dir="ltr">
Regarding the 0002 patch:
<br>

<br>
- errdetail("Relation \"%s\" is a %s index.",
<br>
-&nbsp;&nbsp;&nbsp; RelationGetRelationName(rel), NameStr(((Form_pg_am)
<br>
GETSTRUCT(amtuprel))-&gt;amname))));
<br>
+ errdetail("Relation \"%s\" is a %s %sindex.",
<br>
+&nbsp;&nbsp;&nbsp; RelationGetRelationName(rel), NameStr(((Form_pg_am)
<br>
GETSTRUCT(amtuprel))-&gt;amname),
<br>
+&nbsp;&nbsp;&nbsp; (rel-&gt;rd_rel-&gt;relkind == RELKIND_PARTITIONED_INDEX) ?
<br>
"partitioned " : "")));
<br>

<br>
Instead of manually building the message, how about using
<br>
errdetail_relkind_not_supported()?
<br>
It would be more consistent with similar error reporting elsewhere.</p></blockquote></div></div></div><div dir="auto">I was thinking of using errdetail_relkind_not_supported(),</div><div dir="auto">but I’m reconsidering building the message manually</div><div dir="auto">since the AM name seems to be important for the error.</div><div dir="auto">What do you think?</div><div dir="auto"><br></div><div dir="auto">Regards,</div><div dir="auto">--</div><div dir="auto">Masahiro Ikeda</div><div dir="auto"><br></div><div dir="auto"><div><br></div></div></div>

On 2025/05/26 16:55, ikedamsh wrote:

2025/05/21 12:54 Fujii Masao <masao.fujii@gmail.com>:

On Mon, May 19, 2025 at 5:18 PM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote:

Thanks for your work and feedback!

I've updated the patches and added regular regression tests for
both pg_prewarm and amcheck.

Thanks for updating the patches!

Regarding the 0001 patch:

+CREATE TABLE test_part1 PARTITION OF test FOR VALUES FROM (1) TO (1000);
+INSERT INTO test SELECT generate_series(1, 100);

These lines don't seem necessary for the test. How about removing them?
It would simplify the test and slightly reduce the execution time - though
the time savings are very minimal.

+-- To specify the relation which does not have storage should fail.

This comment could be clearer as:
"pg_prewarm() should fail if the target relation has no storage."

+ /* Check that the storage exists. */

Maybe rephrase to:
"Check that the relation has storage." ?

Thanks! I will fix them.

Thanks!

Regarding the 0002 patch:

- errdetail("Relation \"%s\" is a %s index.",
-    RelationGetRelationName(rel), NameStr(((Form_pg_am)
GETSTRUCT(amtuprel))->amname))));
+ errdetail("Relation \"%s\" is a %s %sindex.",
+    RelationGetRelationName(rel), NameStr(((Form_pg_am)
GETSTRUCT(amtuprel))->amname),
+    (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX) ?
"partitioned " : "")));

Instead of manually building the message, how about using
errdetail_relkind_not_supported()?
It would be more consistent with similar error reporting elsewhere.

I was thinking of using errdetail_relkind_not_supported(),
but I’m reconsidering building the message manually
since the AM name seems to be important for the error.
What do you think?

Understood.
I was trying to better understand the error message, as I found
the following is still a bit confusing for users. However, I don't
have a better suggestion at the moment, so I'm okay with
the proposed change.

ERROR: expected "btree" index as targets for verification
DETAIL: Relation "pgbench_accounts_pkey" is a btree partitioned

This is not directly relation to your proposal, but while reading
the index_checkable() function, I noticed that ReleaseSysCache()
is not called after SearchSysCache1(). Shouldn't we call
ReleaseSysCache() here? Alternatively, we could use get_am_name()
instead of SearchSysCache1(), which might be simpler.

I also observed that the error code ERRCODE_FEATURE_NOT_SUPPORTED
is used when the relation is not the expected type in index_checkable().
However, based on similar cases (e.g., pgstattuple), it seems that
ERRCODE_WRONG_OBJECT_TYPE might be more appropriate in this situation.
Thought?

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

Thanks for your feedback. I've attached the updated patches.

On 2025-05-28 10:10, Fujii Masao wrote:

On 2025/05/26 16:55, ikedamsh wrote:

2025/05/21 12:54 Fujii Masao <masao(dot)fujii(at)gmail(dot)com>:
Regarding the 0002 patch:

- errdetail("Relation \"%s\" is a %s index.",
-    RelationGetRelationName(rel), NameStr(((Form_pg_am)
GETSTRUCT(amtuprel))->amname))));
+ errdetail("Relation \"%s\" is a %s %sindex.",
+    RelationGetRelationName(rel), NameStr(((Form_pg_am)
GETSTRUCT(amtuprel))->amname),
+    (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX) ?
"partitioned " : "")));

Instead of manually building the message, how about using
errdetail_relkind_not_supported()?
It would be more consistent with similar error reporting
elsewhere.

I was thinking of using errdetail_relkind_not_supported(),
but I’m reconsidering building the message manually
since the AM name seems to be important for the error.
What do you think?

Understood.
I was trying to better understand the error message, as I found
the following is still a bit confusing for users. However, I don't
have a better suggestion at the moment, so I'm okay with
the proposed change.

ERROR: expected "btree" index as targets for verification
DETAIL: Relation "pgbench_accounts_pkey" is a btree partitioned

What do you think about adding new error messages specifically for
partitioned
indexes?

If the target is a partitioned index, the error messages would be:

ERROR: expected index as targets for verification
DETAIL: This operation is not supported for partitioned indexes.

If the target is an index, but its access method is not supported, the
error
messages would be:

ERROR: expected "btree" index as targets for verification
DETAIL: Relation "bttest_a_brin_idx" is a brin index.

This is not directly relation to your proposal, but while reading
the index_checkable() function, I noticed that ReleaseSysCache()
is not called after SearchSysCache1(). Shouldn't we call
ReleaseSysCache() here? Alternatively, we could use get_am_name()
instead of SearchSysCache1(), which might be simpler.

Agreed.

I also observed that the error code ERRCODE_FEATURE_NOT_SUPPORTED
is used when the relation is not the expected type in
index_checkable().
However, based on similar cases (e.g., pgstattuple), it seems that
ERRCODE_WRONG_OBJECT_TYPE might be more appropriate in this situation.
Thought?

Agreed. I also change the error code to
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE
when the index is not valid.

Regards,
--
Masahiro Ikeda
NTT DATA Japan Corporation

On 2025/05/29 11:46, Masahiro Ikeda wrote:

Thanks for your feedback. I've attached the updated patches.

I've pushed the 0001 patch. Thanks!

What do you think about adding new error messages specifically for partitioned
indexes?

If the target is a partitioned index, the error messages would be:

  ERROR:  expected index as targets for verification
  DETAIL:  This operation is not supported for partitioned indexes.

If the target is an index, but its access method is not supported, the error
messages would be:

  ERROR:  expected "btree" index as targets for verification
  DETAIL:  Relation "bttest_a_brin_idx" is a brin index.

Thanks for considering this. The proposed messages look good to me.

This is not directly relation to your proposal, but while reading
the index_checkable() function, I noticed that ReleaseSysCache()
is not called after SearchSysCache1(). Shouldn't we call
ReleaseSysCache() here? Alternatively, we could use get_am_name()
instead of SearchSysCache1(), which might be simpler.

Agreed.

I may have been mistaken earlier. Based on the comment in SearchSysCache(),
the tuple returned by SearchSysCache1() is valid until the end of the transaction.
Since index_checkable() raises an error and ends the transaction immediately
after calling SearchSysCache1(), it seems safe to skip ReleaseSysCache()
in this case. Using get_am_name() instead seems simpler, though.
Thought?

I also observed that the error code ERRCODE_FEATURE_NOT_SUPPORTED
is used when the relation is not the expected type in index_checkable().
However, based on similar cases (e.g., pgstattuple), it seems that
ERRCODE_WRONG_OBJECT_TYPE might be more appropriate in this situation.
Thought?

Agreed. I also change the error code to ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE
when the index is not valid.

+1.
Should we commit patch 0003 before 0002? Also, should we consider back-patching it?

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

On 2025-05-30 01:44, Fujii Masao wrote:

I've pushed the 0001 patch. Thanks!

Thanks!

This is not directly relation to your proposal, but while reading
the index_checkable() function, I noticed that ReleaseSysCache()
is not called after SearchSysCache1(). Shouldn't we call
ReleaseSysCache() here? Alternatively, we could use get_am_name()
instead of SearchSysCache1(), which might be simpler.

Agreed.

I may have been mistaken earlier. Based on the comment in
SearchSysCache(),
the tuple returned by SearchSysCache1() is valid until the end of the
transaction.
Since index_checkable() raises an error and ends the transaction
immediately
after calling SearchSysCache1(), it seems safe to skip
ReleaseSysCache()
in this case. Using get_am_name() instead seems simpler, though.
Thought?

As you said, it seems safe since SearchSysCache() is only used for
constructing
the error message. However, using get_am_name() is simpler and cleaner.

I also observed that the error code ERRCODE_FEATURE_NOT_SUPPORTED
is used when the relation is not the expected type in
index_checkable().
However, based on similar cases (e.g., pgstattuple), it seems that
ERRCODE_WRONG_OBJECT_TYPE might be more appropriate in this
situation.
Thought?

Agreed. I also change the error code to
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE
when the index is not valid.

+1.
Should we commit patch 0003 before 0002? Also, should we consider
back-patching it?

OK, I think v5-0002 should be back-patched, since using incorrect error
codes is essentially a bug.

Regards,
--
Masahiro Ikeda
NTT DATA Japan Corporation

On 2025/06/02 16:32, Masahiro Ikeda wrote:

OK, I think v5-0002 should be back-patched, since using incorrect error codes is essentially a bug.

Thanks for updating the patches!

While reviewing the code:

                 amtup = SearchSysCache1(AMOID, ObjectIdGetDatum(am_id));
                 amtuprel = SearchSysCache1(AMOID, ObjectIdGetDatum(rel->rd_rel->relam));
                 ereport(ERROR,
-                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                                  errmsg("expected \"%s\" index as targets for verification", NameStr(((Form_pg_am) GETSTRUCT(amtup))->amname)),
                                  errdetail("Relation \"%s\" is a %s index.",
                                                    RelationGetRelationName(rel), NameStr(((Form_pg_am) GETSTRUCT(amtuprel))->amname))));

I initially thought switching to ERRCODE_WRONG_OBJECT_TYPE made sense,
but after checking similar cases, I'm reconsidering. For instance,
pgstat_relation() in pgstattuple.c uses ERRCODE_FEATURE_NOT_SUPPORTED
when the access method is unexpected. That suggests using
FEATURE_NOT_SUPPORTED here may not be incorrect.

         if (!rel->rd_index->indisvalid)
                 ereport(ERROR,
-                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                                  errmsg("cannot check index \"%s\"",
                                                 RelationGetRelationName(rel)),
                                  errdetail("Index is not valid.")));

Other parts of the codebase (including pgstattuple) use
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE when rejecting invalid index.
So, changing to this error code still seems reasonable and more
consistent overall.

Given that, I'd like to merge just this error code change from
the 0002 patch into 0003, and apply the combined patch to
the master branch only. Although I briefly considered backpatching
the change of error code, it could be seen as a behavioral change,
and the existing error code doesn't cause any real problem in older
branches. So it's probably better to leave it as-is there.

The updated patch is attached. It also changes index_checkable()
from extern to static, since it's only used within verify_common.c.

Thoughts?

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

On 2025-07-04 20:32, Fujii Masao wrote:

On 2025/06/02 16:32, Masahiro Ikeda wrote:

OK, I think v5-0002 should be back-patched, since using incorrect
error codes is essentially a bug.

Thanks for updating the patches!

While reviewing the code:

amtup = SearchSysCache1(AMOID, 
ObjectIdGetDatum(am_id));
amtuprel = SearchSysCache1(AMOID,
ObjectIdGetDatum(rel->rd_rel->relam));
ereport(ERROR,
-                               
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("expected \"%s\" index as
targets for verification", NameStr(((Form_pg_am)
GETSTRUCT(amtup))->amname)),
errdetail("Relation \"%s\" is a %s 
index.",

RelationGetRelationName(rel), NameStr(((Form_pg_am)
GETSTRUCT(amtuprel))->amname))));

I initially thought switching to ERRCODE_WRONG_OBJECT_TYPE made sense,
but after checking similar cases, I'm reconsidering. For instance,
pgstat_relation() in pgstattuple.c uses ERRCODE_FEATURE_NOT_SUPPORTED
when the access method is unexpected. That suggests using
FEATURE_NOT_SUPPORTED here may not be incorrect.

OK, that seems reasonable to me.

if (!rel->rd_index->indisvalid)
ereport(ERROR,
-
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("cannot check index \"%s\"",

RelationGetRelationName(rel)),
errdetail("Index is not valid.")));

Other parts of the codebase (including pgstattuple) use
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE when rejecting invalid index.
So, changing to this error code still seems reasonable and more
consistent overall.

Given that, I'd like to merge just this error code change from
the 0002 patch into 0003, and apply the combined patch to
the master branch only. Although I briefly considered backpatching
the change of error code, it could be seen as a behavioral change,
and the existing error code doesn't cause any real problem in older
branches. So it's probably better to leave it as-is there.

The updated patch is attached. It also changes index_checkable()
from extern to static, since it's only used within verify_common.c.

Thoughts?

Thanks for updating the patch. I agree with your idea.
BTW, is "witha" a typo in the commit message? Aside from that, I have
no additional comments on your patch.

Regards,
--
Masahiro Ikeda
NTT DATA Japan Corporation

On 2025/07/14 17:13, Masahiro Ikeda wrote:

On 2025-07-04 20:32, Fujii Masao wrote:

On 2025/06/02 16:32, Masahiro Ikeda wrote:

OK, I think v5-0002 should be back-patched, since using incorrect error codes is essentially a bug.

Thanks for updating the patches!

While reviewing the code:

                amtup = SearchSysCache1(AMOID, ObjectIdGetDatum(am_id));
                amtuprel = SearchSysCache1(AMOID,
ObjectIdGetDatum(rel->rd_rel->relam));
                ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                                 errmsg("expected \"%s\" index as
targets for verification", NameStr(((Form_pg_am)
GETSTRUCT(amtup))->amname)),
                                 errdetail("Relation \"%s\" is a %s index.",

RelationGetRelationName(rel), NameStr(((Form_pg_am)
GETSTRUCT(amtuprel))->amname))));

I initially thought switching to ERRCODE_WRONG_OBJECT_TYPE made sense,
but after checking similar cases, I'm reconsidering. For instance,
pgstat_relation() in pgstattuple.c uses ERRCODE_FEATURE_NOT_SUPPORTED
when the access method is unexpected. That suggests using
FEATURE_NOT_SUPPORTED here may not be incorrect.

OK, that seems reasonable to me.

        if (!rel->rd_index->indisvalid)
                ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                                 errmsg("cannot check index \"%s\"",
RelationGetRelationName(rel)),
                                 errdetail("Index is not valid.")));

Other parts of the codebase (including pgstattuple) use
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE when rejecting invalid index.
So, changing to this error code still seems reasonable and more
consistent overall.

Given that, I'd like to merge just this error code change from
the 0002 patch into 0003, and apply the combined patch to
the master branch only. Although I briefly considered backpatching
the change of error code, it could be seen as a behavioral change,
and the existing error code doesn't cause any real problem in older
branches. So it's probably better to leave it as-is there.

The updated patch is attached. It also changes index_checkable()
from extern to static, since it's only used within verify_common.c.

Thoughts?

Thanks for updating the patch. I agree with your idea.
BTW, is "witha" a typo in the commit message? Aside from that, I have
no additional comments on your patch.

Thanks for the review! You're right, that was a typo.
I've fixed it and pushed the patch. Thanks!

Regards,

--
Fujii Masao
NTT DATA Japan Corporation