Hi all, This is basically a new thread after what has been discussed for pg_controldata with its error handling for read(): https://www.postgresql.org/message-id/CABUevEx8ZRV5Ut_FvP2etXiQppx3xVzm7oOaV3AcdHxX81Yt8Q%40mail.gmail.com
While reviewing the core code, I have noticed similar weird error handling for read(). At the same time, some of those places may use an incorrect errno, as an error is invoked using an errno which may be overwritten by another system call. I found a funny one in slru.c, for which I have added a note in the patch. I don't think that this is worth addressing with more facility, thoughts are welcome. Attached is a patch addressing the issues I found. Thanks, -- Michael
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 87942b4cca..d487347cc6 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -683,6 +683,11 @@ SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotno)
pgstat_report_wait_start(WAIT_EVENT_SLRU_READ);
if (read(fd, shared->page_buffer[slotno], BLCKSZ) != BLCKSZ)
{
+ /*
+ * XXX: Note that this may actually report sucess if the number
+ * of bytes read is positive, but lacking data so that errno is
+ * not set.
+ */
pgstat_report_wait_end();
slru_errcause = SLRU_READ_FAILED;
slru_errno = errno;
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 65194db70e..52f634e706 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\": %d read, 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..72fd800646 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("not enough data in file \"%s\": read %d, expected %d",
+ path, r, nread)));
}
pgstat_report_wait_end();
}
@@ -11594,6 +11597,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 +11679,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 +11691,29 @@ 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),
+ (errcode_for_file_access(),
+ errmsg("could not read from log segment %s, offset %u: read %d, 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..c39d023d22 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\": %d read of %zu",
+ path, readBytes, sizeof(magic))));
+ }
COMP_CRC32C(crc, &magic, sizeof(magic));
if (magic != REPLICATION_STATE_MAGIC)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 056628fe8e..cc64ff22ac 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1394,11 +1394,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 */
@@ -1434,10 +1438,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/utils/cache/relmapper.c b/src/backend/utils/cache/relmapper.c
index 99d095f2df..c4839b11f6 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\": %d read, expected %zu",
+ mapfilename, r, sizeof(RelMapFile))));
+ }
pgstat_report_wait_end();
CloseTransientFile(fd);
diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c
index 94bcc13ae8..174ca7216d 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,17 @@ 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, 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..4f27cfe42b 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, 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..64391a7e5c 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("not enough data in file \"%s\": %d read, %d expected",
+ fname, r, XLOG_BLCKSZ);
}
close(fd);
return true;
signature.asc
Description: PGP signature
