Assertion failure in smgr.c when using pg_prewarm with partitioned tables
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?
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
Attachments:
v1-0001-Fix-assertion-failure-when-pg_prewarm-is-used-on-.patchtext/x-diff; name=v1-0001-Fix-assertion-failure-when-pg_prewarm-is-used-on-.patchDownload
From 3c27152719f3bbc84d61dc136584f530a75e6da1 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Date: Thu, 15 May 2025 17:26:27 +0900
Subject: [PATCH v1] Fix assertion failure when pg_prewarm is used on a
partitioned table
This issue was introduced by commit 049ef33.
Specifying a partitioned table as an argument to pg_prewarm
could trigger an assertion failure:
Failed Assert("RelFileNumberIsValid(rlocator.relNumber)")
This fix ensures that such cases are handled appropriately.
---
contrib/pg_prewarm/pg_prewarm.c | 3 ++-
contrib/pg_prewarm/t/001_basic.pl | 11 ++++++++++-
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index 50808569bd7..94a36795130 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -113,7 +113,8 @@ pg_prewarm(PG_FUNCTION_ARGS)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
/* Check that the fork exists. */
- if (!smgrexists(RelationGetSmgr(rel), forkNumber))
+ if (!RelFileNumberIsValid(rel->rd_locator.relNumber) ||
+ !smgrexists(RelationGetSmgr(rel), forkNumber))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("fork \"%s\" does not exist for this relation",
diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d3678..57a0134454b 100644
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,10 @@ $node->start;
$node->safe_psql("postgres",
"CREATE EXTENSION pg_prewarm;\n"
. "CREATE TABLE test(c1 int);\n"
- . "INSERT INTO test SELECT generate_series(1, 100);");
+ . "INSERT INTO test SELECT generate_series(1, 100);\n"
+ . "CREATE TABLE test_part(c1 int) PARTITION BY RANGE (c1);\n"
+ . "CREATE TABLE test_part1 PARTITION OF test_part FOR VALUES FROM (1) TO (1000);\n"
+ . "INSERT INTO test_part SELECT generate_series(1, 100);");
# test read mode
my $result =
@@ -42,6 +45,12 @@ ok( ( $stdout =~ qr/^[1-9][0-9]*$/
or $stderr =~ qr/prefetch is not supported by this build/),
'prefetch mode succeeded');
+# test partition table
+($cmdret, $stdout, $stderr) =
+ $node->psql("postgres", "SELECT pg_prewarm('test_part', 'buffer');");
+ok( $stderr =~ /fork "main" does not exist for this relation/,
+ 'detected the fork does not exist');
+
# test autoprewarm_dump_now()
$result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');
--
2.34.1
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');
EOFThe 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');
EOFThe 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');
EOFThe 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
Attachments:
v2-0001-Fix-assertion-failure-when-pg_prewarm-is-run-on-o.patchtext/x-diff; name=v2-0001-Fix-assertion-failure-when-pg_prewarm-is-run-on-o.patchDownload
From db240142ab4de334ef072f6af59a792701e806c5 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Date: Fri, 16 May 2025 14:35:32 +0900
Subject: [PATCH v2 1/2] Fix assertion failure when pg_prewarm() is run on
objects that don't have storage.
This issue was introduced by commit 049ef33.
Specifying objects that don't have storage as an argument to
pg_prewarm() could trigger an assertion failure:
Failed Assert("RelFileNumberIsValid(rlocator.relNumber)")
This fix ensures that such cases are handled appropriately.
---
contrib/pg_prewarm/pg_prewarm.c | 8 ++++++++
contrib/pg_prewarm/t/001_basic.pl | 11 ++++++++++-
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index 50808569bd7..1f89c9adc86 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -112,6 +112,14 @@ pg_prewarm(PG_FUNCTION_ARGS)
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
+ /* Check that the storage exists. */
+ if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("relation \"%s\" does not have storage",
+ RelationGetRelationName(rel)),
+ errdetail_relkind_not_supported(rel->rd_rel->relkind)));
+
/* Check that the fork exists. */
if (!smgrexists(RelationGetSmgr(rel), forkNumber))
ereport(ERROR,
diff --git a/contrib/pg_prewarm/t/001_basic.pl b/contrib/pg_prewarm/t/001_basic.pl
index 0a8259d3678..f7c31ae60a8 100644
--- a/contrib/pg_prewarm/t/001_basic.pl
+++ b/contrib/pg_prewarm/t/001_basic.pl
@@ -23,7 +23,10 @@ $node->start;
$node->safe_psql("postgres",
"CREATE EXTENSION pg_prewarm;\n"
. "CREATE TABLE test(c1 int);\n"
- . "INSERT INTO test SELECT generate_series(1, 100);");
+ . "INSERT INTO test SELECT generate_series(1, 100);\n"
+ . "CREATE TABLE test_part(c1 int) PARTITION BY RANGE (c1);\n"
+ . "CREATE TABLE test_part1 PARTITION OF test_part FOR VALUES FROM (1) TO (1000);\n"
+ . "INSERT INTO test_part SELECT generate_series(1, 100);");
# test read mode
my $result =
@@ -42,6 +45,12 @@ ok( ( $stdout =~ qr/^[1-9][0-9]*$/
or $stderr =~ qr/prefetch is not supported by this build/),
'prefetch mode succeeded');
+# test partition table
+($cmdret, $stdout, $stderr) =
+ $node->psql("postgres", "SELECT pg_prewarm('test_part', 'buffer');");
+ok( $stderr =~ /relation "test_part" does not have storage/,
+ 'detected storage does not exist');
+
# test autoprewarm_dump_now()
$result = $node->safe_psql("postgres", "SELECT autoprewarm_dump_now();");
like($result, qr/^[1-9][0-9]*$/, 'autoprewarm_dump_now succeeded');
--
2.34.1
v2-0002-Fix-error-message-details-for-partitioned-indexes.patchtext/x-diff; name=v2-0002-Fix-error-message-details-for-partitioned-indexes.patchDownload
From 83f92645ff1f934e7960534c32ad2d67b2f0b910 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Date: Fri, 16 May 2025 14:41:27 +0900
Subject: [PATCH v2 2/2] Fix error message details for partitioned indexes in
amcheck
Until now, the error detail could be misleading when bt_index_check()
is run on partitioned indexes. For example, the index
"pgbench_accounts_pkey" is both a btree and a partitioned index.
Before this change, the error message was:
> ERROR: expected "btree" index as targets for verification
> DETAIL: Relation "pgbench_accounts_pkey" is a btree index.
This change adds the word "partitioned" to the error detail to avoid
confusion about why the error occurred.
After this change, the error message becomes:
> ERROR: expected "btree" index as targets for verification
> DETAIL: Relation "pgbench_accounts_pkey" is a btree partitioned index.
---
contrib/amcheck/verify_common.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/contrib/amcheck/verify_common.c b/contrib/amcheck/verify_common.c
index d095e62ce55..b68ddaf895d 100644
--- a/contrib/amcheck/verify_common.c
+++ b/contrib/amcheck/verify_common.c
@@ -169,8 +169,9 @@ index_checkable(Relation rel, Oid am_id)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
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))));
+ errdetail("Relation \"%s\" is a %s %sindex.",
+ RelationGetRelationName(rel), NameStr(((Form_pg_am) GETSTRUCT(amtuprel))->amname),
+ (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX) ? "partitioned " : "")));
}
if (RELATION_IS_OTHER_TEMP(rel))
--
2.34.1
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
Attachments:
v3-0001-Fix-assertion-failure-when-pg_prewarm-is-run-on-o.patchtext/x-diff; name=v3-0001-Fix-assertion-failure-when-pg_prewarm-is-run-on-o.patchDownload
From 16027ecedc4301904965306d8a495f7a08c6d798 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Date: Mon, 19 May 2025 16:24:55 +0900
Subject: [PATCH v3 1/2] Fix assertion failure when pg_prewarm() is run on
objects that don't have storage.
This issue was introduced by commit 049ef33.
Specifying objects that don't have storage as an argument to
pg_prewarm() could trigger an assertion failure:
Failed Assert("RelFileNumberIsValid(rlocator.relNumber)")
This fix ensures that such cases are handled appropriately.
---
contrib/pg_prewarm/Makefile | 2 ++
contrib/pg_prewarm/expected/pg_prewarm.out | 12 ++++++++++++
contrib/pg_prewarm/meson.build | 5 +++++
contrib/pg_prewarm/pg_prewarm.c | 8 ++++++++
contrib/pg_prewarm/sql/pg_prewarm.sql | 12 ++++++++++++
5 files changed, 39 insertions(+)
create mode 100644 contrib/pg_prewarm/expected/pg_prewarm.out
create mode 100644 contrib/pg_prewarm/sql/pg_prewarm.sql
diff --git a/contrib/pg_prewarm/Makefile b/contrib/pg_prewarm/Makefile
index 9cfde8c4e4f..617ac8e09b2 100644
--- a/contrib/pg_prewarm/Makefile
+++ b/contrib/pg_prewarm/Makefile
@@ -10,6 +10,8 @@ EXTENSION = pg_prewarm
DATA = pg_prewarm--1.1--1.2.sql pg_prewarm--1.1.sql pg_prewarm--1.0--1.1.sql
PGFILEDESC = "pg_prewarm - preload relation data into system buffer cache"
+REGRESS = pg_prewarm
+
TAP_TESTS = 1
ifdef USE_PGXS
diff --git a/contrib/pg_prewarm/expected/pg_prewarm.out b/contrib/pg_prewarm/expected/pg_prewarm.out
new file mode 100644
index 00000000000..3bb644499d4
--- /dev/null
+++ b/contrib/pg_prewarm/expected/pg_prewarm.out
@@ -0,0 +1,12 @@
+-- Test pg_prewarm extension
+CREATE EXTENSION pg_prewarm;
+-- To specify the relation which does not have storage should fail.
+CREATE TABLE test (c1 int) PARTITION BY RANGE (c1);
+CREATE TABLE test_part1 PARTITION OF test FOR VALUES FROM (1) TO (1000);
+INSERT INTO test SELECT generate_series(1, 100);
+SELECT pg_prewarm('test', 'buffer');
+ERROR: relation "test" does not have storage
+DETAIL: This operation is not supported for partitioned tables.
+-- Cleanup
+DROP TABLE test;
+DROP EXTENSION pg_prewarm;
diff --git a/contrib/pg_prewarm/meson.build b/contrib/pg_prewarm/meson.build
index 82b9851303c..f24c47ef6a5 100644
--- a/contrib/pg_prewarm/meson.build
+++ b/contrib/pg_prewarm/meson.build
@@ -29,6 +29,11 @@ tests += {
'name': 'pg_prewarm',
'sd': meson.current_source_dir(),
'bd': meson.current_build_dir(),
+ 'regress': {
+ 'sql': [
+ 'pg_prewarm',
+ ],
+ },
'tap': {
'tests': [
't/001_basic.pl',
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index 50808569bd7..1f89c9adc86 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -112,6 +112,14 @@ pg_prewarm(PG_FUNCTION_ARGS)
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
+ /* Check that the storage exists. */
+ if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("relation \"%s\" does not have storage",
+ RelationGetRelationName(rel)),
+ errdetail_relkind_not_supported(rel->rd_rel->relkind)));
+
/* Check that the fork exists. */
if (!smgrexists(RelationGetSmgr(rel), forkNumber))
ereport(ERROR,
diff --git a/contrib/pg_prewarm/sql/pg_prewarm.sql b/contrib/pg_prewarm/sql/pg_prewarm.sql
new file mode 100644
index 00000000000..2743fa5e778
--- /dev/null
+++ b/contrib/pg_prewarm/sql/pg_prewarm.sql
@@ -0,0 +1,12 @@
+-- Test pg_prewarm extension
+CREATE EXTENSION pg_prewarm;
+
+-- To specify the relation which does not have storage should fail.
+CREATE TABLE test (c1 int) PARTITION BY RANGE (c1);
+CREATE TABLE test_part1 PARTITION OF test FOR VALUES FROM (1) TO (1000);
+INSERT INTO test SELECT generate_series(1, 100);
+SELECT pg_prewarm('test', 'buffer');
+
+-- Cleanup
+DROP TABLE test;
+DROP EXTENSION pg_prewarm;
--
2.34.1
v3-0002-Fix-error-message-details-for-partitioned-indexes.patchtext/x-diff; name=v3-0002-Fix-error-message-details-for-partitioned-indexes.patchDownload
From 500945b94f053c1230830e1211a727edc6e1f01b Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Date: Mon, 19 May 2025 16:39:27 +0900
Subject: [PATCH v3 2/2] Fix error message details for partitioned indexes in
amcheck
Until now, the error detail could be misleading when bt_index_check()
is run on partitioned indexes. For example, the index
"pgbench_accounts_pkey" is both a btree and a partitioned index.
Before this change, the error message was:
> ERROR: expected "btree" index as targets for verification
> DETAIL: Relation "pgbench_accounts_pkey" is a btree index.
This change adds the word "partitioned" to the error detail to avoid
confusion about why the error occurred.
After this change, the error message becomes:
> ERROR: expected "btree" index as targets for verification
> DETAIL: Relation "pgbench_accounts_pkey" is a btree partitioned index.
---
contrib/amcheck/expected/check_btree.out | 8 ++++++++
contrib/amcheck/sql/check_btree.sql | 7 +++++++
contrib/amcheck/verify_common.c | 5 +++--
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/contrib/amcheck/expected/check_btree.out b/contrib/amcheck/expected/check_btree.out
index c6f4b16c556..8947496d890 100644
--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out
@@ -60,6 +60,14 @@ SELECT bt_index_parent_check('bttest_a_brin_idx');
ERROR: expected "btree" index as targets for verification
DETAIL: Relation "bttest_a_brin_idx" is a brin index.
ROLLBACK;
+-- verify btree partitioned indexes are rejected (error)
+BEGIN;
+CREATE TABLE bttest_partitioned (a int, b int) PARTITION BY list (a);
+CREATE INDEX bttest_btree_partitioned_idx ON bttest_partitioned USING btree (b);
+SELECT bt_index_parent_check('bttest_btree_partitioned_idx');
+ERROR: expected "btree" index as targets for verification
+DETAIL: Relation "bttest_btree_partitioned_idx" is a btree partitioned index.
+ROLLBACK;
-- normal check outside of xact
SELECT bt_index_check('bttest_a_idx');
bt_index_check
diff --git a/contrib/amcheck/sql/check_btree.sql b/contrib/amcheck/sql/check_btree.sql
index 0793dbfeebd..07750084b3a 100644
--- a/contrib/amcheck/sql/check_btree.sql
+++ b/contrib/amcheck/sql/check_btree.sql
@@ -52,6 +52,13 @@ CREATE INDEX bttest_a_brin_idx ON bttest_a USING brin(id);
SELECT bt_index_parent_check('bttest_a_brin_idx');
ROLLBACK;
+-- verify btree partitioned indexes are rejected (error)
+BEGIN;
+CREATE TABLE bttest_partitioned (a int, b int) PARTITION BY list (a);
+CREATE INDEX bttest_btree_partitioned_idx ON bttest_partitioned USING btree (b);
+SELECT bt_index_parent_check('bttest_btree_partitioned_idx');
+ROLLBACK;
+
-- normal check outside of xact
SELECT bt_index_check('bttest_a_idx');
-- more expansive tests
diff --git a/contrib/amcheck/verify_common.c b/contrib/amcheck/verify_common.c
index d095e62ce55..b68ddaf895d 100644
--- a/contrib/amcheck/verify_common.c
+++ b/contrib/amcheck/verify_common.c
@@ -169,8 +169,9 @@ index_checkable(Relation rel, Oid am_id)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
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))));
+ errdetail("Relation \"%s\" is a %s %sindex.",
+ RelationGetRelationName(rel), NameStr(((Form_pg_am) GETSTRUCT(amtuprel))->amname),
+ (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX) ? "partitioned " : "")));
}
if (RELATION_IS_OTHER_TEMP(rel))
--
2.34.1
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 <masao.fujii@gmail.com>:<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 <ikedamsh@oss.nttdata.com> wrote:
<br>
>
<br>
> Thanks for your work and feedback!
<br>
>
<br>
> I've updated the patches and added regular regression tests for
<br>
> 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>
- RelationGetRelationName(rel), NameStr(((Form_pg_am)
<br>
GETSTRUCT(amtuprel))->amname))));
<br>
+ errdetail("Relation \"%s\" is a %s %sindex.",
<br>
+ RelationGetRelationName(rel), NameStr(((Form_pg_am)
<br>
GETSTRUCT(amtuprel))->amname),
<br>
+ (rel->rd_rel->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
Attachments:
v4-0001-Fix-assertion-failure-when-pg_prewarm-is-run-on-o.patchtext/x-diff; name=v4-0001-Fix-assertion-failure-when-pg_prewarm-is-run-on-o.patchDownload
From c1c7c06a1d2fd7f39a22a81679bfbefb5e0b1911 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Date: Mon, 26 May 2025 16:25:42 +0900
Subject: [PATCH v4 1/3] Fix assertion failure when pg_prewarm() is run on
objects that don't have storage.
This issue was introduced by commit 049ef33.
Specifying objects that don't have storage as an argument to
pg_prewarm() could trigger an assertion failure:
Failed Assert("RelFileNumberIsValid(rlocator.relNumber)")
This fix ensures that such cases are handled appropriately.
---
contrib/pg_prewarm/Makefile | 2 ++
contrib/pg_prewarm/expected/pg_prewarm.out | 10 ++++++++++
contrib/pg_prewarm/meson.build | 5 +++++
contrib/pg_prewarm/pg_prewarm.c | 8 ++++++++
contrib/pg_prewarm/sql/pg_prewarm.sql | 10 ++++++++++
5 files changed, 35 insertions(+)
create mode 100644 contrib/pg_prewarm/expected/pg_prewarm.out
create mode 100644 contrib/pg_prewarm/sql/pg_prewarm.sql
diff --git a/contrib/pg_prewarm/Makefile b/contrib/pg_prewarm/Makefile
index 9cfde8c4e4f..617ac8e09b2 100644
--- a/contrib/pg_prewarm/Makefile
+++ b/contrib/pg_prewarm/Makefile
@@ -10,6 +10,8 @@ EXTENSION = pg_prewarm
DATA = pg_prewarm--1.1--1.2.sql pg_prewarm--1.1.sql pg_prewarm--1.0--1.1.sql
PGFILEDESC = "pg_prewarm - preload relation data into system buffer cache"
+REGRESS = pg_prewarm
+
TAP_TESTS = 1
ifdef USE_PGXS
diff --git a/contrib/pg_prewarm/expected/pg_prewarm.out b/contrib/pg_prewarm/expected/pg_prewarm.out
new file mode 100644
index 00000000000..94e4fa1a9d2
--- /dev/null
+++ b/contrib/pg_prewarm/expected/pg_prewarm.out
@@ -0,0 +1,10 @@
+-- Test pg_prewarm extension
+CREATE EXTENSION pg_prewarm;
+-- pg_prewarm() should fail if the target relation has no storage.
+CREATE TABLE test (c1 int) PARTITION BY RANGE (c1);
+SELECT pg_prewarm('test', 'buffer');
+ERROR: relation "test" does not have storage
+DETAIL: This operation is not supported for partitioned tables.
+-- Cleanup
+DROP TABLE test;
+DROP EXTENSION pg_prewarm;
diff --git a/contrib/pg_prewarm/meson.build b/contrib/pg_prewarm/meson.build
index 82b9851303c..f24c47ef6a5 100644
--- a/contrib/pg_prewarm/meson.build
+++ b/contrib/pg_prewarm/meson.build
@@ -29,6 +29,11 @@ tests += {
'name': 'pg_prewarm',
'sd': meson.current_source_dir(),
'bd': meson.current_build_dir(),
+ 'regress': {
+ 'sql': [
+ 'pg_prewarm',
+ ],
+ },
'tap': {
'tests': [
't/001_basic.pl',
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index 50808569bd7..b968933ea8b 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -112,6 +112,14 @@ pg_prewarm(PG_FUNCTION_ARGS)
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind), get_rel_name(relOid));
+ /* Check that the relation has storage. */
+ if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("relation \"%s\" does not have storage",
+ RelationGetRelationName(rel)),
+ errdetail_relkind_not_supported(rel->rd_rel->relkind)));
+
/* Check that the fork exists. */
if (!smgrexists(RelationGetSmgr(rel), forkNumber))
ereport(ERROR,
diff --git a/contrib/pg_prewarm/sql/pg_prewarm.sql b/contrib/pg_prewarm/sql/pg_prewarm.sql
new file mode 100644
index 00000000000..c76f2c79164
--- /dev/null
+++ b/contrib/pg_prewarm/sql/pg_prewarm.sql
@@ -0,0 +1,10 @@
+-- Test pg_prewarm extension
+CREATE EXTENSION pg_prewarm;
+
+-- pg_prewarm() should fail if the target relation has no storage.
+CREATE TABLE test (c1 int) PARTITION BY RANGE (c1);
+SELECT pg_prewarm('test', 'buffer');
+
+-- Cleanup
+DROP TABLE test;
+DROP EXTENSION pg_prewarm;
--
2.34.1
v4-0002-Fix-error-message-for-partitioned-indexes-in-amch.patchtext/x-diff; name=v4-0002-Fix-error-message-for-partitioned-indexes-in-amch.patchDownload
From 70166aeecc0b096a106a4f9264e28d75468d7167 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Date: Thu, 29 May 2025 10:33:56 +0900
Subject: [PATCH v4 2/3] Fix error message for partitioned indexes in amcheck
Until now, the error detail could be misleading when to
verify partitioned indexes.
For example, when bt_index_check() is run on partitioned
indexes, the error messages are:
> ERROR: expected "btree" index as targets for verification
> DETAIL: Relation "pgbench_accounts_pkey" is a btree index.
This change adds to check whether it's partitioned indexes
first to avoid confusion about why the error occurred.
After this change, the error message becomes:
> ERROR: expected index as targets for verification
> DETAIL: This operation is not supported for partitioned indexes.
---
contrib/amcheck/expected/check_btree.out | 8 ++++++++
contrib/amcheck/sql/check_btree.sql | 7 +++++++
contrib/amcheck/verify_common.c | 9 +++++++--
3 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/contrib/amcheck/expected/check_btree.out b/contrib/amcheck/expected/check_btree.out
index c6f4b16c556..6558f2c5a4f 100644
--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out
@@ -60,6 +60,14 @@ SELECT bt_index_parent_check('bttest_a_brin_idx');
ERROR: expected "btree" index as targets for verification
DETAIL: Relation "bttest_a_brin_idx" is a brin index.
ROLLBACK;
+-- verify partitioned indexes are rejected (error)
+BEGIN;
+CREATE TABLE bttest_partitioned (a int, b int) PARTITION BY list (a);
+CREATE INDEX bttest_btree_partitioned_idx ON bttest_partitioned USING btree (b);
+SELECT bt_index_parent_check('bttest_btree_partitioned_idx');
+ERROR: expected index as targets for verification
+DETAIL: This operation is not supported for partitioned indexes.
+ROLLBACK;
-- normal check outside of xact
SELECT bt_index_check('bttest_a_idx');
bt_index_check
diff --git a/contrib/amcheck/sql/check_btree.sql b/contrib/amcheck/sql/check_btree.sql
index 0793dbfeebd..171f7f691ec 100644
--- a/contrib/amcheck/sql/check_btree.sql
+++ b/contrib/amcheck/sql/check_btree.sql
@@ -52,6 +52,13 @@ CREATE INDEX bttest_a_brin_idx ON bttest_a USING brin(id);
SELECT bt_index_parent_check('bttest_a_brin_idx');
ROLLBACK;
+-- verify partitioned indexes are rejected (error)
+BEGIN;
+CREATE TABLE bttest_partitioned (a int, b int) PARTITION BY list (a);
+CREATE INDEX bttest_btree_partitioned_idx ON bttest_partitioned USING btree (b);
+SELECT bt_index_parent_check('bttest_btree_partitioned_idx');
+ROLLBACK;
+
-- normal check outside of xact
SELECT bt_index_check('bttest_a_idx');
-- more expansive tests
diff --git a/contrib/amcheck/verify_common.c b/contrib/amcheck/verify_common.c
index d095e62ce55..cbbc8420f96 100644
--- a/contrib/amcheck/verify_common.c
+++ b/contrib/amcheck/verify_common.c
@@ -158,8 +158,13 @@ amcheck_lock_relation_and_check(Oid indrelid,
bool
index_checkable(Relation rel, Oid am_id)
{
- if (rel->rd_rel->relkind != RELKIND_INDEX ||
- rel->rd_rel->relam != am_id)
+ if (rel->rd_rel->relkind != RELKIND_INDEX)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("expected index as targets for verification"),
+ errdetail_relkind_not_supported(rel->rd_rel->relkind)));
+
+ if (rel->rd_rel->relam != am_id)
{
HeapTuple amtup;
HeapTuple amtuprel;
--
2.34.1
v4-0003-Improve-index_checkable-in-amcheck.patchtext/x-diff; name=v4-0003-Improve-index_checkable-in-amcheck.patchDownload
From 1215ad6ed96501130fb650141de69ed3fb339c94 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Date: Thu, 29 May 2025 10:59:04 +0900
Subject: [PATCH v4 3/3] Improve index_checkable() in amcheck
Refactor the function and use more appropriate error codes.
---
contrib/amcheck/verify_common.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/contrib/amcheck/verify_common.c b/contrib/amcheck/verify_common.c
index cbbc8420f96..22dda9afddd 100644
--- a/contrib/amcheck/verify_common.c
+++ b/contrib/amcheck/verify_common.c
@@ -18,6 +18,7 @@
#include "verify_common.h"
#include "catalog/index.h"
#include "catalog/pg_am.h"
+#include "commands/defrem.h"
#include "commands/tablecmds.h"
#include "utils/guc.h"
#include "utils/syscache.h"
@@ -160,23 +161,16 @@ index_checkable(Relation rel, Oid am_id)
{
if (rel->rd_rel->relkind != RELKIND_INDEX)
ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("expected index as targets for verification"),
errdetail_relkind_not_supported(rel->rd_rel->relkind)));
if (rel->rd_rel->relam != am_id)
- {
- HeapTuple amtup;
- HeapTuple amtuprel;
-
- amtup = SearchSysCache1(AMOID, ObjectIdGetDatum(am_id));
- amtuprel = SearchSysCache1(AMOID, ObjectIdGetDatum(rel->rd_rel->relam));
ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("expected \"%s\" index as targets for verification", NameStr(((Form_pg_am) GETSTRUCT(amtup))->amname)),
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("expected \"%s\" index as targets for verification", get_am_name(am_id)),
errdetail("Relation \"%s\" is a %s index.",
- RelationGetRelationName(rel), NameStr(((Form_pg_am) GETSTRUCT(amtuprel))->amname))));
- }
+ RelationGetRelationName(rel), get_am_name(rel->rd_rel->relam))));
if (RELATION_IS_OTHER_TEMP(rel))
ereport(ERROR,
@@ -187,7 +181,7 @@ index_checkable(Relation rel, Oid am_id)
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.")));
--
2.34.1
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
Attachments:
v5-0002-Fix-incorrect-error-code-in-index_checkable.patchtext/x-diff; name=v5-0002-Fix-incorrect-error-code-in-index_checkable.patchDownload
From 4ccb2dc947270073f67481e73862bb5ed8c40c03 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Date: Mon, 2 Jun 2025 16:02:17 +0900
Subject: [PATCH v5 1/2] Fix incorrect error code in index_checkable().
ERRCODE_FEATURE_NOT_SUPPORTED was used for object type
error and invalid indexes error, use proper error codes
instead.
---
contrib/amcheck/verify_common.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/contrib/amcheck/verify_common.c b/contrib/amcheck/verify_common.c
index d095e62ce55..54f9c230a3e 100644
--- a/contrib/amcheck/verify_common.c
+++ b/contrib/amcheck/verify_common.c
@@ -167,7 +167,7 @@ index_checkable(Relation rel, Oid am_id)
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))));
@@ -182,7 +182,7 @@ index_checkable(Relation rel, Oid am_id)
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.")));
--
2.34.1
v5-0003-Refactor-and-improve-error-messages-in-index_chec.patchtext/x-diff; name=v5-0003-Refactor-and-improve-error-messages-in-index_chec.patchDownload
From f53992783e1b8e5915bd076eeeb071ef98051aba Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Date: Mon, 2 Jun 2025 16:07:22 +0900
Subject: [PATCH v5 2/2] Refactor and improve error messages in
index_checkable().
Refactor the function to use get_am_name() and improve error
messages for partitioned indexes.
---
contrib/amcheck/expected/check_btree.out | 8 ++++++++
contrib/amcheck/sql/check_btree.sql | 7 +++++++
contrib/amcheck/verify_common.c | 19 +++++++++----------
3 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/contrib/amcheck/expected/check_btree.out b/contrib/amcheck/expected/check_btree.out
index c6f4b16c556..6558f2c5a4f 100644
--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out
@@ -60,6 +60,14 @@ SELECT bt_index_parent_check('bttest_a_brin_idx');
ERROR: expected "btree" index as targets for verification
DETAIL: Relation "bttest_a_brin_idx" is a brin index.
ROLLBACK;
+-- verify partitioned indexes are rejected (error)
+BEGIN;
+CREATE TABLE bttest_partitioned (a int, b int) PARTITION BY list (a);
+CREATE INDEX bttest_btree_partitioned_idx ON bttest_partitioned USING btree (b);
+SELECT bt_index_parent_check('bttest_btree_partitioned_idx');
+ERROR: expected index as targets for verification
+DETAIL: This operation is not supported for partitioned indexes.
+ROLLBACK;
-- normal check outside of xact
SELECT bt_index_check('bttest_a_idx');
bt_index_check
diff --git a/contrib/amcheck/sql/check_btree.sql b/contrib/amcheck/sql/check_btree.sql
index 0793dbfeebd..171f7f691ec 100644
--- a/contrib/amcheck/sql/check_btree.sql
+++ b/contrib/amcheck/sql/check_btree.sql
@@ -52,6 +52,13 @@ CREATE INDEX bttest_a_brin_idx ON bttest_a USING brin(id);
SELECT bt_index_parent_check('bttest_a_brin_idx');
ROLLBACK;
+-- verify partitioned indexes are rejected (error)
+BEGIN;
+CREATE TABLE bttest_partitioned (a int, b int) PARTITION BY list (a);
+CREATE INDEX bttest_btree_partitioned_idx ON bttest_partitioned USING btree (b);
+SELECT bt_index_parent_check('bttest_btree_partitioned_idx');
+ROLLBACK;
+
-- normal check outside of xact
SELECT bt_index_check('bttest_a_idx');
-- more expansive tests
diff --git a/contrib/amcheck/verify_common.c b/contrib/amcheck/verify_common.c
index 54f9c230a3e..22dda9afddd 100644
--- a/contrib/amcheck/verify_common.c
+++ b/contrib/amcheck/verify_common.c
@@ -18,6 +18,7 @@
#include "verify_common.h"
#include "catalog/index.h"
#include "catalog/pg_am.h"
+#include "commands/defrem.h"
#include "commands/tablecmds.h"
#include "utils/guc.h"
#include "utils/syscache.h"
@@ -158,20 +159,18 @@ amcheck_lock_relation_and_check(Oid indrelid,
bool
index_checkable(Relation rel, Oid am_id)
{
- if (rel->rd_rel->relkind != RELKIND_INDEX ||
- rel->rd_rel->relam != am_id)
- {
- HeapTuple amtup;
- HeapTuple amtuprel;
+ if (rel->rd_rel->relkind != RELKIND_INDEX)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("expected index as targets for verification"),
+ errdetail_relkind_not_supported(rel->rd_rel->relkind)));
- amtup = SearchSysCache1(AMOID, ObjectIdGetDatum(am_id));
- amtuprel = SearchSysCache1(AMOID, ObjectIdGetDatum(rel->rd_rel->relam));
+ if (rel->rd_rel->relam != am_id)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("expected \"%s\" index as targets for verification", NameStr(((Form_pg_am) GETSTRUCT(amtup))->amname)),
+ errmsg("expected \"%s\" index as targets for verification", get_am_name(am_id)),
errdetail("Relation \"%s\" is a %s index.",
- RelationGetRelationName(rel), NameStr(((Form_pg_am) GETSTRUCT(amtuprel))->amname))));
- }
+ RelationGetRelationName(rel), get_am_name(rel->rd_rel->relam))));
if (RELATION_IS_OTHER_TEMP(rel))
ereport(ERROR,
--
2.34.1
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
Attachments:
v6-0001-amcheck-Improve-error-message-for-partitioned-ind.patchtext/plain; charset=UTF-8; name=v6-0001-amcheck-Improve-error-message-for-partitioned-ind.patchDownload
From e8f158d36bad49966f2843a908a0a270bd441357 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Fri, 4 Jul 2025 14:14:05 +0900
Subject: [PATCH v6] amcheck: Improve error message for partitioned index
target.
Previously, amcheck could produce misleading error message when
a partitioned index was passed to functions like bt_index_check().
For example, bt_index_check() with a partitioned btree index produced:
ERROR: expected "btree" index as targets for verification
DETAIL: Relation ... is a btree index.
Reporting "expected btree index as targets" even when the specified
index was a btree was confusing. In this case, the function should fail
since the partitioned index specified is not valid target. This commit
improves the error reporting to better reflect this actual issue. Now,
bt_index_check() witha partitioned index, the error message is:
ERROR: expected index as targets for verification
DETAIL: This operation is not supported for partitioned indexes.
This commit also applies the following minor changes:
- Simplifies index_checkable() by using get_am_name() to retrieve
the access method name.
- Changes index_checkable() from extern to static, as it is only used
in verify_common.c.
- Updates the error code for invalid indexes to
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE,
aligning with usage in similar modules like pgstattuple.
Author: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/8829854bbfc8635ddecd0846bb72dfda@oss.nttdata.com
---
contrib/amcheck/expected/check_btree.out | 8 ++++++++
contrib/amcheck/sql/check_btree.sql | 7 +++++++
contrib/amcheck/verify_common.c | 22 +++++++++++-----------
contrib/amcheck/verify_common.h | 2 --
4 files changed, 26 insertions(+), 13 deletions(-)
diff --git a/contrib/amcheck/expected/check_btree.out b/contrib/amcheck/expected/check_btree.out
index c6f4b16c556..6558f2c5a4f 100644
--- a/contrib/amcheck/expected/check_btree.out
+++ b/contrib/amcheck/expected/check_btree.out
@@ -60,6 +60,14 @@ SELECT bt_index_parent_check('bttest_a_brin_idx');
ERROR: expected "btree" index as targets for verification
DETAIL: Relation "bttest_a_brin_idx" is a brin index.
ROLLBACK;
+-- verify partitioned indexes are rejected (error)
+BEGIN;
+CREATE TABLE bttest_partitioned (a int, b int) PARTITION BY list (a);
+CREATE INDEX bttest_btree_partitioned_idx ON bttest_partitioned USING btree (b);
+SELECT bt_index_parent_check('bttest_btree_partitioned_idx');
+ERROR: expected index as targets for verification
+DETAIL: This operation is not supported for partitioned indexes.
+ROLLBACK;
-- normal check outside of xact
SELECT bt_index_check('bttest_a_idx');
bt_index_check
diff --git a/contrib/amcheck/sql/check_btree.sql b/contrib/amcheck/sql/check_btree.sql
index 0793dbfeebd..171f7f691ec 100644
--- a/contrib/amcheck/sql/check_btree.sql
+++ b/contrib/amcheck/sql/check_btree.sql
@@ -52,6 +52,13 @@ CREATE INDEX bttest_a_brin_idx ON bttest_a USING brin(id);
SELECT bt_index_parent_check('bttest_a_brin_idx');
ROLLBACK;
+-- verify partitioned indexes are rejected (error)
+BEGIN;
+CREATE TABLE bttest_partitioned (a int, b int) PARTITION BY list (a);
+CREATE INDEX bttest_btree_partitioned_idx ON bttest_partitioned USING btree (b);
+SELECT bt_index_parent_check('bttest_btree_partitioned_idx');
+ROLLBACK;
+
-- normal check outside of xact
SELECT bt_index_check('bttest_a_idx');
-- more expansive tests
diff --git a/contrib/amcheck/verify_common.c b/contrib/amcheck/verify_common.c
index d095e62ce55..7e9c5eda1bd 100644
--- a/contrib/amcheck/verify_common.c
+++ b/contrib/amcheck/verify_common.c
@@ -18,11 +18,13 @@
#include "verify_common.h"
#include "catalog/index.h"
#include "catalog/pg_am.h"
+#include "commands/defrem.h"
#include "commands/tablecmds.h"
#include "utils/guc.h"
#include "utils/syscache.h"
static bool amcheck_index_mainfork_expected(Relation rel);
+static bool index_checkable(Relation rel, Oid am_id);
/*
@@ -158,20 +160,18 @@ amcheck_lock_relation_and_check(Oid indrelid,
bool
index_checkable(Relation rel, Oid am_id)
{
- if (rel->rd_rel->relkind != RELKIND_INDEX ||
- rel->rd_rel->relam != am_id)
- {
- HeapTuple amtup;
- HeapTuple amtuprel;
+ if (rel->rd_rel->relkind != RELKIND_INDEX)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("expected index as targets for verification"),
+ errdetail_relkind_not_supported(rel->rd_rel->relkind)));
- amtup = SearchSysCache1(AMOID, ObjectIdGetDatum(am_id));
- amtuprel = SearchSysCache1(AMOID, ObjectIdGetDatum(rel->rd_rel->relam));
+ if (rel->rd_rel->relam != am_id)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("expected \"%s\" index as targets for verification", NameStr(((Form_pg_am) GETSTRUCT(amtup))->amname)),
+ errmsg("expected \"%s\" index as targets for verification", get_am_name(am_id)),
errdetail("Relation \"%s\" is a %s index.",
- RelationGetRelationName(rel), NameStr(((Form_pg_am) GETSTRUCT(amtuprel))->amname))));
- }
+ RelationGetRelationName(rel), get_am_name(rel->rd_rel->relam))));
if (RELATION_IS_OTHER_TEMP(rel))
ereport(ERROR,
@@ -182,7 +182,7 @@ index_checkable(Relation rel, Oid am_id)
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.")));
diff --git a/contrib/amcheck/verify_common.h b/contrib/amcheck/verify_common.h
index e78adb68808..7e56a2d3c4a 100644
--- a/contrib/amcheck/verify_common.h
+++ b/contrib/amcheck/verify_common.h
@@ -27,5 +27,3 @@ extern void amcheck_lock_relation_and_check(Oid indrelid,
Oid am_id,
IndexDoCheckCallback check,
LOCKMODE lockmode, void *state);
-
-extern bool index_checkable(Relation rel, Oid am_id);
--
2.49.0
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