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
From d413109bcb6c239023effc64ead945ad594008a6 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 7 Sep 2022 15:25:50 +0900
Subject: [PATCH] Provide more context for errors of RestoreBlockImage()
---
src/backend/access/transam/xlogreader.c | 22 +++++++++++++++++-----
src/backend/access/transam/xlogrecovery.c | 4 +++-
src/backend/access/transam/xlogutils.c | 4 +++-
3 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index cdcacc7803..94cbeba459 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -2021,7 +2021,8 @@ XLogRecGetBlockData(XLogReaderState *record, uint8 block_id, Size *len)
/*
* Restore a full-page image from a backup block attached to an XLOG record.
*
- * Returns true if a full-page image is restored.
+ * Returns true if a full-page image is restored, and false on failure with
+ * an error to be consumed by the caller.
*/
bool
RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page)
@@ -2032,9 +2033,20 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page)
if (block_id > record->record->max_block_id ||
!record->record->blocks[block_id].in_use)
+ {
+ report_invalid_record(record,
+ "could not restore image at %X/%X with invalid block %d specified",
+ LSN_FORMAT_ARGS(record->ReadRecPtr),
+ block_id);
return false;
+ }
if (!record->record->blocks[block_id].has_image)
+ {
+ report_invalid_record(record, "could not restore image at %X/%X with invalid state, block %d",
+ LSN_FORMAT_ARGS(record->ReadRecPtr),
+ block_id);
return false;
+ }
bkpb = &record->record->blocks[block_id];
ptr = bkpb->bkp_image;
@@ -2057,7 +2069,7 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page)
bkpb->bimg_len, BLCKSZ - bkpb->hole_length) <= 0)
decomp_success = false;
#else
- report_invalid_record(record, "image at %X/%X compressed with %s not supported by build, block %d",
+ report_invalid_record(record, "could not restore image at %X/%X compressed with %s not supported by build, block %d",
LSN_FORMAT_ARGS(record->ReadRecPtr),
"LZ4",
block_id);
@@ -2074,7 +2086,7 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page)
if (ZSTD_isError(decomp_result))
decomp_success = false;
#else
- report_invalid_record(record, "image at %X/%X compressed with %s not supported by build, block %d",
+ report_invalid_record(record, "could not restore image at %X/%X compressed with %s not supported by build, block %d",
LSN_FORMAT_ARGS(record->ReadRecPtr),
"zstd",
block_id);
@@ -2083,7 +2095,7 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page)
}
else
{
- report_invalid_record(record, "image at %X/%X compressed with unknown method, block %d",
+ report_invalid_record(record, "could not restore image at %X/%X compressed with unknown method, block %d",
LSN_FORMAT_ARGS(record->ReadRecPtr),
block_id);
return false;
@@ -2091,7 +2103,7 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page)
if (!decomp_success)
{
- report_invalid_record(record, "invalid compressed image at %X/%X, block %d",
+ report_invalid_record(record, "could not decompress image at %X/%X, block %d",
LSN_FORMAT_ARGS(record->ReadRecPtr),
block_id);
return false;
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index ae2af5ae3d..9a80084a68 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -2412,7 +2412,9 @@ verifyBackupPageConsistency(XLogReaderState *record)
* can be directly applied on it.
*/
if (!RestoreBlockImage(record, block_id, primary_image_masked))
- elog(ERROR, "failed to restore block image");
+ ereport(ERROR,
+ (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg_internal("%s", record->errormsg_buf)));
/*
* If masking function is defined, mask both the primary and replay
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 0cda22597f..e60951a5fc 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -393,7 +393,9 @@ XLogReadBufferForRedoExtended(XLogReaderState *record,
prefetch_buffer);
page = BufferGetPage(*buf);
if (!RestoreBlockImage(record, block_id, page))
- elog(ERROR, "failed to restore block image");
+ ereport(ERROR,
+ (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg_internal("%s", record->errormsg_buf)));
/*
* The page may be uninitialized. If so, we can't set the LSN because
--
2.37.2
signature.asc
Description: PGP signature
