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: Mon, 02 Jun 2025 16:32:40 +0900
Message-ID: <548d2eb7ae9d1ff4c8cc7e4e3251b8d3@oss.nttdata.com> (raw)
In-Reply-To: <23d5aea0-be92-4837-a5ee-1bdf0ebc49c5@oss.nttdata.com>
References: <1cf7a731-024b-4421-8f09-b67b5c25df93@email.android.com>
<591e4384-87c1-46b6-a4f2-144738f24956@oss.nttdata.com>
<dfe167ebf508b9693a7f870b261df22a@oss.nttdata.com>
<23d5aea0-be92-4837-a5ee-1bdf0ebc49c5@oss.nttdata.com>
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:download all (2 files, .tar.gz)v5-0002-Fix-incorrect-error-code-in-index_checkable.patch (text/x-diff, 1.5K) download
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.cindex 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.patch (text/x-diff, 3.8K) download
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.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 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
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: <548d2eb7ae9d1ff4c8cc7e4e3251b8d3@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