From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Fujii Masao <masao.fujii@gmail.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Richard Guo <guofenglinux@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
Date: Thu, 29 May 2025 11:46:41 +0900
Message-ID: <dfe167ebf508b9693a7f870b261df22a@oss.nttdata.com> (raw)
In-Reply-To: <591e4384-87c1-46b6-a4f2-144738f24956@oss.nttdata.com>
References: <1cf7a731-024b-4421-8f09-b67b5c25df93@email.android.com>
<591e4384-87c1-46b6-a4f2-144738f24956@oss.nttdata.com>
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:download all (3 files, .tar.gz)v4-0001-Fix-assertion-failure-when-pg_prewarm-is-run-on-o.patch (text/x-diff, 3.7K) download
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/Makefileindex 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.outnew file mode 100644index 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.buildindex 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.cindex 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.sqlnew file mode 100644index 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.patch (text/x-diff, 3.3K) download
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.outindex 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.sqlindex 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.cindex 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.patch (text/x-diff, 2.3K) download
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.cindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ikedamsh@oss.nttdata.com, masao.fujii@oss.nttdata.com, masao.fujii@gmail.com, dilipbalaut@gmail.com, guofenglinux@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables
In-Reply-To: <dfe167ebf508b9693a7f870b261df22a@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox