On Thu, Oct 20, 2022 at 6:57 AM Kyotaro Horiguchi
<horikyota....@gmail.com> wrote:
>
> > The archive module must be responsible for cleaning up the temp file
> > that it creates. One way to do it is in the archive module's shutdown
> > callback, this covers most of the cases, but not all.
>
> True. But I agree to Robert that such temporary files should be
> cleanup-able without needing temporarily-valid knowledge (current file
> name, in this case). A common strategy for this is to name those files
> by names that can be identifed as garbage.
I'm not sure how we can distinguish temp files as garbage based on
name. As Robert pointed out upthread, using system identifier may not
help as the standbys share the same system identifier and it's
possible that they might archive to the same directory. Is there any
other way?
> But since power cut is a typical crash source, we need to identify
> ruined temporary files and the current naming convention is incomplete
> in this regard.
Please note that basic_archive module creates one temp file at a time
to make file copying/moving atomic and it can keep track of the temp
file name and delete it using shutdown callback which helps in most of
the scenarios. As said upthread, repeated crashes while basic_archive
module is atomically copying files around is a big problem in itself
and basic_archive module need not worry about it much.
> flock() on nfs..
>
> The worst case I can come up with regardless of feasibility is a
> multi-standby physical replication set where all hosts share one
> archive directory. Indeed live and dead temprary files can coexist
> there. However, I think we can identify truly rotten temp files by
> inserting host name or cluster name (means cluster_name in
> postgresql.conf) even in that case. This premise that DBA names every
> cluster differently, but I think DBAs that is going to configure such
> a system are required to be very cautious about that kind of aspect.
Well, these ideas are great! However, we can leave defining such
strategies to archive module implementors. IMO, the basich_archive
module ought to be as simple and elegant as possible yet showing up
the usability of archive modules feature.
> Of course, it premised that a cluster uses the same name for a
> segment. If we insert cluseter_name into the temprary name, a starting
> cluster can indentify garbage files to clean up. For example if we
> name them as follows.
>
> ARCHTEMP_cluster1_pid_time_<lsn>
>
> A starting cluster can clean up all files starts with
> "archtemp_cluster1_*". (We need to choose the delimiter carefully,
> though..)
Postgres cleaning up basic_archive modules temp files at the start up
isn't a great idea IMO. Because these files are not related to server
functionality in any way unlike temp files removed in
RemovePgTempFiles(). IMO, we ought to keep the basic_archive module
simple.
.
> No. I didn't mean that, If server stops after a successfull
> durable_rename but before the next call to
> basic_archive_file_internal, that call back makes false comlaint since
> that temprary file is actually gone.
Right. Fixed it.
> > Please see the attached v2 patch.
>
> +static char tempfilepath[MAXPGPATH + 256];
>
> MAXPGPATH is the maximum length of a file name that PG assumes to be
> able to handle. Thus extending that length seems wrong.
I think it was to accommodate the temp file name - "archtemp", file,
MyProcPid, epoch, but I agree that it can just be MAXPGPATH. However,
most of the places the core defines the path name to be MAXPGPATH +
some bytes.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From 591e58e31881d86dc0acc3bd9ca67aeed80a1041 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Thu, 20 Oct 2022 07:12:17 +0000
Subject: [PATCH v3] Remove leftover temporary files via basic_archive shutdown
callback
---
contrib/basic_archive/basic_archive.c | 32 +++++++++++++++++++++++----
doc/src/sgml/basic-archive.sgml | 5 ++++-
2 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 9f221816bb..f971b166be 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;
}
/*
@@ -215,11 +218,12 @@ 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 */
+ MemSet(tempfilepath, '\0', sizeof(tempfilepath));
+
ereport(DEBUG3,
(errmsg("archiving \"%s\" via basic_archive", file)));
@@ -268,21 +272,28 @@ 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);
+
+ /*
+ * Reset tempfilepath after renaming the temporary file to the final file
+ * so that the shutdown callback, if called after this point, will not
+ * attempt to remove it and fail.
+ */
+ MemSet(tempfilepath, '\0', sizeof(tempfilepath));
ereport(DEBUG1,
(errmsg("archived \"%s\" via basic_archive", file)));
@@ -368,3 +379,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)));
+}
diff --git a/doc/src/sgml/basic-archive.sgml b/doc/src/sgml/basic-archive.sgml
index 0b650f17a8..d14bc083af 100644
--- a/doc/src/sgml/basic-archive.sgml
+++ b/doc/src/sgml/basic-archive.sgml
@@ -38,7 +38,10 @@
directory must already exist. The default is an empty string, which
effectively halts WAL archiving, but if <xref linkend="guc-archive-mode"/>
is enabled, the server will accumulate WAL segment files in the
- expectation that a value will soon be provided.
+ expectation that a value will soon be provided. Care must be taken when
+ multiple servers are archiving to the same
+ <varname>basic_archive.archive_library</varname> directory as they all
+ might try to archive the same WAL file.
</para>
</listitem>
</varlistentry>
--
2.34.1
