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
