On 2025/05/29 11:46, Masahiro Ikeda wrote:
Thanks for your feedback. I've attached the updated patches.
I've pushed the 0001 patch. Thanks!
What do you think about adding new error messages specifically for partitioned
indexes?
If the target is a partitioned index, the error messages would be:
ERROR: expected index as targets for verification
DETAIL: This operation is not supported for partitioned indexes.
If the target is an index, but its access method is not supported, the error
messages would be:
ERROR: expected "btree" index as targets for verification
DETAIL: Relation "bttest_a_brin_idx" is a brin index.
Thanks for considering this. The proposed messages look good to me.
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.
Agreed.
I may have been mistaken earlier. Based on the comment in SearchSysCache(),
the tuple returned by SearchSysCache1() is valid until the end of the
transaction.
Since index_checkable() raises an error and ends the transaction immediately
after calling SearchSysCache1(), it seems safe to skip ReleaseSysCache()
in this case. Using get_am_name() instead seems simpler, though.
Thought?
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?
Agreed. I also change the error code to ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE
when the index is not valid.
+1.
Should we commit patch 0003 before 0002? Also, should we consider back-patching
it?
Regards,
--
Fujii Masao
NTT DATA Japan Corporation
