From cd805d6d26e66dcfc51ca6632a0147827a5b9544 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Wed, 9 Nov 2022 08:29:19 +0000 Subject: [PATCH v5] Refactor copy_file() to remove leftover temp files in basic_archive --- contrib/basic_archive/basic_archive.c | 23 ++++++++++- src/backend/storage/file/copydir.c | 56 ++++++++++++++++++++------- src/backend/storage/file/reinit.c | 2 +- src/include/storage/copydir.h | 2 +- 4 files changed, 66 insertions(+), 17 deletions(-) diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c index 9f221816bb..1b9f48eeed 100644 --- a/contrib/basic_archive/basic_archive.c +++ b/contrib/basic_archive/basic_archive.c @@ -275,14 +275,33 @@ basic_archive_file_internal(const char *file, const char *path) * Copy the file to its temporary destination. Note that this will fail * if temp already exists. */ - copy_file(unconstify(char *, path), temp); + if (copy_file(unconstify(char *, path), temp, LOG) != 0) + { + /* Remove the leftover temporary file. */ + if (errno != EEXIST) + unlink(temp); + + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not copy file \"%s\" to temporary file \"%s\": %m", + path, temp))); + } /* * Sync the temporary file to disk and move it to its final destination. * Note that this will overwrite any existing file, but this is only * possible if someone else created the file since the stat() above. */ - (void) durable_rename(temp, destination, ERROR); + if (durable_rename(temp, destination, LOG) != 0) + { + /* Remove the leftover temporary file. */ + unlink(temp); + + ereport(ERROR, + (errcode_for_file_access(), + errmsg("could not rename temporary file \"%s\" to \"%s\": %m", + temp, destination))); + } ereport(DEBUG1, (errmsg("archived \"%s\" via basic_archive", file))); diff --git a/src/backend/storage/file/copydir.c b/src/backend/storage/file/copydir.c index 0cea4cbc89..ee20c83949 100644 --- a/src/backend/storage/file/copydir.c +++ b/src/backend/storage/file/copydir.c @@ -71,7 +71,7 @@ copydir(char *fromdir, char *todir, bool recurse) copydir(fromfile, tofile, true); } else if (xlde_type == PGFILETYPE_REG) - copy_file(fromfile, tofile); + copy_file(fromfile, tofile, ERROR); } FreeDir(xldir); @@ -113,8 +113,8 @@ copydir(char *fromdir, char *todir, bool recurse) /* * copy one file */ -void -copy_file(char *fromfile, char *tofile) +int +copy_file(char *fromfile, char *tofile, int elevel) { char *buffer; int srcfd; @@ -138,28 +138,35 @@ copy_file(char *fromfile, char *tofile) #define FLUSH_DISTANCE (1024 * 1024) #endif - /* Use palloc to ensure we get a maxaligned buffer */ - buffer = palloc(COPY_BUF_SIZE); - /* * Open the files */ srcfd = OpenTransientFile(fromfile, O_RDONLY | PG_BINARY); if (srcfd < 0) - ereport(ERROR, + { + ereport(elevel, (errcode_for_file_access(), errmsg("could not open file \"%s\": %m", fromfile))); + return -1; + } dstfd = OpenTransientFile(tofile, O_RDWR | O_CREAT | O_EXCL | PG_BINARY); if (dstfd < 0) - ereport(ERROR, + { + ereport(elevel, (errcode_for_file_access(), errmsg("could not create file \"%s\": %m", tofile))); + return -1; + } /* * Do the data copying. */ flush_offset = 0; + + /* Use palloc to ensure we get a maxaligned buffer */ + buffer = palloc(COPY_BUF_SIZE); + for (offset = 0;; offset += nbytes) { /* If we got a cancel signal during the copy of the file, quit */ @@ -180,37 +187,60 @@ copy_file(char *fromfile, char *tofile) nbytes = read(srcfd, buffer, COPY_BUF_SIZE); pgstat_report_wait_end(); if (nbytes < 0) - ereport(ERROR, + { + ereport(elevel, (errcode_for_file_access(), errmsg("could not read file \"%s\": %m", fromfile))); + pfree(buffer); + CloseTransientFile(srcfd); + CloseTransientFile(dstfd); + return -1; + } if (nbytes == 0) break; errno = 0; 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; - ereport(ERROR, + ereport(elevel, (errcode_for_file_access(), errmsg("could not write to file \"%s\": %m", tofile))); + + pfree(buffer); + CloseTransientFile(srcfd); + CloseTransientFile(dstfd); + return -1; } pgstat_report_wait_end(); } + pfree(buffer); + if (offset > flush_offset) pg_flush_data(dstfd, flush_offset, offset - flush_offset); if (CloseTransientFile(dstfd) != 0) - ereport(ERROR, + { + ereport(elevel, (errcode_for_file_access(), errmsg("could not close file \"%s\": %m", tofile))); + CloseTransientFile(srcfd); + return -1; + } + if (CloseTransientFile(srcfd) != 0) - ereport(ERROR, + { + ereport(elevel, (errcode_for_file_access(), errmsg("could not close file \"%s\": %m", fromfile))); + return -1; + } - pfree(buffer); + return 0; } diff --git a/src/backend/storage/file/reinit.c b/src/backend/storage/file/reinit.c index 647c458b52..0a12e57eb8 100644 --- a/src/backend/storage/file/reinit.c +++ b/src/backend/storage/file/reinit.c @@ -312,7 +312,7 @@ ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op) /* OK, we're ready to perform the actual copy. */ elog(DEBUG2, "copying %s to %s", srcpath, dstpath); - copy_file(srcpath, dstpath); + copy_file(srcpath, dstpath, ERROR); } FreeDir(dbspace_dir); diff --git a/src/include/storage/copydir.h b/src/include/storage/copydir.h index 50a26edeb0..68fbbd12a3 100644 --- a/src/include/storage/copydir.h +++ b/src/include/storage/copydir.h @@ -14,6 +14,6 @@ #define COPYDIR_H extern void copydir(char *fromdir, char *todir, bool recurse); -extern void copy_file(char *fromfile, char *tofile); +extern int copy_file(char *fromfile, char *tofile, int elevel); #endif /* COPYDIR_H */ -- 2.34.1