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

[ikedamsh]

2025/05/21 12:54 Fujii Masao <masao.fu...@gmail.com>:

On Mon, May 19, 2025 at 5:18 PM Masahiro Ikeda <ikeda...@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.

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?

Regards,

--

Masahiro Ikeda