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

On 2025/05/26 16:55, ikedamsh wrote:
> 2025/05/21 12:54 Fujii Masao <masao.fujii@gmail.com>:
> 
>     On Mon, May 19, 2025 at 5:18 PM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote:
>      >
>      > Thanks for your work and feedback!
>      >
>      > I've updated the patches and added regular regression tests for
>      > both pg_prewarm and amcheck.
> 
>     Thanks for updating the patches!
> 
>     Regarding the 0001 patch:
> 
>     +CREATE TABLE test_part1 PARTITION OF test FOR VALUES FROM (1) TO (1000);
>     +INSERT INTO test SELECT generate_series(1, 100);
> 
>     These lines don't seem necessary for the test. How about removing them?
>     It would simplify the test and slightly reduce the execution time - though
>     the time savings are very minimal.
> 
>     +-- To specify the relation which does not have storage should fail.
> 
>     This comment could be clearer as:
>     "pg_prewarm() should fail if the target relation has no storage."
> 
>     + /* Check that the storage exists. */
> 
>     Maybe rephrase to:
>     "Check that the relation has storage." ?
> 
> 
> Thanks! I will fix them.

Thanks!


>     Regarding the 0002 patch:
> 
>     - errdetail("Relation \"%s\" is a %s index.",
>     -    RelationGetRelationName(rel), NameStr(((Form_pg_am)
>     GETSTRUCT(amtuprel))->amname))));
>     + errdetail("Relation \"%s\" is a %s %sindex.",
>     +    RelationGetRelationName(rel), NameStr(((Form_pg_am)
>     GETSTRUCT(amtuprel))->amname),
>     +    (rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX) ?
>     "partitioned " : "")));
> 
>     Instead of manually building the message, how about using
>     errdetail_relkind_not_supported()?
>     It would be more consistent with similar error reporting elsewhere.
> 
> I was thinking of using errdetail_relkind_not_supported(),
> but I’m reconsidering building the message manually
> since the AM name seems to be important for the error.
> What do you think?

Understood.
I was trying to better understand the error message, as I found
the following is still a bit confusing for users. However, I don't
have a better suggestion at the moment, so I'm okay with
the proposed change.

ERROR:  expected "btree" index as targets for verification
DETAIL:  Relation "pgbench_accounts_pkey" is a btree partitioned


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.

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?

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation