MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Mon, 14 Jul 2025 17:13:15 +0900
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Fujii Masao <masao.fujii@gmail.com>, Dilip Kumar
 <dilipbalaut@gmail.com>, Richard Guo <guofenglinux@gmail.com>, PostgreSQL
 Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Assertion failure in smgr.c when using pg_prewarm with
 partitioned tables
In-Reply-To: <1a3170ba-a2d2-4519-be04-6b620f698be3@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>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <e8d98d7a49937083a4894368bce87694@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/e8d98d7a49937083a4894368bce87694%40oss.nttdata.com>
Precedence: bulk

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