On 2025-07-04 20:32, Fujii Masao wrote:
On 2025/06/02 16:32, Masahiro Ikeda wrote:
OK, I think v5-0002 should be back-patched, since using incorrect
error codes is essentially a bug.
Thanks for updating the patches!
While reviewing the code:
amtup = SearchSysCache1(AMOID,
ObjectIdGetDatum(am_id));
amtuprel = SearchSysCache1(AMOID,
ObjectIdGetDatum(rel->rd_rel->relam));
ereport(ERROR,
-
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("expected \"%s\" index as
targets for verification", NameStr(((Form_pg_am)
GETSTRUCT(amtup))->amname)),
errdetail("Relation \"%s\" is a %s
index.",
RelationGetRelationName(rel), NameStr(((Form_pg_am)
GETSTRUCT(amtuprel))->amname))));
I initially thought switching to ERRCODE_WRONG_OBJECT_TYPE made sense,
but after checking similar cases, I'm reconsidering. For instance,
pgstat_relation() in pgstattuple.c uses ERRCODE_FEATURE_NOT_SUPPORTED
when the access method is unexpected. That suggests using
FEATURE_NOT_SUPPORTED here may not be incorrect.
OK, that seems reasonable to me.
if (!rel->rd_index->indisvalid)
ereport(ERROR,
-
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("cannot check index \"%s\"",
RelationGetRelationName(rel)),
errdetail("Index is not valid.")));
Other parts of the codebase (including pgstattuple) use
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE when rejecting invalid index.
So, changing to this error code still seems reasonable and more
consistent overall.
Given that, I'd like to merge just this error code change from
the 0002 patch into 0003, and apply the combined patch to
the master branch only. Although I briefly considered backpatching
the change of error code, it could be seen as a behavioral change,
and the existing error code doesn't cause any real problem in older
branches. So it's probably better to leave it as-is there.
The updated patch is attached. It also changes index_checkable()
from extern to static, since it's only used within verify_common.c.
Thoughts?
Thanks for updating the patch. I agree with your idea.
BTW, is "witha" a typo in the commit message? Aside from that, I have
no additional comments on your patch.
Regards,
--
Masahiro Ikeda
NTT DATA Japan Corporation