On Tue, Jun 12, 2018 at 01:19:54PM +0900, Michael Paquier wrote: > Agreed. I also quite like the message mentioning directly 2PC files as > well. I think that we could gain by making all end messages more > consistent, as the markers used and the style of each message is > slightly different, so I would suggest something like that instead to > gain in consistency: > if (readBytes < 0) > ereport(elevel, "could not blah: %m"); > else > ereport(elevel, "could not blah: %d read, expected %zu"); > > My point is that if we use the same markers and the same end messages, > then those are easier to grep for, and callers are still free to provide > the head of error messages the way they want depending on the situation.
I have dug again into this stuff, and I have finished with the attached which uses mainly "could not read file %s: read %d bytes, expected %zu". The markers are harder to make consistent without being more invasive so I stopped on that. There is also this bit in slru.c which I'd like to discuss: + /* + * Note that this would report success if the number of bytes read is + * positive, but lacking data so that errno is not set, which would be + * confusing, so set errno to EIO in this case. + */ + if (errno == 0) + errno = EIO; Please note that I don't necessarily propose to add this in the final patch, and I think that at least an XXX comment should be added here to mention that errno may not be set. Thoughts? -- Michael
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 1132eef038..805aa521cf 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -684,6 +684,14 @@ SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotno)
if (read(fd, shared->page_buffer[slotno], BLCKSZ) != BLCKSZ)
{
pgstat_report_wait_end();
+
+ /*
+ * Note that this would report success if the number of bytes read is
+ * positive, but lacking data so that errno is not set, which would be
+ * confusing, so set errno to EIO in this case.
+ */
+ if (errno == 0)
+ errno = EIO;
slru_errcause = SLRU_READ_FAILED;
slru_errno = errno;
CloseTransientFile(fd);
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 65194db70e..95ab03628f 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1219,6 +1219,7 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
uint32 crc_offset;
pg_crc32c calc_crc,
file_crc;
+ int r;
TwoPhaseFilePath(path, xid);
@@ -1272,15 +1273,28 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
buf = (char *) palloc(stat.st_size);
pgstat_report_wait_start(WAIT_EVENT_TWOPHASE_FILE_READ);
- if (read(fd, buf, stat.st_size) != stat.st_size)
+ r = read(fd, buf, stat.st_size);
+ if (r != stat.st_size)
{
+ int save_errno = errno;
+
pgstat_report_wait_end();
CloseTransientFile(fd);
if (give_warnings)
- ereport(WARNING,
- (errcode_for_file_access(),
- errmsg("could not read two-phase state file \"%s\": %m",
- path)));
+ {
+ if (r < 0)
+ {
+ errno = save_errno;
+ ereport(WARNING,
+ (errcode_for_file_access(),
+ errmsg("could not read two-phase state file \"%s\": %m",
+ path)));
+ }
+ else
+ ereport(WARNING,
+ (errmsg("could not read two-phase state file \"%s\": read %d bytes, expected %zu",
+ path, r, stat.st_size)));
+ }
pfree(buf);
return NULL;
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index adbd6a2126..471ef87842 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3395,21 +3395,24 @@ XLogFileCopy(XLogSegNo destsegno, TimeLineID srcTLI, XLogSegNo srcsegno,
if (nread > 0)
{
+ int r;
+
if (nread > sizeof(buffer))
nread = sizeof(buffer);
errno = 0;
pgstat_report_wait_start(WAIT_EVENT_WAL_COPY_READ);
- if (read(srcfd, buffer, nread) != nread)
+ r = read(srcfd, buffer, nread);
+ if (r != nread)
{
- if (errno != 0)
+ if (r < 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not read file \"%s\": %m",
path)));
else
ereport(ERROR,
- (errmsg("not enough data in file \"%s\"",
- path)));
+ (errmsg("could not read file \"%s\": read %d bytes, expected %d",
+ path, r, nread)));
}
pgstat_report_wait_end();
}
@@ -4509,7 +4512,8 @@ ReadControlFile(void)
errmsg("could not read from control file: %m")));
else
ereport(PANIC,
- (errmsg("could not read from control file: read %d bytes, expected %d", r, (int) sizeof(ControlFileData))));
+ (errmsg("could not read from control file: read %d bytes, expected %zu",
+ r, sizeof(ControlFileData))));
}
pgstat_report_wait_end();
@@ -11594,6 +11598,7 @@ XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
int emode = private->emode;
uint32 targetPageOff;
XLogSegNo targetSegNo PG_USED_FOR_ASSERTS_ONLY;
+ int r;
XLByteToSeg(targetPagePtr, targetSegNo, wal_segment_size);
targetPageOff = XLogSegmentOffset(targetPagePtr, wal_segment_size);
@@ -11675,8 +11680,10 @@ retry:
if (lseek(readFile, (off_t) readOff, SEEK_SET) < 0)
{
char fname[MAXFNAMELEN];
+ int save_errno = errno;
XLogFileName(fname, curFileTLI, readSegNo, wal_segment_size);
+ errno = save_errno;
ereport(emode_for_corrupt_record(emode, targetPagePtr + reqLen),
(errcode_for_file_access(),
errmsg("could not seek in log segment %s to offset %u: %m",
@@ -11685,16 +11692,28 @@ retry:
}
pgstat_report_wait_start(WAIT_EVENT_WAL_READ);
- if (read(readFile, readBuf, XLOG_BLCKSZ) != XLOG_BLCKSZ)
+ r = read(readFile, readBuf, XLOG_BLCKSZ);
+ if (r != XLOG_BLCKSZ)
{
char fname[MAXFNAMELEN];
+ int save_errno = errno;
pgstat_report_wait_end();
XLogFileName(fname, curFileTLI, readSegNo, wal_segment_size);
- ereport(emode_for_corrupt_record(emode, targetPagePtr + reqLen),
- (errcode_for_file_access(),
- errmsg("could not read from log segment %s, offset %u: %m",
- fname, readOff)));
+
+ if (r < 0)
+ {
+ errno = save_errno;
+ ereport(emode_for_corrupt_record(emode, targetPagePtr + reqLen),
+ (errcode_for_file_access(),
+ errmsg("could not read from log segment %s, offset %u: %m",
+ fname, readOff)));
+ }
+ else
+ ereport(emode_for_corrupt_record(emode, targetPagePtr + reqLen),
+ (errmsg("could not read from log segment %s, offset %u: read %d bytes, expected %d",
+ fname, readOff, r, XLOG_BLCKSZ)));
+
goto next_record_is_invalid;
}
pgstat_report_wait_end();
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index 963878a5d8..053c13c43e 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -697,9 +697,16 @@ StartupReplicationOrigin(void)
/* verify magic, that is written even if nothing was active */
readBytes = read(fd, &magic, sizeof(magic));
if (readBytes != sizeof(magic))
- ereport(PANIC,
- (errmsg("could not read file \"%s\": %m",
- path)));
+ {
+ if (readBytes < 0)
+ ereport(PANIC,
+ (errmsg("could not read file \"%s\": %m",
+ path)));
+ else
+ ereport(PANIC,
+ (errmsg("could not read file \"%s\": read %d bytes, expected %zu",
+ path, readBytes, sizeof(magic))));
+ }
COMP_CRC32C(crc, &magic, sizeof(magic));
if (magic != REPLICATION_STATE_MAGIC)
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 4123cdebcf..448f6a87c5 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1708,11 +1708,21 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
pgstat_report_wait_end();
if (readBytes != SnapBuildOnDiskConstantSize)
{
+ int save_errno = errno;
+
CloseTransientFile(fd);
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not read file \"%s\", read %d of %d: %m",
- path, readBytes, (int) SnapBuildOnDiskConstantSize)));
+
+ if (readBytes < 0)
+ {
+ errno = save_errno;
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m", path)));
+ }
+ else
+ ereport(ERROR,
+ (errmsg("could not read file \"%s\": read %d bytes, expected %zu",
+ path, readBytes, SnapBuildOnDiskConstantSize)));
}
if (ondisk.magic != SNAPBUILD_MAGIC)
@@ -1736,11 +1746,21 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
pgstat_report_wait_end();
if (readBytes != sizeof(SnapBuild))
{
+ int save_errno = errno;
+
CloseTransientFile(fd);
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not read file \"%s\", read %d of %d: %m",
- path, readBytes, (int) sizeof(SnapBuild))));
+
+ if (readBytes < 0)
+ {
+ errno = save_errno;
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m", path)));
+ }
+ else
+ ereport(ERROR,
+ (errmsg("could not read file \"%s\": read %d bytes, expected %zu",
+ path, readBytes, sizeof(SnapBuild))));
}
COMP_CRC32C(checksum, &ondisk.builder, sizeof(SnapBuild));
@@ -1753,11 +1773,21 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
pgstat_report_wait_end();
if (readBytes != sz)
{
+ int save_errno = errno;
+
CloseTransientFile(fd);
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not read file \"%s\", read %d of %d: %m",
- path, readBytes, (int) sz)));
+
+ if (readBytes < 0)
+ {
+ errno = save_errno;
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m", path)));
+ }
+ else
+ ereport(ERROR,
+ (errmsg("could not read file \"%s\": read %d bytes, expected %zu",
+ path, readBytes, sz)));
}
COMP_CRC32C(checksum, ondisk.builder.was_running.was_xip, sz);
@@ -1769,11 +1799,21 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
pgstat_report_wait_end();
if (readBytes != sz)
{
+ int save_errno = errno;
+
CloseTransientFile(fd);
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not read file \"%s\", read %d of %d: %m",
- path, readBytes, (int) sz)));
+
+ if (readBytes < 0)
+ {
+ errno = save_errno;
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m", path)));
+ }
+ else
+ ereport(ERROR,
+ (errmsg("could not read file \"%s\": read %d bytes, expected %zu",
+ path, readBytes, sz)));
}
COMP_CRC32C(checksum, ondisk.builder.committed.xip, sz);
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index e8b76b2f20..54e02f9963 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1401,11 +1401,15 @@ RestoreSlotFromDisk(const char *name)
CloseTransientFile(fd);
errno = saved_errno;
- ereport(PANIC,
- (errcode_for_file_access(),
- errmsg("could not read file \"%s\", read %d of %u: %m",
- path, readBytes,
- (uint32) ReplicationSlotOnDiskConstantSize)));
+ if (readBytes < 0)
+ ereport(PANIC,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m", path)));
+ else
+ ereport(PANIC,
+ (errmsg("could not read file \"%s\": read %d bytes, expected %u",
+ path, readBytes,
+ (uint32) ReplicationSlotOnDiskConstantSize)));
}
/* verify magic */
@@ -1441,10 +1445,14 @@ RestoreSlotFromDisk(const char *name)
CloseTransientFile(fd);
errno = saved_errno;
- ereport(PANIC,
- (errcode_for_file_access(),
- errmsg("could not read file \"%s\", read %d of %u: %m",
- path, readBytes, cp.length)));
+ if (readBytes < 0)
+ ereport(PANIC,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m", path)));
+ else
+ ereport(PANIC,
+ (errmsg("could not read file \"%s\": read %d bytes, expected %u",
+ path, readBytes, cp.length)));
}
CloseTransientFile(fd);
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index e47ddca6bc..15508ae633 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -501,11 +501,16 @@ SendTimeLineHistory(TimeLineHistoryCmd *cmd)
pgstat_report_wait_start(WAIT_EVENT_WALSENDER_TIMELINE_HISTORY_READ);
nread = read(fd, rbuf, sizeof(rbuf));
pgstat_report_wait_end();
- if (nread <= 0)
+ if (nread < 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not read file \"%s\": %m",
path)));
+ else if (nread == 0)
+ ereport(ERROR,
+ (errmsg("could not read file \"%s\": read %d bytes, expected %zu",
+ path, nread, bytesleft)));
+
pq_sendbytes(&buf, rbuf, nread);
bytesleft -= nread;
}
@@ -2425,7 +2430,7 @@ retry:
pgstat_report_wait_start(WAIT_EVENT_WAL_READ);
readbytes = read(sendFile, p, segbytes);
pgstat_report_wait_end();
- if (readbytes <= 0)
+ if (readbytes < 0)
{
ereport(ERROR,
(errcode_for_file_access(),
@@ -2433,6 +2438,13 @@ retry:
XLogFileNameP(curFileTimeLine, sendSegNo),
sendOff, (unsigned long) segbytes)));
}
+ else if (readbytes == 0)
+ {
+ ereport(ERROR,
+ (errmsg("could not read from log segment %s, offset %u: read %d bytes, expected %lu",
+ XLogFileNameP(curFileTimeLine, sendSegNo),
+ sendOff, readbytes, (unsigned long) segbytes)));
+ }
/* Update state for read */
recptr += readbytes;
diff --git a/src/backend/utils/cache/relmapper.c b/src/backend/utils/cache/relmapper.c
index 99d095f2df..f4fd0f1819 100644
--- a/src/backend/utils/cache/relmapper.c
+++ b/src/backend/utils/cache/relmapper.c
@@ -629,6 +629,7 @@ load_relmap_file(bool shared)
char mapfilename[MAXPGPATH];
pg_crc32c crc;
int fd;
+ int r;
if (shared)
{
@@ -659,11 +660,19 @@ load_relmap_file(bool shared)
* are able to access any relation that's affected by the change.
*/
pgstat_report_wait_start(WAIT_EVENT_RELATION_MAP_READ);
- if (read(fd, map, sizeof(RelMapFile)) != sizeof(RelMapFile))
- ereport(FATAL,
- (errcode_for_file_access(),
- errmsg("could not read relation mapping file \"%s\": %m",
- mapfilename)));
+ r = read(fd, map, sizeof(RelMapFile));
+ if (r != sizeof(RelMapFile))
+ {
+ if (r < 0)
+ ereport(FATAL,
+ (errcode_for_file_access(),
+ errmsg("could not read relation mapping file \"%s\": %m",
+ mapfilename)));
+ else
+ ereport(FATAL,
+ (errmsg("could not read relation mapping file \"%s\": read %d bytes, expected %zu",
+ mapfilename, r, sizeof(RelMapFile))));
+ }
pgstat_report_wait_end();
CloseTransientFile(fd);
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 071b32d19d..fc4e2d0492 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -284,6 +284,7 @@ FindStreamingStart(uint32 *tli)
char buf[4];
int bytes_out;
char fullpath[MAXPGPATH * 2];
+ int r;
snprintf(fullpath, sizeof(fullpath), "%s/%s", basedir, dirent->d_name);
@@ -300,10 +301,15 @@ FindStreamingStart(uint32 *tli)
progname, fullpath, strerror(errno));
disconnect_and_exit(1);
}
- if (read(fd, (char *) buf, sizeof(buf)) != sizeof(buf))
+ r = read(fd, (char *) buf, sizeof(buf));
+ if (r != sizeof(buf))
{
- fprintf(stderr, _("%s: could not read compressed file \"%s\": %s\n"),
- progname, fullpath, strerror(errno));
+ if (r < 0)
+ fprintf(stderr, _("%s: could not read compressed file \"%s\": %s\n"),
+ progname, fullpath, strerror(errno));
+ else
+ fprintf(stderr, _("%s: could not read compressed file \"%s\": read %d bytes, expected %zu\n"),
+ progname, fullpath, r, sizeof(buf));
disconnect_and_exit(1);
}
diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c
index 94bcc13ae8..895eb50cc0 100644
--- a/src/bin/pg_rewind/file_ops.c
+++ b/src/bin/pg_rewind/file_ops.c
@@ -289,6 +289,7 @@ slurpFile(const char *datadir, const char *path, size_t *filesize)
struct stat statbuf;
char fullpath[MAXPGPATH];
int len;
+ int r;
snprintf(fullpath, sizeof(fullpath), "%s/%s", datadir, path);
@@ -304,9 +305,16 @@ slurpFile(const char *datadir, const char *path, size_t *filesize)
buffer = pg_malloc(len + 1);
- if (read(fd, buffer, len) != len)
- pg_fatal("could not read file \"%s\": %s\n",
- fullpath, strerror(errno));
+ r = read(fd, buffer, len);
+ if (r != len)
+ {
+ if (r < 0)
+ pg_fatal("could not read file \"%s\": %s\n",
+ fullpath, strerror(errno));
+ else
+ pg_fatal("not enough data in file \"%s\": read %d bytes, expected %d\n",
+ fullpath, r, len);
+ }
close(fd);
/* Zero-terminate the buffer. */
diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c
index b4c1f827a6..de3112a1bd 100644
--- a/src/bin/pg_rewind/parsexlog.c
+++ b/src/bin/pg_rewind/parsexlog.c
@@ -246,6 +246,7 @@ SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
uint32 targetPageOff;
XLogRecPtr targetSegEnd;
XLogSegNo targetSegNo;
+ int r;
XLByteToSeg(targetPagePtr, targetSegNo, WalSegSz);
XLogSegNoOffsetToRecPtr(targetSegNo + 1, 0, targetSegEnd, WalSegSz);
@@ -309,10 +310,17 @@ SimpleXLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr,
return -1;
}
- if (read(xlogreadfd, readBuf, XLOG_BLCKSZ) != XLOG_BLCKSZ)
+
+ r = read(xlogreadfd, readBuf, XLOG_BLCKSZ);
+ if (r != XLOG_BLCKSZ)
{
- printf(_("could not read from file \"%s\": %s\n"), xlogfpath,
- strerror(errno));
+ if (r < 0)
+ printf(_("could not read from file \"%s\": %s\n"), xlogfpath,
+ strerror(errno));
+ else
+ printf(_("could not read from file \"%s\": read %d bytes, expected %d\n"),
+ xlogfpath, r, XLOG_BLCKSZ);
+
return -1;
}
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 5c4f38e597..ef142e8095 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -210,8 +210,10 @@ search_directory(const char *directory, const char *fname)
if (fd >= 0)
{
char buf[XLOG_BLCKSZ];
+ int r;
- if (read(fd, buf, XLOG_BLCKSZ) == XLOG_BLCKSZ)
+ r = read(fd, buf, XLOG_BLCKSZ);
+ if (r == XLOG_BLCKSZ)
{
XLogLongPageHeader longhdr = (XLogLongPageHeader) buf;
@@ -229,7 +231,8 @@ search_directory(const char *directory, const char *fname)
fatal_error("could not read file \"%s\": %s",
fname, strerror(errno));
else
- fatal_error("not enough data in file \"%s\"", fname);
+ fatal_error("could not read file \"%s\": read %d bytes, %d expected",
+ fname, r, XLOG_BLCKSZ);
}
close(fd);
return true;
@@ -411,11 +414,17 @@ XLogDumpXLogRead(const char *directory, TimeLineID timeline_id,
{
int err = errno;
char fname[MAXPGPATH];
+ int save_errno = errno;
XLogFileName(fname, timeline_id, sendSegNo, WalSegSz);
+ errno = save_errno;
- fatal_error("could not read from log file %s, offset %u, length %d: %s",
- fname, sendOff, segbytes, strerror(err));
+ if (readbytes < 0)
+ fatal_error("could not read from log file %s, offset %u, length %d: %s",
+ fname, sendOff, segbytes, strerror(err));
+ else if (readbytes == 0)
+ fatal_error("could not read from log file %s, offset %u: read %d bytes, expected %d",
+ fname, sendOff, readbytes, segbytes);
}
/* Update state for read */
signature.asc
Description: PGP signature
