Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables

On 2025/05/16 15:33, Dilip Kumar wrote:
> On Fri, May 16, 2025 at 11:51 AM Masahiro Ikeda
> <ikedamsh@oss.nttdata.com> wrote:
>>
>> Thank you for your comments!
>>
>> I updated the patch to use RELKIND_HAS_STORAGE() as done in
>> commit 4623d7144. Please see the v2-0001 patch for the changes.

Thanks for updating the patch!

You added a test for pg_prewarm() with a partitioned table in the TAP tests.
But, wouldn't it be simpler and easier to maintain if we added this
as a regular regression test (i.e., using .sql and .out files) instead of
TAP tests? That should be sufficient for this case?

Also, since the issue was introduced in v17, this patch should be
back-patched to v17, right?


>> On 2025-05-15 19:57, Richard Guo wrote:
>>> +1.  FWIW, not long ago we fixed a similar Assert failure in
>>> contrib/pg_freespacemap by verifying RELKIND_HAS_STORAGE() before
>>> trying to access the storage (see 4623d7144).  Wondering if there are
>>> other similar issues elsewhere in contrib ...
>>
>> I tested all contrib functions that take regclass arguments (see
>> attached test.sql and test_result.txt). The result shows that only
>> pg_prewarm() can lead to the assertion failure.
> 
> Right, even while I was searching, I see this is used in 3 contrib
> modules, amcheck, autoprewarm, and pg_prewarm. amcheck is already
> checking for AM type, and Autoprewarm is identifying the relation by
> block, so there is no chance of getting the relation which do not have
> storage.
> 
>>
>> However, I found that amcheck's error messages can be misleading
>> when run on partitioned indexes.
>>
>> For example, on the master branch, amcheck (e.g., bt_index_check
>> function)
>> shows this error:
>>
>>    > ERROR:  expected "btree" index as targets for verification
>>    > DETAIL:  Relation "pgbench_accounts_pkey" is a btree index.
>>
>> This message says it expects a "btree" index, yet also states the
>> relation
>> is a "btree" index, which can seem contradictory. The actual issue is
>> that
>> the index is a btree partitioned index, but this detail is missing,
>> causing
>> possible confusion.
> 
> Yes, I found the error message confusing during testing as well, so it
> makes sense to improve it.

+1

Regarding the patch, how about also adding a regression test for
amcheck with a partitioned index?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION