Hi all, Joe's message here has reminded me that we have lacked a lot of error handling around CloseTransientFile(): https://www.postgresql.org/message-id/c49b69ec-e2f7-ff33-4f17-0eaa4f2ce...@joeconway.com
This has been mentioned by Alvaro a couple of months ago (cannot find the thread about that at quick glance), and I just forgot about it at that time. Anyway, attached is a patch to do some cleanup for all that: - Switch OpenTransientFile to read-only where sufficient. - Add more error handling for CloseTransientFile A major take of this patch is to make sure that the new error messages generated have an elevel consistent with their neighbors. Just on time for this last CF. Thoughts? -- Michael
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 9905593661..7b39283c89 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1991,7 +1991,10 @@ qtext_load_file(Size *buffer_size)
return NULL;
}
- CloseTransientFile(fd);
+ if (CloseTransientFile(fd))
+ ereport(LOG,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", PGSS_TEXT_FILE)));
*buffer_size = stat.st_size;
return buf;
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index f5cf9ffc9c..bce4274362 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -1202,7 +1202,10 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
errmsg("could not fsync file \"%s\": %m", path)));
pgstat_report_wait_end();
- CloseTransientFile(fd);
+ if (CloseTransientFile(fd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", path)));
}
/* ---
@@ -1300,7 +1303,11 @@ CheckPointLogicalRewriteHeap(void)
(errcode_for_file_access(),
errmsg("could not fsync file \"%s\": %m", path)));
pgstat_report_wait_end();
- CloseTransientFile(fd);
+
+ if (CloseTransientFile(fd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", path)));
}
}
FreeDir(mappings_dir);
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 3623352b9c..974d42fc86 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -599,7 +599,7 @@ SimpleLruDoesPhysicalPageExist(SlruCtl ctl, int pageno)
SlruFileName(ctl, path, segno);
- fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
+ fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
if (fd < 0)
{
/* expected: file doesn't exist */
@@ -621,7 +621,13 @@ SimpleLruDoesPhysicalPageExist(SlruCtl ctl, int pageno)
result = endpos >= (off_t) (offset + BLCKSZ);
- CloseTransientFile(fd);
+ if (CloseTransientFile(fd))
+ {
+ slru_errcause = SLRU_CLOSE_FAILED;
+ slru_errno = errno;
+ return false;
+ }
+
return result;
}
@@ -654,7 +660,7 @@ SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotno)
* SlruPhysicalWritePage). Hence, if we are InRecovery, allow the case
* where the file doesn't exist, and return zeroes instead.
*/
- fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
+ fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
if (fd < 0)
{
if (errno != ENOENT || !InRecovery)
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index c96c8b60ba..cbd9b2cee1 100644
--- a/src/backend/access/transam/timeline.c
+++ b/src/backend/access/transam/timeline.c
@@ -370,7 +370,11 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
}
pgstat_report_wait_end();
}
- CloseTransientFile(srcfd);
+
+ if (CloseTransientFile(srcfd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", path)));
}
/*
@@ -416,7 +420,6 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
(errcode_for_file_access(),
errmsg("could not close file \"%s\": %m", tmppath)));
-
/*
* Now move the completed history file into place with its final name.
*/
@@ -495,7 +498,6 @@ writeTimeLineHistoryFile(TimeLineID tli, char *content, int size)
(errcode_for_file_access(),
errmsg("could not close file \"%s\": %m", tmppath)));
-
/*
* Now move the completed history file into place with its final name.
*/
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 64679dd2de..21986e48fe 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1297,7 +1297,11 @@ ReadTwoPhaseFile(TransactionId xid, bool missing_ok)
}
pgstat_report_wait_end();
- CloseTransientFile(fd);
+
+ if (CloseTransientFile(fd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", path)));
hdr = (TwoPhaseFileHeader *) buf;
if (hdr->magic != TWOPHASE_MAGIC)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index ecd12fc53a..86b7f051a8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3476,7 +3476,10 @@ XLogFileCopy(XLogSegNo destsegno, TimeLineID srcTLI, XLogSegNo srcsegno,
(errcode_for_file_access(),
errmsg("could not close file \"%s\": %m", tmppath)));
- CloseTransientFile(srcfd);
+ if (CloseTransientFile(srcfd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", path)));
/*
* Now move the segment into place with its final name.
diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index 521236d7c9..f6293ab2ba 100644
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -455,7 +455,11 @@ lo_import_internal(text *filename, Oid lobjOid)
fnamebuf)));
inv_close(lobj);
- CloseTransientFile(fd);
+
+ if (CloseTransientFile(fd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", fnamebuf)));
return oid;
}
@@ -524,7 +528,11 @@ be_lo_export(PG_FUNCTION_ARGS)
fnamebuf)));
}
- CloseTransientFile(fd);
+ if (CloseTransientFile(fd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", fnamebuf)));
+
inv_close(lobj);
PG_RETURN_INT32(1);
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index dad2b3d065..6b5b5116f7 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -658,7 +658,10 @@ CheckPointReplicationOrigin(void)
tmppath)));
}
- CloseTransientFile(tmpfd);
+ if (CloseTransientFile(tmpfd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", tmppath)));
/* fsync, rename to permanent file, fsync file and directory */
durable_rename(tmppath, path, PANIC);
@@ -793,7 +796,10 @@ StartupReplicationOrigin(void)
errmsg("replication slot checkpoint has wrong checksum %u, expected %u",
crc, file_crc)));
- CloseTransientFile(fd);
+ if (CloseTransientFile(fd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", path)));
}
void
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 2b486b5e9f..2cfdf1c9ac 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -3360,7 +3360,10 @@ ApplyLogicalMappingFile(HTAB *tuplecid_data, Oid relid, const char *fname)
}
}
- CloseTransientFile(fd);
+ if (CloseTransientFile(fd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", path)));
}
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index a59896f082..3e9d4cd79f 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1651,7 +1651,11 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn)
errmsg("could not fsync file \"%s\": %m", tmppath)));
}
pgstat_report_wait_end();
- CloseTransientFile(fd);
+
+ if (CloseTransientFile(fd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", tmppath)));
fsync_fname("pg_logical/snapshots", true);
@@ -1846,7 +1850,10 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
}
COMP_CRC32C(checksum, ondisk.builder.committed.xip, sz);
- CloseTransientFile(fd);
+ if (CloseTransientFile(fd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", path)));
FIN_CRC32C(checksum);
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 33b23b6b6d..c2bb0d96f2 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1377,7 +1377,7 @@ RestoreSlotFromDisk(const char *name)
elog(DEBUG1, "restoring replication slot from \"%s\"", path);
- fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
+ fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
/*
* We do not need to handle this as we are rename()ing the directory into
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 9b143f361b..4bb98ef352 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -521,7 +521,11 @@ SendTimeLineHistory(TimeLineHistoryCmd *cmd)
pq_sendbytes(&buf, rbuf.data, nread);
bytesleft -= nread;
}
- CloseTransientFile(fd);
+
+ if (CloseTransientFile(fd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", path)));
pq_endmessage(&buf);
}
diff --git a/src/backend/storage/file/copydir.c b/src/backend/storage/file/copydir.c
index 1f766d20d1..342d078a8f 100644
--- a/src/backend/storage/file/copydir.c
+++ b/src/backend/storage/file/copydir.c
@@ -218,7 +218,10 @@ copy_file(char *fromfile, char *tofile)
(errcode_for_file_access(),
errmsg("could not close file \"%s\": %m", tofile)));
- CloseTransientFile(srcfd);
+ if (CloseTransientFile(srcfd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", fromfile)));
pfree(buffer);
}
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 1ba0ddac10..fdac9850e0 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -646,7 +646,14 @@ durable_rename(const char *oldfile, const char *newfile, int elevel)
errmsg("could not fsync file \"%s\": %m", newfile)));
return -1;
}
- CloseTransientFile(fd);
+
+ if (CloseTransientFile(fd))
+ {
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", newfile)));
+ return -1;
+ }
}
/* Time to do the real deal... */
@@ -3295,7 +3302,10 @@ pre_sync_fname(const char *fname, bool isdir, int elevel)
*/
pg_flush_data(fd, 0, 0);
- (void) CloseTransientFile(fd);
+ if (CloseTransientFile(fd))
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", fname)));
}
#endif /* PG_FLUSH_DATA_WORKS */
@@ -3394,7 +3404,13 @@ fsync_fname_ext(const char *fname, bool isdir, bool ignore_perm, int elevel)
return -1;
}
- (void) CloseTransientFile(fd);
+ if (CloseTransientFile(fd))
+ {
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", fname)));
+ return -1;
+ }
return 0;
}
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index aeda32c9c5..60189c57d4 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -916,7 +916,14 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
}
*mapped_address = address;
*mapped_size = request_size;
- CloseTransientFile(fd);
+
+ if (CloseTransientFile(fd))
+ {
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", name)));
+ return false;
+ }
return true;
}
diff --git a/src/backend/utils/cache/relmapper.c b/src/backend/utils/cache/relmapper.c
index 5e61d908fd..3e676d55b2 100644
--- a/src/backend/utils/cache/relmapper.c
+++ b/src/backend/utils/cache/relmapper.c
@@ -747,7 +747,11 @@ load_relmap_file(bool shared)
}
pgstat_report_wait_end();
- CloseTransientFile(fd);
+ if (CloseTransientFile(fd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m",
+ mapfilename)));
/* check for correct magic number, etc */
if (map->magic != RELMAPPER_FILEMAGIC ||
diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
index 000f3c66d6..a008e0e942 100644
--- a/src/common/controldata_utils.c
+++ b/src/common/controldata_utils.c
@@ -100,9 +100,17 @@ get_controlfile(const char *DataDir, const char *progname, bool *crc_ok_p)
}
#ifndef FRONTEND
- CloseTransientFile(fd);
+ if (CloseTransientFile(fd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not close file \"%s\": %m", ControlFilePath)));
#else
- close(fd);
+ if (close(fd))
+ {
+ fprintf(stderr, _("%s: could not close file \"%s\": %s"),
+ progname, ControlFilePath, strerror(errno));
+ exit(EXIT_FAILURE);
+ }
#endif
/* Check the CRC. */
signature.asc
Description: PGP signature
