On 2025/05/26 16:55, ikedamsh wrote:
2025/05/21 12:54 Fujii Masao <[email protected]>:
On Mon, May 19, 2025 at 5:18 PM Masahiro Ikeda <[email protected]>
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
