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

On Mon, May 19, 2025 at 5:18 PM Masahiro Ikeda <ikedamsh(at)oss(dot)nttdata(dot)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."?

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 agree with back-patching v3-0001. I was able to reproduce the issue
> on the REL_17_STABLE branch.

Thanks for the confirmation.

> One concern is that this patch changes
> the error message in production:
>
> * v17.5 (without --enable-cassert)
> > ERROR: fork "main" does not exist for this relation
>
> * REL_17_STABLE with the v3-0001 patch (without --enable-cassert)
> > ERROR: relation "test" does not have storage
>
> However, I think preventing the assertion failure should take priority.

Yes.

Regards,

--
Fujii Masao