Message-ID: <23d5aea0-be92-4837-a5ee-1bdf0ebc49c5@oss.nttdata.com>
Date: Fri, 30 May 2025 01:44:25 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Assertion failure in smgr.c when using pg_prewarm with
 partitioned tables
To: Masahiro Ikeda <ikedamsh@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>
References: <1cf7a731-024b-4421-8f09-b67b5c25df93@email.android.com>
 <591e4384-87c1-46b6-a4f2-144738f24956@oss.nttdata.com>
 <dfe167ebf508b9693a7f870b261df22a@oss.nttdata.com>
Content-Language: en-US
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <dfe167ebf508b9693a7f870b261df22a@oss.nttdata.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/23d5aea0-be92-4837-a5ee-1bdf0ebc49c5%40oss.nttdata.com>
Precedence: bulk


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