On Fri, Apr 12, 2019 at 10:06:41PM +0900, Masahiko Sawada wrote: > But I think that's not right, I've checked the code. If the startup > process failed in that function it raises a FATAL and recovery fails, > and if checkpointer process does then it calls > pgstat_report_wait_end() in CheckpointerMain().
Well, the point is that the code raises an ERROR, then a FATAL because it gets upgraded by recovery. The take, at least it seems to me, is that if any new caller of the function misses to clean up the event then the routine gets cleared. So it seems to me that the current coding is aimed to be more defensive than anything. I agree that there is perhaps little point in doing so. In my experience a backend switches very quickly back to ClientRead, cleaning up the previous event. Looking around, we have also some code paths in slot.c and origin.c which close a transient file, clear the event flag... And then PANIC, which makes even less sense. In short, I tend to think that the attached is an acceptable cleanup. Thoughts? -- Michael
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index f9a4960f8a..a399c0052d 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1693,26 +1693,18 @@ RecreateTwoPhaseFile(TransactionId xid, void *content, int len)
pgstat_report_wait_start(WAIT_EVENT_TWOPHASE_FILE_WRITE);
if (write(fd, content, len) != len)
{
- int save_errno = errno;
-
- pgstat_report_wait_end();
- CloseTransientFile(fd);
-
/* if write didn't set errno, assume problem is no disk space */
- errno = save_errno ? save_errno : ENOSPC;
+ if (errno == 0)
+ errno = ENOSPC;
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not write file \"%s\": %m", path)));
}
if (write(fd, &statefile_crc, sizeof(pg_crc32c)) != sizeof(pg_crc32c))
{
- int save_errno = errno;
-
- pgstat_report_wait_end();
- CloseTransientFile(fd);
-
/* if write didn't set errno, assume problem is no disk space */
- errno = save_errno ? save_errno : ENOSPC;
+ if (errno == 0)
+ errno = ENOSPC;
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not write file \"%s\": %m", path)));
@@ -1725,15 +1717,9 @@ RecreateTwoPhaseFile(TransactionId xid, void *content, int len)
*/
pgstat_report_wait_start(WAIT_EVENT_TWOPHASE_FILE_SYNC);
if (pg_fsync(fd) != 0)
- {
- int save_errno = errno;
-
- CloseTransientFile(fd);
- errno = save_errno;
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not fsync file \"%s\": %m", path)));
- }
pgstat_report_wait_end();
if (CloseTransientFile(fd) != 0)
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index 42fd8f5b6b..ff4d54d6ed 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -579,12 +579,9 @@ CheckPointReplicationOrigin(void)
errno = 0;
if ((write(tmpfd, &magic, sizeof(magic))) != sizeof(magic))
{
- int save_errno = errno;
-
- CloseTransientFile(tmpfd);
-
/* if write didn't set errno, assume problem is no disk space */
- errno = save_errno ? save_errno : ENOSPC;
+ if (errno == 0)
+ errno = ENOSPC;
ereport(PANIC,
(errcode_for_file_access(),
errmsg("could not write to file \"%s\": %m",
@@ -624,12 +621,9 @@ CheckPointReplicationOrigin(void)
if ((write(tmpfd, &disk_state, sizeof(disk_state))) !=
sizeof(disk_state))
{
- int save_errno = errno;
-
- CloseTransientFile(tmpfd);
-
/* if write didn't set errno, assume problem is no disk space */
- errno = save_errno ? save_errno : ENOSPC;
+ if (errno == 0)
+ errno = ENOSPC;
ereport(PANIC,
(errcode_for_file_access(),
errmsg("could not write to file \"%s\": %m",
@@ -646,12 +640,9 @@ CheckPointReplicationOrigin(void)
errno = 0;
if ((write(tmpfd, &crc, sizeof(crc))) != sizeof(crc))
{
- int save_errno = errno;
-
- CloseTransientFile(tmpfd);
-
/* if write didn't set errno, assume problem is no disk space */
- errno = save_errno ? save_errno : ENOSPC;
+ if (errno == 0)
+ errno = ENOSPC;
ereport(PANIC,
(errcode_for_file_access(),
errmsg("could not write to file \"%s\": %m",
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 006446b663..057c5d7ab2 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1398,16 +1398,10 @@ RestoreSlotFromDisk(const char *name)
*/
pgstat_report_wait_start(WAIT_EVENT_REPLICATION_SLOT_RESTORE_SYNC);
if (pg_fsync(fd) != 0)
- {
- int save_errno = errno;
-
- CloseTransientFile(fd);
- errno = save_errno;
ereport(PANIC,
(errcode_for_file_access(),
errmsg("could not fsync file \"%s\": %m",
path)));
- }
pgstat_report_wait_end();
/* Also sync the parent directory */
@@ -1421,10 +1415,6 @@ RestoreSlotFromDisk(const char *name)
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(),
@@ -1466,10 +1456,6 @@ RestoreSlotFromDisk(const char *name)
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(),
diff --git a/src/backend/storage/file/copydir.c b/src/backend/storage/file/copydir.c
index 342d078a8f..30f6200a86 100644
--- a/src/backend/storage/file/copydir.c
+++ b/src/backend/storage/file/copydir.c
@@ -199,7 +199,6 @@ copy_file(char *fromfile, char *tofile)
pgstat_report_wait_start(WAIT_EVENT_COPY_FILE_WRITE);
if ((int) write(dstfd, buffer, nbytes) != nbytes)
{
- pgstat_report_wait_end();
/* if write didn't set errno, assume problem is no disk space */
if (errno == 0)
errno = ENOSPC;
signature.asc
Description: PGP signature
