From: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
To: Michael Paquier <michael@paquier.xyz>
To: Shawn Debnath <sdn@amazon.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: fix "Success" error messages
Date: Mon, 26 Aug 2019 21:40:23 +0200
Message-ID: <36d38542-63e9-b73c-09fe-bd5e32e06066@2ndquadrant.com> (raw)
In-Reply-To: <20190619025155.GD17450@paquier.xyz>
References: <5de61b6b-8be9-7771-0048-860328efe027@2ndquadrant.com>
<20190618161319.GA5285@f01898859afd.ant.amazon.com>
<20190619025155.GD17450@paquier.xyz>
On 2019-06-19 04:51, Michael Paquier wrote:
> On Tue, Jun 18, 2019 at 09:13:19AM -0700, Shawn Debnath wrote:
>>> case SLRU_WRITE_FAILED:
>>> ereport(ERROR,
>>> (errcode_for_file_access(),
>>> errmsg("could not access status of transaction %u", xid),
>>> - errdetail("Could not write to file \"%s\" at offset %u: %m.",
>>> - path, offset)));
>>> + errno == 0
>>> + ? errdetail("Short write to file \"%s\" at offset %u.", path, offset)
>>> + : errdetail("Could not write to file \"%s\" at offset %u: %m.",
>>> + path, offset)));
>
> There is no point to call errcode_for_file_access() if we know that
> errno is 0. Not a big deal, still.. The last time I looked at that,
> the best way I could think of would be to replace slru_errcause with a
> proper error string generated at error time. Perhaps the partial
> read/write case does not justify this extra facility. I don't know.
> At least that would be more flexible.
Here is an updated patch set that rearranges this a bit according to
your suggestions, and also fixes some similar issues in pg_checksums.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:download all (2 files, .tar.gz)v2-0001-Better-error-messages-for-short-reads-writes-in-S.patch (text/plain, 2.3K) download
From d1d306f80c53a59b67823ae044bf85a2718ea94e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 26 Aug 2019 21:37:10 +0200
Subject: [PATCH v2 1/2] Better error messages for short reads/writes in SLRU
This avoids getting a
Could not read from file ...: Success.
for a short read or write (since errno is not set in that case).
Instead, report a more specific error messages.
---
src/backend/access/transam/slru.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.cindex 0fbcb4e6fe..e38f9199dd 100644--- a/src/backend/access/transam/slru.c+++ b/src/backend/access/transam/slru.c@@ -920,18 +920,29 @@ SlruReportIOError(SlruCtl ctl, int pageno, TransactionId xid)
path, offset)));
break;
case SLRU_READ_FAILED:
- ereport(ERROR,- (errcode_for_file_access(),- errmsg("could not access status of transaction %u", xid),- errdetail("Could not read from file \"%s\" at offset %u: %m.",- path, offset)));+ if (errno)+ ereport(ERROR,+ (errcode_for_file_access(),+ errmsg("could not access status of transaction %u", xid),+ errdetail("Could not read from file \"%s\" at offset %u: %m.",+ path, offset)));+ else+ ereport(ERROR,+ (errmsg("could not access status of transaction %u", xid),+ errdetail("Could not read from file \"%s\" at offset %u: read too few bytes.", path, offset)));
break;
case SLRU_WRITE_FAILED:
- ereport(ERROR,- (errcode_for_file_access(),- errmsg("could not access status of transaction %u", xid),- errdetail("Could not write to file \"%s\" at offset %u: %m.",- path, offset)));+ if (errno)+ ereport(ERROR,+ (errcode_for_file_access(),+ errmsg("could not access status of transaction %u", xid),+ errdetail("Could not write to file \"%s\" at offset %u: %m.",+ path, offset)));+ else+ ereport(ERROR,+ (errmsg("could not access status of transaction %u", xid),+ errdetail("Could not write to file \"%s\" at offset %u: wrote too few bytes.",+ path, offset)));
break;
case SLRU_FSYNC_FAILED:
ereport(data_sync_elevel(ERROR),
base-commit: acb96eb7d294a003a9392cdd445630ef137d9918
--
2.22.0
v2-0002-pg_checksums-Handle-read-and-write-returns-correc.patch (text/plain, 1.8K) download
From 89fe3678ef9443fc56a8ebfc3657ad06f5585bf5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 26 Aug 2019 21:37:50 +0200
Subject: [PATCH v2 2/2] pg_checksums: Handle read and write returns correctly
The read() return was not checking for errors, the write() return was
not checking for short writes.
---
src/bin/pg_checksums/pg_checksums.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.cindex 8c00ec9a3b..40619be0fa 100644--- a/src/bin/pg_checksums/pg_checksums.c+++ b/src/bin/pg_checksums/pg_checksums.c@@ -196,7 +196,13 @@ scan_file(const char *fn, BlockNumber segmentno)
if (r == 0)
break;
- if (r != BLCKSZ)+ else if (r < 0)+ {+ pg_log_error("could not read block %u in file \"%s\": %m",+ blockno, fn);+ exit(1);+ }+ else if (r != BLCKSZ)
{
pg_log_error("could not read block %u in file \"%s\": read %d of %d",
blockno, fn, r, BLCKSZ);
@@ -222,6 +228,8 @@ scan_file(const char *fn, BlockNumber segmentno)
}
else if (mode == PG_MODE_ENABLE)
{
+ int w;+
/* Set checksum in page header */
header->pd_checksum = csum;
@@ -233,10 +241,15 @@ scan_file(const char *fn, BlockNumber segmentno)
}
/* Write block with checksum */
- if (write(f, buf.data, BLCKSZ) != BLCKSZ)+ w = write(f, buf.data, BLCKSZ);+ if (w != BLCKSZ)
{
- pg_log_error("could not write block %u in file \"%s\": %m",- blockno, fn);+ if (w < 0)+ pg_log_error("could not write block %u in file \"%s\": %m",+ blockno, fn);+ else+ pg_log_error("could not write block %u in file \"%s\": wrote %d of %d",+ blockno, fn, w, BLCKSZ);
exit(1);
}
}
--
2.22.0
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: peter.eisentraut@2ndquadrant.com, michael@paquier.xyz, sdn@amazon.com
Subject: Re: fix "Success" error messages
In-Reply-To: <36d38542-63e9-b73c-09fe-bd5e32e06066@2ndquadrant.com>
* 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