On 2025/05/16 15:33, Dilip Kumar wrote:
On Fri, May 16, 2025 at 11:51 AM Masahiro Ikeda
<ikeda...@oss.nttdata.com> wrote:
Thank you for your comments!
I updated the patch to use RELKIND_HAS_STORAGE() as done in
commit 4623d7144. Please see the v2-0001 patch for the changes.
Thanks for updating the patch!
You added a test for pg_prewarm() with a partitioned table in the TAP tests.
But, wouldn't it be simpler and easier to maintain if we added this
as a regular regression test (i.e., using .sql and .out files) instead of
TAP tests? That should be sufficient for this case?
Also, since the issue was introduced in v17, this patch should be
back-patched to v17, right?
On 2025-05-15 19:57, Richard Guo wrote:
+1. FWIW, not long ago we fixed a similar Assert failure in
contrib/pg_freespacemap by verifying RELKIND_HAS_STORAGE() before
trying to access the storage (see 4623d7144). Wondering if there are
other similar issues elsewhere in contrib ...
I tested all contrib functions that take regclass arguments (see
attached test.sql and test_result.txt). The result shows that only
pg_prewarm() can lead to the assertion failure.
Right, even while I was searching, I see this is used in 3 contrib
modules, amcheck, autoprewarm, and pg_prewarm. amcheck is already
checking for AM type, and Autoprewarm is identifying the relation by
block, so there is no chance of getting the relation which do not have
storage.
However, I found that amcheck's error messages can be misleading
when run on partitioned indexes.
For example, on the master branch, amcheck (e.g., bt_index_check
function)
shows this error:
> ERROR: expected "btree" index as targets for verification
> DETAIL: Relation "pgbench_accounts_pkey" is a btree index.
This message says it expects a "btree" index, yet also states the
relation
is a "btree" index, which can seem contradictory. The actual issue is
that
the index is a btree partitioned index, but this detail is missing,
causing
possible confusion.
Yes, I found the error message confusing during testing as well, so it
makes sense to improve it.
+1
Regarding the patch, how about also adding a regression test for
amcheck with a partitioned index?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
