From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Masahiro Ikeda <ikedamsh@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, 14 Jul 2025 20:10:40 +0900
Message-ID: <fe77b7b3-b64f-4f53-9c2e-2a8e9db5e836@oss.nttdata.com> (raw)
In-Reply-To: <e8d98d7a49937083a4894368bce87694@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>
<548d2eb7ae9d1ff4c8cc7e4e3251b8d3@oss.nttdata.com>
<1a3170ba-a2d2-4519-be04-6b620f698be3@oss.nttdata.com>
<e8d98d7a49937083a4894368bce87694@oss.nttdata.com>
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
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: masao.fujii@oss.nttdata.com, ikedamsh@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: <fe77b7b3-b64f-4f53-9c2e-2a8e9db5e836@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