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."?
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
