On Mon, Oct 17, 2022 at 6:45 PM Robert Haas <robertmh...@gmail.com> wrote: > > On Fri, Oct 14, 2022 at 4:45 AM Bharath Rupireddy > <bharath.rupireddyforpostg...@gmail.com> wrote: > > What happens to the left-over temp files after a server crash? Will > > they be lying around in the archive directory? I understand that we > > can't remove such files because we can't distinguish left-over files > > from a crash and the temp files that another server is in the process > > of copying. > > Yeah, leaving a potentially unbounded number of files around after > system crashes seems pretty unfriendly. I'm not sure how to fix that, > exactly.
A simple server restart while the basic_archive module is copying to/from temp file would leave the file behind, see[1]. I think we can fix this by defining shutdown_cb for the basic_archive module, like the attached patch. While this helps in most of the crashes, but not all. However, this is better than what we have right now. [1] ubuntu:~/postgres/contrib/basic_archive/archive_directory$ ls 000000010000000000000001 archtemp.000000010000000000000002.2493876.1666091933457 archtemp.000000010000000000000002.2495316.1666091958680 -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
From cc5450714c8f1624546bf575589cae9562c5db8e Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Date: Tue, 18 Oct 2022 12:54:59 +0000 Subject: [PATCH v1] Remove leftover temporary files via basic_archive shutdown callback --- contrib/basic_archive/basic_archive.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c index 9f221816bb..91b6af18bf 100644 --- a/contrib/basic_archive/basic_archive.c +++ b/contrib/basic_archive/basic_archive.c @@ -42,10 +42,12 @@ PG_MODULE_MAGIC; static char *archive_directory = NULL; static MemoryContext basic_archive_context; +static char tempfilepath[MAXPGPATH + 256]; static bool basic_archive_configured(void); static bool basic_archive_file(const char *file, const char *path); static void basic_archive_file_internal(const char *file, const char *path); +static void basic_archive_shutdown(void); static bool check_archive_directory(char **newval, void **extra, GucSource source); static bool compare_files(const char *file1, const char *file2); @@ -85,6 +87,7 @@ _PG_archive_module_init(ArchiveModuleCallbacks *cb) cb->check_configured_cb = basic_archive_configured; cb->archive_file_cb = basic_archive_file; + cb->shutdown_cb = basic_archive_shutdown; } /* @@ -151,6 +154,8 @@ basic_archive_file(const char *file, const char *path) sigjmp_buf local_sigjmp_buf; MemoryContext oldcontext; + MemSet(tempfilepath, '\0', sizeof(tempfilepath)); + /* * We run basic_archive_file_internal() in our own memory context so that * we can easily reset it during error recovery (thus avoiding memory @@ -215,7 +220,6 @@ static void basic_archive_file_internal(const char *file, const char *path) { char destination[MAXPGPATH]; - char temp[MAXPGPATH + 256]; struct stat st; struct timeval tv; uint64 epoch; /* milliseconds */ @@ -268,21 +272,21 @@ basic_archive_file_internal(const char *file, const char *path) pg_add_u64_overflow(epoch, (uint64) (tv.tv_usec / 1000), &epoch)) elog(ERROR, "could not generate temporary file name for archiving"); - snprintf(temp, sizeof(temp), "%s/%s.%s.%d." UINT64_FORMAT, + snprintf(tempfilepath, sizeof(tempfilepath), "%s/%s.%s.%d." UINT64_FORMAT, archive_directory, "archtemp", file, MyProcPid, epoch); /* * Copy the file to its temporary destination. Note that this will fail * if temp already exists. */ - copy_file(unconstify(char *, path), temp); + copy_file(unconstify(char *, path), tempfilepath); /* * 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); + (void) durable_rename(tempfilepath, destination, ERROR); ereport(DEBUG1, (errmsg("archived \"%s\" via basic_archive", file))); @@ -368,3 +372,16 @@ compare_files(const char *file1, const char *file2) return ret; } + +static void +basic_archive_shutdown(void) +{ + if (tempfilepath[0] == '\0') + return; + + /* Remove the leftover temporary file. */ + if (unlink(tempfilepath) < 0) + ereport(WARNING, + (errcode_for_file_access(), + errmsg("could not unlink file \"%s\": %m", tempfilepath))); +} -- 2.34.1