On Sat, Jun 23, 2018 at 08:48:03AM +0900, Michael Paquier wrote: > That's exactly why I have started this thread so as both problems are > addressed separately: > https://www.postgresql.org/message-id/20180622061535.gd5...@paquier.xyz > And back-patching the errno patch while only bothering about messages on > HEAD matches also what I got in mind. I'll come back to this thread > once the errno issues are all addressed.
As this one is done, I have been looking at that this thread again.
Peter Eisentraut has pushed as e5d11b9 something which does not need to
worry about pluralization of error messages. So I have moved to this
message style for all messages. All of this is done as 0001.
I have been thinking as well about a common interface which could be
used to read/write/fsync transient files:
void WriteTransientFile(int fd, char *buf, Size count, int elevel,
const char *filename, uint32 wait_event_info);
bool ReadTransientFile(int fd, char *buf, Size count, int elevel,
const char *filename, uint32 wait_event_info);
void SyncTransientFile(int fd, int elevel, const char *filename
uint32 wait_event_info);
There are rather equivalent things with FileRead and FileWrite but the
purpose is different as well.
If you look at 0002, this shaves a bit of code:
6 files changed, 128 insertions(+), 200 deletions(-)
There are also a couple of advantages here:
- Centralize errno handling for transient files with ENOSPC for write(2)
and read count for read(2)
- Wait events have to be defined, so those would unlikely get forgotten
in the future.
- Error handling for CloseTransientFile in code paths is centralized.
ReadTransientFile could be redefined to return the number of bytes read
as result with caller checking for errno, but that feels a bit duplicate
work for twophase.c. WriteTransientFile and SyncTransientFile could
also have the same treatment for consistency but they would not really
be used now. Do you guys think that this is worth pursuing? Merging
0001 and 0002 together may make sense then.
--
Michael
From 041068b821dd555e437f77a99e95795eceff3189 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 25 Jun 2018 14:37:18 +0900 Subject: [PATCH 1/2] Rework error messages around file handling Some error messages related to file handling are using the code path context to define their state. For example, 2PC-related errors are referring to "two-phase status files", or "relation mapping file" is used for catalog-to-filenode mapping, however those prove to be difficult to translate, and are not more helpful than just referring to the path of the file being manipulated. So simplify all those error messages by just referring to files with their path used. In some cases, like the manipulation of WAL segments, the context is helpful so those are kept. Calls to the system function read() have also been rather inconsistent with their error handling sometimes not reporting the number of bytes read, and some other code paths trying to use an errno which has not been set. The in-core functions are using a more consistent pattern with this patch, which checks for both errno if set or if an inconsistent read is happening. So as to care about pluralization when reading an unexpected number of bytes, "could not read: read %d of %d" is used as error message. Author: Michael Paquier Discussion: https://postgr.es/m/20180622234803.ga1...@paquier.xyz --- src/backend/access/transam/twophase.c | 40 ++++++------ src/backend/access/transam/xlog.c | 40 ++++++++---- src/backend/replication/logical/origin.c | 13 +++- src/backend/replication/logical/snapbuild.c | 68 +++++++++++++++------ src/backend/replication/slot.c | 26 +++++--- src/backend/replication/walsender.c | 16 ++++- src/backend/utils/cache/relmapper.c | 28 ++++++--- src/bin/pg_basebackup/pg_receivewal.c | 12 +++- src/bin/pg_rewind/file_ops.c | 14 ++++- src/bin/pg_rewind/parsexlog.c | 14 ++++- src/bin/pg_waldump/pg_waldump.c | 17 ++++-- 11 files changed, 200 insertions(+), 88 deletions(-) diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index a9ef1b3d73..10c1e31c0f 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); @@ -1228,8 +1229,7 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings) if (give_warnings) ereport(WARNING, (errcode_for_file_access(), - errmsg("could not open two-phase state file \"%s\": %m", - path))); + errmsg("could not open file \"%s\": %m", path))); return NULL; } @@ -1249,8 +1249,7 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings) errno = save_errno; ereport(WARNING, (errcode_for_file_access(), - errmsg("could not stat two-phase state file \"%s\": %m", - path))); + errmsg("could not stat file \"%s\": %m", path))); } return NULL; } @@ -1277,7 +1276,8 @@ 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; @@ -1285,11 +1285,17 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings) CloseTransientFile(fd); if (give_warnings) { - errno = save_errno; - 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 file \"%s\": %m", path))); + } + else + ereport(WARNING, + (errmsg("could not read file \"%s\": read %d of %zu", + path, r, stat.st_size))); } pfree(buf); return NULL; @@ -1637,8 +1643,7 @@ RemoveTwoPhaseFile(TransactionId xid, bool giveWarning) if (errno != ENOENT || giveWarning) ereport(WARNING, (errcode_for_file_access(), - errmsg("could not remove two-phase state file \"%s\": %m", - path))); + errmsg("could not remove file \"%s\": %m", path))); } /* @@ -1666,8 +1671,7 @@ RecreateTwoPhaseFile(TransactionId xid, void *content, int len) if (fd < 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not recreate two-phase state file \"%s\": %m", - path))); + errmsg("could not recreate file \"%s\": %m", path))); /* Write content and CRC */ pgstat_report_wait_start(WAIT_EVENT_TWOPHASE_FILE_WRITE); @@ -1682,7 +1686,7 @@ RecreateTwoPhaseFile(TransactionId xid, void *content, int len) errno = save_errno ? save_errno : ENOSPC; ereport(ERROR, (errcode_for_file_access(), - errmsg("could not write two-phase state file: %m"))); + errmsg("could not write file \"%s\": %m", path))); } if (write(fd, &statefile_crc, sizeof(pg_crc32c)) != sizeof(pg_crc32c)) { @@ -1695,7 +1699,7 @@ RecreateTwoPhaseFile(TransactionId xid, void *content, int len) errno = save_errno ? save_errno : ENOSPC; ereport(ERROR, (errcode_for_file_access(), - errmsg("could not write two-phase state file: %m"))); + errmsg("could not write file \"%s\": %m", path))); } pgstat_report_wait_end(); @@ -1712,14 +1716,14 @@ RecreateTwoPhaseFile(TransactionId xid, void *content, int len) errno = save_errno; ereport(ERROR, (errcode_for_file_access(), - errmsg("could not fsync two-phase state file: %m"))); + errmsg("could not fsync file \"%s\": %m", path))); } pgstat_report_wait_end(); if (CloseTransientFile(fd) != 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not close two-phase state file: %m"))); + errmsg("could not close file \"%s\": %m", path))); } /* diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 1a419aa49b..5bb19cef31 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -3398,21 +3398,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 of %d", + path, r, nread))); } pgstat_report_wait_end(); } @@ -4499,7 +4502,7 @@ ReadControlFile(void) if (fd < 0) ereport(PANIC, (errcode_for_file_access(), - errmsg("could not open control file \"%s\": %m", + errmsg("could not open file \"%s\": %m", XLOG_CONTROL_FILE))); pgstat_report_wait_start(WAIT_EVENT_CONTROL_FILE_READ); @@ -4509,10 +4512,12 @@ ReadControlFile(void) if (r < 0) ereport(PANIC, (errcode_for_file_access(), - errmsg("could not read from control file: %m"))); + errmsg("could not read file \"%s\": %m", + XLOG_CONTROL_FILE))); else ereport(PANIC, - (errmsg("could not read from control file: read %d bytes, expected %d", r, (int) sizeof(ControlFileData)))); + (errmsg("could not read file \"%s\": read %d of %zu", + XLOG_CONTROL_FILE, r, sizeof(ControlFileData)))); } pgstat_report_wait_end(); @@ -11597,6 +11602,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); @@ -11690,18 +11696,26 @@ 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); - 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))); + 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 of %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 3d3f6dff1b..841e24c03d 100644 --- a/src/backend/replication/logical/origin.c +++ b/src/backend/replication/logical/origin.c @@ -712,9 +712,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 of %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 2c4a1bab4b..da97efc305 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -1719,11 +1719,18 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn) int save_errno = errno; CloseTransientFile(fd); - errno = save_errno; - 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 of %zu", + path, readBytes, SnapBuildOnDiskConstantSize))); } if (ondisk.magic != SNAPBUILD_MAGIC) @@ -1750,11 +1757,18 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn) int save_errno = errno; CloseTransientFile(fd); - errno = save_errno; - 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 of %zu", + path, readBytes, sizeof(SnapBuild)))); } COMP_CRC32C(checksum, &ondisk.builder, sizeof(SnapBuild)); @@ -1770,11 +1784,18 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn) int save_errno = errno; CloseTransientFile(fd); - errno = save_errno; - 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 of %zu", + path, readBytes, sz))); } COMP_CRC32C(checksum, ondisk.builder.was_running.was_xip, sz); @@ -1789,11 +1810,18 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn) int save_errno = errno; CloseTransientFile(fd); - errno = save_errno; - 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 of %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 f5927b4d1d..ddd91ef886 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -1406,11 +1406,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 of %u", + path, readBytes, + (uint32) ReplicationSlotOnDiskConstantSize))); } /* verify magic */ @@ -1446,10 +1450,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 of %u", + path, readBytes, cp.length))); } CloseTransientFile(fd); diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index e47ddca6bc..9b87c7ca41 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 of %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 of %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..2d31f9f912 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) { @@ -648,7 +649,7 @@ load_relmap_file(bool shared) if (fd < 0) ereport(FATAL, (errcode_for_file_access(), - errmsg("could not open relation mapping file \"%s\": %m", + errmsg("could not open file \"%s\": %m", mapfilename))); /* @@ -659,11 +660,18 @@ 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 file \"%s\": %m", mapfilename))); + else + ereport(FATAL, + (errmsg("could not read file \"%s\": read %d of %zu", + mapfilename, r, sizeof(RelMapFile)))); + } pgstat_report_wait_end(); CloseTransientFile(fd); @@ -748,7 +756,7 @@ write_relmap_file(bool shared, RelMapFile *newmap, if (fd < 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not open relation mapping file \"%s\": %m", + errmsg("could not open file \"%s\": %m", mapfilename))); if (write_wal) @@ -782,7 +790,7 @@ write_relmap_file(bool shared, RelMapFile *newmap, errno = ENOSPC; ereport(ERROR, (errcode_for_file_access(), - errmsg("could not write to relation mapping file \"%s\": %m", + errmsg("could not write file \"%s\": %m", mapfilename))); } pgstat_report_wait_end(); @@ -797,14 +805,14 @@ write_relmap_file(bool shared, RelMapFile *newmap, if (pg_fsync(fd) != 0) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not fsync relation mapping file \"%s\": %m", + errmsg("could not fsync file \"%s\": %m", mapfilename))); pgstat_report_wait_end(); if (CloseTransientFile(fd)) ereport(ERROR, (errcode_for_file_access(), - errmsg("could not close relation mapping file \"%s\": %m", + errmsg("could not close file \"%s\": %m", mapfilename))); /* diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c index 071b32d19d..bdea9a85ff 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 of %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..da2bf449e4 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("could not read file \"%s\": read %d of %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..6b3a337509 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 of %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..147929f52b 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 of %d", + 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 of %d", + fname, sendOff, readbytes, segbytes); } /* Update state for read */ -- 2.18.0
From 057d3ec6b42c1c5a5b332b82af89e532ac2893f3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 25 Jun 2018 16:15:03 +0900
Subject: [PATCH 2/2] Add interface to read/write/fsync with transient files
The following set of routines gets added for the manipulation of
transient files:
void WriteTransientFile(int fd, char *buf, Size count, int elevel,
const char *filename, uint32 wait_event_info);
bool ReadTransientFile(int fd, char *buf, Size count, int elevel,
const char *filename, uint32 wait_event_info);
void SyncTransientFile(int fd, int elevel, const char *filename,
uint32 wait_event_info);
This simplifies code related to replication slots, 2PC files, relation
mapper files and snapshot builds:
- Centralize errno handling for transient files with ENOSPC for write(2)
and read count for read(2)
- Wait events have to be defined, so those would unlikely get forgotten
in the future.
- Error handling for CloseTransientFile in code paths is centralized.
---
src/backend/access/transam/twophase.c | 25 +----
src/backend/replication/logical/snapbuild.c | 110 ++------------------
src/backend/replication/slot.c | 46 +-------
src/backend/storage/file/fd.c | 97 ++++++++++++++++-
src/backend/utils/cache/relmapper.c | 40 ++-----
src/include/storage/fd.h | 10 +-
6 files changed, 128 insertions(+), 200 deletions(-)
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 10c1e31c0f..61b3780119 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1219,7 +1219,6 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
uint32 crc_offset;
pg_crc32c calc_crc,
file_crc;
- int r;
TwoPhaseFilePath(path, xid);
@@ -1275,28 +1274,10 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
*/
buf = (char *) palloc(stat.st_size);
- pgstat_report_wait_start(WAIT_EVENT_TWOPHASE_FILE_READ);
- r = read(fd, buf, stat.st_size);
- if (r != stat.st_size)
+ if (!ReadTransientFile(fd, buf, stat.st_size,
+ give_warnings ? WARNING : DEBUG3, path,
+ WAIT_EVENT_TWOPHASE_FILE_READ))
{
- int save_errno = errno;
-
- pgstat_report_wait_end();
- CloseTransientFile(fd);
- if (give_warnings)
- {
- if (r < 0)
- {
- errno = save_errno;
- ereport(WARNING,
- (errcode_for_file_access(),
- errmsg("could not read file \"%s\": %m", path)));
- }
- else
- ereport(WARNING,
- (errmsg("could not read file \"%s\": read %d of %zu",
- path, r, stat.st_size)));
- }
pfree(buf);
return NULL;
}
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index da97efc305..4ea8be6c34 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1602,20 +1602,8 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn)
ereport(ERROR,
(errmsg("could not open file \"%s\": %m", path)));
- pgstat_report_wait_start(WAIT_EVENT_SNAPBUILD_WRITE);
- if ((write(fd, ondisk, needed_length)) != needed_length)
- {
- int save_errno = errno;
-
- CloseTransientFile(fd);
-
- /* if write didn't set errno, assume problem is no disk space */
- errno = save_errno ? save_errno : ENOSPC;
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not write to file \"%s\": %m", tmppath)));
- }
- pgstat_report_wait_end();
+ WriteTransientFile(fd, (char *) ondisk, needed_length, ERROR, tmppath,
+ WAIT_EVENT_SNAPBUILD_WRITE);
/*
* fsync the file before renaming so that even if we crash after this we
@@ -1679,7 +1667,6 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
int fd;
char path[MAXPGPATH];
Size sz;
- int readBytes;
pg_crc32c checksum;
/* no point in loading a snapshot if we're already there */
@@ -1709,29 +1696,9 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
fsync_fname(path, false);
fsync_fname("pg_logical/snapshots", true);
-
/* read statically sized portion of snapshot */
- pgstat_report_wait_start(WAIT_EVENT_SNAPBUILD_READ);
- readBytes = read(fd, &ondisk, SnapBuildOnDiskConstantSize);
- pgstat_report_wait_end();
- if (readBytes != SnapBuildOnDiskConstantSize)
- {
- int save_errno = errno;
-
- CloseTransientFile(fd);
-
- 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 of %zu",
- path, readBytes, SnapBuildOnDiskConstantSize)));
- }
+ (void) ReadTransientFile(fd, (char *) &ondisk, SnapBuildOnDiskConstantSize,
+ ERROR, path, WAIT_EVENT_SNAPBUILD_READ);
if (ondisk.magic != SNAPBUILD_MAGIC)
ereport(ERROR,
@@ -1749,80 +1716,23 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
SnapBuildOnDiskConstantSize - SnapBuildOnDiskNotChecksummedSize);
/* read SnapBuild */
- pgstat_report_wait_start(WAIT_EVENT_SNAPBUILD_READ);
- readBytes = read(fd, &ondisk.builder, sizeof(SnapBuild));
- pgstat_report_wait_end();
- if (readBytes != sizeof(SnapBuild))
- {
- int save_errno = errno;
-
- CloseTransientFile(fd);
-
- 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 of %zu",
- path, readBytes, sizeof(SnapBuild))));
- }
+ (void) ReadTransientFile(fd, (char *) &ondisk.builder, sizeof(SnapBuild),
+ ERROR, path, WAIT_EVENT_SNAPBUILD_READ);
COMP_CRC32C(checksum, &ondisk.builder, sizeof(SnapBuild));
/* restore running xacts (dead, but kept for backward compat) */
sz = sizeof(TransactionId) * ondisk.builder.was_running.was_xcnt_space;
ondisk.builder.was_running.was_xip =
MemoryContextAllocZero(builder->context, sz);
- pgstat_report_wait_start(WAIT_EVENT_SNAPBUILD_READ);
- readBytes = read(fd, ondisk.builder.was_running.was_xip, sz);
- pgstat_report_wait_end();
- if (readBytes != sz)
- {
- int save_errno = errno;
-
- CloseTransientFile(fd);
-
- 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 of %zu",
- path, readBytes, sz)));
- }
+ (void) ReadTransientFile(fd, (char *) ondisk.builder.was_running.was_xip, sz,
+ ERROR, path, WAIT_EVENT_SNAPBUILD_READ);
COMP_CRC32C(checksum, ondisk.builder.was_running.was_xip, sz);
/* restore committed xacts information */
sz = sizeof(TransactionId) * ondisk.builder.committed.xcnt;
ondisk.builder.committed.xip = MemoryContextAllocZero(builder->context, sz);
- pgstat_report_wait_start(WAIT_EVENT_SNAPBUILD_READ);
- readBytes = read(fd, ondisk.builder.committed.xip, sz);
- pgstat_report_wait_end();
- if (readBytes != sz)
- {
- int save_errno = errno;
-
- CloseTransientFile(fd);
-
- 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 of %zu",
- path, readBytes, sz)));
- }
+ (void) ReadTransientFile(fd, (char *) ondisk.builder.committed.xip, sz,
+ ERROR, path, WAIT_EVENT_SNAPBUILD_READ);
COMP_CRC32C(checksum, ondisk.builder.committed.xip, sz);
CloseTransientFile(fd);
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index ddd91ef886..3f06aac2cd 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1346,7 +1346,6 @@ RestoreSlotFromDisk(const char *name)
char path[MAXPGPATH + 22];
int fd;
bool restored = false;
- int readBytes;
pg_crc32c checksum;
/* no need to lock here, no concurrent access allowed yet */
@@ -1397,25 +1396,8 @@ RestoreSlotFromDisk(const char *name)
END_CRIT_SECTION();
/* read part of statefile that's guaranteed to be version independent */
- pgstat_report_wait_start(WAIT_EVENT_REPLICATION_SLOT_READ);
- readBytes = read(fd, &cp, ReplicationSlotOnDiskConstantSize);
- pgstat_report_wait_end();
- if (readBytes != ReplicationSlotOnDiskConstantSize)
- {
- int saved_errno = errno;
-
- CloseTransientFile(fd);
- errno = saved_errno;
- 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 of %u",
- path, readBytes,
- (uint32) ReplicationSlotOnDiskConstantSize)));
- }
+ (void) ReadTransientFile(fd, (char *) &cp, ReplicationSlotOnDiskConstantSize,
+ PANIC, path, WAIT_EVENT_REPLICATION_SLOT_READ);
/* verify magic */
if (cp.magic != SLOT_MAGIC)
@@ -1439,27 +1421,9 @@ RestoreSlotFromDisk(const char *name)
path, cp.length)));
/* Now that we know the size, read the entire file */
- pgstat_report_wait_start(WAIT_EVENT_REPLICATION_SLOT_READ);
- readBytes = read(fd,
- (char *) &cp + ReplicationSlotOnDiskConstantSize,
- cp.length);
- pgstat_report_wait_end();
- if (readBytes != cp.length)
- {
- int saved_errno = errno;
-
- CloseTransientFile(fd);
- errno = saved_errno;
- 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 of %u",
- path, readBytes, cp.length)));
- }
-
+ (void) ReadTransientFile(fd, (char *) &cp + ReplicationSlotOnDiskConstantSize,
+ cp.length, PANIC, path,
+ WAIT_EVENT_REPLICATION_SLOT_READ);
CloseTransientFile(fd);
/* now verify the CRC */
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 8dd51f1767..fba7774ddc 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -47,8 +47,9 @@
* ownership mechanism that provides automatic cleanup for shared files when
* the last of a group of backends detaches.
*
- * AllocateFile, AllocateDir, OpenPipeStream and OpenTransientFile are
- * wrappers around fopen(3), opendir(3), popen(3) and open(2), respectively.
+ * AllocateFile, AllocateDir, OpenPipeStream, OpenTransientFile,
+ * WriteTransientFile and ReadTransientFile are wrappers around fopen(3),
+ * opendir(3), popen(3), open(2), write(2) and read(2) respectively.
* They behave like the corresponding native functions, except that the handle
* is registered with the current subtransaction, and will be automatically
* closed at abort. These are intended mainly for short operations like
@@ -2480,6 +2481,98 @@ TryAgain:
return NULL;
}
+/*
+ * Write to a file which has been opened using OpenTransientFile or
+ * OpenTransientFilePerm. Equivalent to write(2).
+ */
+void
+WriteTransientFile(int fd, char *buf, Size count, int elevel,
+ const char *filename, uint32 wait_event_info)
+{
+ int r;
+
+ pgstat_report_wait_start(wait_event_info);
+ r = write(fd, buf, count);
+ pgstat_report_wait_end();
+
+ if (r != count)
+ {
+ int save_errno = errno;
+
+ (void) CloseTransientFile(fd);
+
+ /* if write didn't set errno, assume problem is no disk space */
+ errno = save_errno ? save_errno : ENOSPC;
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("could not write to file \"%s\": %m", filename)));
+ }
+}
+
+/*
+ * Read from a file which has been opened using OpenTransientFile or
+ * OpenTransientFilePerm. Equivalent to read(2). Returns true on
+ * success and false on failure.
+ */
+bool
+ReadTransientFile(int fd, char *buf, Size count, int elevel,
+ const char *filename, uint32 wait_event_info)
+{
+ int r;
+
+ pgstat_report_wait_start(wait_event_info);
+ r = read(fd, buf, count);
+ pgstat_report_wait_end();
+
+ if (r != count)
+ {
+ int save_errno = errno;
+
+ CloseTransientFile(fd);
+
+ if (r < 0)
+ {
+ errno = save_errno;
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m", filename)));
+ }
+ else
+ ereport(elevel,
+ (errmsg("could not read file \"%s\": read %d of %zu",
+ filename, r, count)));
+ return false;
+ }
+
+ return true;
+}
+
+/*
+ * Write to a file which has been opened using OpenTransientFile or
+ * OpenTransientFilePerm. Equivalent to fsync(2).
+ */
+void
+SyncTransientFile(int fd, int elevel, const char *filename,
+ uint32 wait_event_info)
+{
+ int status;
+
+ pgstat_report_wait_start(wait_event_info);
+ status = pg_fsync(fd);
+ pgstat_report_wait_end();
+
+ if (status != 0)
+ {
+ int save_errno = errno;
+
+ (void) CloseTransientFile(fd);
+ errno = save_errno;
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("could not fsync file \"%s\": %m", filename)));
+ }
+}
+
/*
* Free an AllocateDesc of any type.
*
diff --git a/src/backend/utils/cache/relmapper.c b/src/backend/utils/cache/relmapper.c
index 2d31f9f912..e6eff58d40 100644
--- a/src/backend/utils/cache/relmapper.c
+++ b/src/backend/utils/cache/relmapper.c
@@ -629,7 +629,6 @@ load_relmap_file(bool shared)
char mapfilename[MAXPGPATH];
pg_crc32c crc;
int fd;
- int r;
if (shared)
{
@@ -659,20 +658,8 @@ load_relmap_file(bool shared)
* look, the sinval signaling mechanism will make us re-read it before we
* are able to access any relation that's affected by the change.
*/
- pgstat_report_wait_start(WAIT_EVENT_RELATION_MAP_READ);
- r = read(fd, map, sizeof(RelMapFile));
- if (r != sizeof(RelMapFile))
- {
- if (r < 0)
- ereport(FATAL,
- (errcode_for_file_access(),
- errmsg("could not read file \"%s\": %m", mapfilename)));
- else
- ereport(FATAL,
- (errmsg("could not read file \"%s\": read %d of %zu",
- mapfilename, r, sizeof(RelMapFile))));
- }
- pgstat_report_wait_end();
+ (void) ReadTransientFile(fd, (char *) map, sizeof(RelMapFile), FATAL,
+ mapfilename, WAIT_EVENT_RELATION_MAP_READ);
CloseTransientFile(fd);
@@ -782,18 +769,9 @@ write_relmap_file(bool shared, RelMapFile *newmap,
}
errno = 0;
- pgstat_report_wait_start(WAIT_EVENT_RELATION_MAP_WRITE);
- if (write(fd, newmap, sizeof(RelMapFile)) != sizeof(RelMapFile))
- {
- /* if write didn't set errno, assume problem is no disk space */
- if (errno == 0)
- errno = ENOSPC;
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not write file \"%s\": %m",
- mapfilename)));
- }
- pgstat_report_wait_end();
+
+ WriteTransientFile(fd, (char *) newmap, sizeof(RelMapFile), ERROR,
+ mapfilename, WAIT_EVENT_RELATION_MAP_WRITE);
/*
* We choose to fsync the data to disk before considering the task done.
@@ -801,13 +779,7 @@ write_relmap_file(bool shared, RelMapFile *newmap,
* issue, but it would complicate checkpointing --- see notes for
* CheckPointRelationMap.
*/
- pgstat_report_wait_start(WAIT_EVENT_RELATION_MAP_SYNC);
- if (pg_fsync(fd) != 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not fsync file \"%s\": %m",
- mapfilename)));
- pgstat_report_wait_end();
+ SyncTransientFile(fd, ERROR, mapfilename, WAIT_EVENT_RELATION_MAP_SYNC);
if (CloseTransientFile(fd))
ereport(ERROR,
diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h
index 8e7c9728f4..4309d9da95 100644
--- a/src/include/storage/fd.h
+++ b/src/include/storage/fd.h
@@ -34,7 +34,9 @@
*
* Likewise, use AllocateDir/FreeDir, not opendir/closedir, to allocate
* open directories (DIR*), and OpenTransientFile/CloseTransient File for an
- * unbuffered file descriptor.
+ * unbuffered file descriptor. WriteTransientFile should be used instead
+ * of write(2), ReadTransientFile instead of read(2), and SyncTransientFile
+ * instead of fsync(2).
*/
#ifndef FD_H
#define FD_H
@@ -105,6 +107,12 @@ extern int FreeDir(DIR *dir);
/* Operations to allow use of a plain kernel FD, with automatic cleanup */
extern int OpenTransientFile(const char *fileName, int fileFlags);
extern int OpenTransientFilePerm(const char *fileName, int fileFlags, mode_t fileMode);
+extern void WriteTransientFile(int fd, char *buf, Size count, int elevel,
+ const char *filename, uint32 wait_event_info);
+extern bool ReadTransientFile(int fd, char *buf, Size count, int elevel,
+ const char *filename, uint32 wait_event_info);
+extern void SyncTransientFile(int fd, int elevel, const char *filename,
+ uint32 wait_event_info);
extern int CloseTransientFile(int fd);
/* If you've really really gotta have a plain kernel FD, use this */
--
2.18.0
signature.asc
Description: PGP signature
