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
