From: Michael Paquier <michael@paquier.xyz>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>
Cc: Peter Geoghegan <pg@bowt.ie>
Cc: Andres Freund <andres@anarazel.de>
Subject: Re: Different compression methods for FPI
Date: Wed, 7 Sep 2022 15:29:08 +0900
Message-ID: <Yxg6NJryuxThgGLF@paquier.xyz> (raw)
In-Reply-To: <Yxbs6TZAh5KwMg/v@paquier.xyz>
References: <20210519030618.GS373@telsasoft.com>
<YKTa4+kTFkb3EIBs@paquier.xyz>
<20210525044444.GV3676@telsasoft.com>
<YKymshLnYZeD0cue@paquier.xyz>
<3252174C-C21D-4711-A038-5230DE465BAE@yandex-team.ru>
<YLWWPaq/KnVS24J4@paquier.xyz>
<20210614012412.GA31772@telsasoft.com>
<20220902115511.GY31833@telsasoft.com>
<20220905002320.GD31833@telsasoft.com>
<Yxbs6TZAh5KwMg/v@paquier.xyz>
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
Attachments:0001-Provide-more-context-for-errors-of-RestoreBlockImage.patch (text/x-diff, 4.8K) download
From d413109bcb6c239023effc64ead945ad594008a6 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@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.cindex 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.cindex 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.cindex 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 (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael@paquier.xyz, pryzby@telsasoft.com, x4mmm@yandex-team.ru, thomas.munro@gmail.com, pg@bowt.ie, andres@anarazel.de
Subject: Re: Different compression methods for FPI
In-Reply-To: <Yxg6NJryuxThgGLF@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox