Обсуждение: 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

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