Обсуждение: Re: Assertion failure in smgr.c when using pg_prewarm with partitioned tables

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

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

On 2025/07/14 17:13, Masahiro Ikeda wrote:
> 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.

Thanks for the review! You're right, that was a typo.
I've fixed it and pushed the patch. Thanks!

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation