On 2025-05-30 01:44, Fujii Masao wrote:
> I've pushed the 0001 patch. Thanks!
Thanks!
>>> 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?
As you said, it seems safe since SearchSysCache() is only used for
constructing
the error message. However, using get_am_name() is simpler and cleaner.
>>> 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?
OK, I think v5-0002 should be back-patched, since using incorrect error
codes is essentially a bug.
Regards,
--
Masahiro Ikeda
NTT DATA Japan Corporation
