On Tue, Sep 06, 2022 at 03:47:05PM +0900, Michael Paquier wrote:
> On Sun, Sep 04, 2022 at 07:23:20PM -0500, Justin Pryzby wrote:
>> page = BufferGetPage(*buf);
>> if (!RestoreBlockImage(record, block_id, page))
>> - elog(ERROR, "failed to restore block image");
>> + ereport(ERROR,
>> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>> + errmsg("failed to restore block image"),
>> + errdetail("%s", record->errormsg_buf));
>> +
>
> Yes, you are right here. elog()'s should never be used for things
> that could be triggered by the user, even if this one depends on the
> build options. I think that the error message ought to be updated as
> "could not restore block image" instead, to be more in line with the
> project policy.
At the end, I have not taken the approach to use errdetail() for this
problem as errormsg_buf is designed to include an error string. So, I
have finished by refining the error messages generated in
RestoreBlockImage(), consuming them with an ERRCODE_INTERNAL_ERROR.
This approach addresses a second issue, actually, because we have
never provided any context when there are inconsistencies in the
decoded record for max_block_id, has_image or in_use when restoring a
block image. This one is older than v15, but we have received
complaints about that for ~14 as far as I know, so I would leave this
change for HEAD and REL_15_STABLE.
--
Michael
