On Tue, Mar 27, 2018 at 01:32:41AM +0900, Fujii Masao wrote: > +1. It's better for us to focus on the code change of the fillter on pg_rewind > rather than such "refactoring".
(filter takes one 'l', not two) Okay. I had my mind mostly focused on how to shape the exclusion list and get it shared between the base backup and pg_rewind, so let's move on with the duplicated list for now. I did not put much efforts into the pg_rewind portion to be honest. > As I told upthread, the previous patch has the > problem where the files which should be skipped are not skipped. ISTM that, > in pg_rewind, the filter should be triggered in recurse_dir() not > process_source_file(). If you put that into recurse_dir you completely ignore the case where changes are fetched by libpq. Doing the filtering when processing the file map has the advantage to take care of both the local and remote cases, which is why I am doing it there. So you would just get half of the cake and not the whole of it. > BTW what should pg_rewind do when it finds the directory which should be > skipped, in the source directory? In your patch, pg_rewind just tries to skip > that directory at all. But isn't this right behavior? If that directory > doesn't > exist in the target directory (though I'm not sure if this situation is > really > possible), I'm thinking that pg_rewind should create that "empty" directory > in the target. No? I am not exactly sure what you are coming up with here. The target server should have the same basic directory mapping as the source as the target has been initialized normally with initdb or a base backup from another node, so checking for the *contents* of directories is enough and keeps the code more simple, as the exclude filter entries are based on elements inherent to PostgreSQL internals. Please note as well that if a non-system directory is present on the source but not the target then it would get created on the target. At the end I have finished with the attached. I have taken the decision to not include as well xlog.h in pg_rewind to avoid having to drag a lot of backend-only headers like pg_resetwal does, which I prefer avoid as that's only hardcoding values for "backup_label" and "tablespace_map". This applies filters based on directory contents, so by running the regression tests you can see entries like the following ones: entry "postmaster.opts" excluded from source file list entry "pg_subtrans/0000" excluded from source file list entry "pg_notify/0000" excluded from source file list entry "base/12360/pg_internal.init" excluded from source file list entry "backup_label.old" excluded from source file list entry "global/pg_internal.init" excluded from source file list entry "postmaster.opts" excluded from target file list entry "pg_subtrans/0000" excluded from target file list entry "pg_notify/0000" excluded from target file list entry "base/12360/pg_internal.init" excluded from target file list entry "global/pg_internal.init" excluded from target file list Processing the filemap list on the target also matters in my opinion. When at recovery, all the previous files will be wiped out, and we should not remove either things like postmaster.pid as those are around to prevent corruption problems. Thanks, -- Michael
From ba4502c9711c7b7933a5ef2186e895c0ae6b4616 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 5 Feb 2018 15:48:35 +0900
Subject: [PATCH] Add exclude list similar to base backups in pg_rewind
After being rewound, a standby to-be-recycled needs to perform recovery
from the last checkpoint where WAL forked after a promotion, which leads
it to automatically remove some files which may have been copied from
the source cluster. This makes use of the same filtering list as base
backups to find out what is this data and then remove it. This reduces
the amount of data transferred during a rewind without changing the
usefulness of the operation.
Documentation is updated to take into account what is filtered out.
---
doc/src/sgml/ref/pg_rewind.sgml | 14 +++-
src/backend/replication/basebackup.c | 3 +
src/bin/pg_rewind/filemap.c | 144 ++++++++++++++++++++++++++++++++---
3 files changed, 148 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 8e49249826..520d843f0e 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -231,7 +231,19 @@ PostgreSQL documentation
<para>
Copy all other files such as <filename>pg_xact</filename> and
configuration files from the source cluster to the target cluster
- (everything except the relation files).
+ (everything except the relation files). Similarly to base backups,
+ the contents of the directories <filename>pg_dynshmem/</filename>,
+ <filename>pg_notify/</filename>, <filename>pg_replslot/</filename>,
+ <filename>pg_serial/</filename>, <filename>pg_snapshots/</filename>,
+ <filename>pg_stat_tmp/</filename>, and
+ <filename>pg_subtrans/</filename> are omitted from the data copied
+ from the source cluster. Any file or directory beginning with
+ <filename>pgsql_tmp</filename> is omitted, as well as are
+ <filename>backup_label</filename>,
+ <filename>tablespace_map</filename>,
+ <filename>pg_internal.init</filename>,
+ <filename>postmaster.opts</filename> and
+ <filename>postmaster.pid</filename>.
</para>
</step>
<step>
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index e4c45c5025..902a553ffa 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -103,6 +103,9 @@ static TimestampTz throttled_last;
* The contents of these directories are removed or recreated during server
* start so they are not included in backups. The directories themselves are
* kept and included as empty to preserve access permissions.
+ *
+ * Note: this list should be kept in sync with the filter lists in pg_rewind's
+ * filemap.c.
*/
static const char *excludeDirContents[] =
{
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index 6122f177fe..67b01cbdfe 100644
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -31,6 +31,83 @@ static char *datasegpath(RelFileNode rnode, ForkNumber forknum,
static int path_cmp(const void *a, const void *b);
static int final_filemap_cmp(const void *a, const void *b);
static void filemap_list_to_array(filemap_t *map);
+static bool is_path_excluded(const char *path, const char *type);
+
+/*
+ * The contents of these directories are removed or recreated during server
+ * start so they are not included in data processed by pg_rewind.
+ *
+ * Note: those lists should be kept in sync with what basebackup.c provides.
+ * Some of the values, contrary to what basebackup.c uses, are hardcoded as
+ * they are defined in backend-only headers. So this list is maintained
+ * with a best effort in mind.
+ */
+static const char *excludeDirContents[] =
+{
+ /*
+ * Skip temporary statistics files. PG_STAT_TMP_DIR must be skipped even
+ * when stats_temp_directory is set because PGSS_TEXT_FILE is always
+ * created there.
+ */
+ "pg_stat_tmp", /* defined as PG_STAT_TMP_DIR */
+
+ /*
+ * It is generally not useful to backup the contents of this directory
+ * even if the intention is to restore to another master. See backup.sgml
+ * for a more detailed description.
+ */
+ "pg_replslot",
+
+ /* Contents removed on startup, see dsm_cleanup_for_mmap(). */
+ "pg_dynshmem", /* defined as PG_DYNSHMEM_DIR */
+
+ /* Contents removed on startup, see AsyncShmemInit(). */
+ "pg_notify",
+
+ /*
+ * Old contents are loaded for possible debugging but are not required for
+ * normal operation, see OldSerXidInit().
+ */
+ "pg_serial",
+
+ /* Contents removed on startup, see DeleteAllExportedSnapshotFiles(). */
+ "pg_snapshots",
+
+ /* Contents zeroed on startup, see StartupSUBTRANS(). */
+ "pg_subtrans",
+
+ /* end of list */
+ NULL
+};
+
+/*
+ * List of files excluded from filemap processing.
+ */
+static const char *excludeFiles[] =
+{
+ /* Skip auto conf temporary file. */
+ "postgresql.auto.conf.tmp", /* defined as PG_AUTOCONF_FILENAME */
+
+ /* Skip current log file temporary file */
+ "current_logfiles.tmp", /* defined as LOG_METAINFO_DATAFILE_TMP */
+
+ /* Skip relation cache because it is rebuilt on startup */
+ "pg_internal.init", /* defined as RELCACHE_INIT_FILENAME */
+
+ /*
+ * If there's a backup_label or tablespace_map file, it belongs to a
+ * backup started by the user with pg_start_backup(). It is *not* correct
+ * for this backup. Our backup_label is written later on separately.
+ */
+ "backup_label", /* defined as BACKUP_LABEL_FILE */
+ "tablespace_map", /* defined as TABLESPACE_MAP */
+
+ "postmaster.pid",
+ "postmaster.opts",
+
+ /* end of list */
+ NULL
+};
/*
* Create a new file map (stored in the global pointer "filemap").
@@ -71,11 +148,8 @@ process_source_file(const char *path, file_type_t type, size_t newsize,
Assert(map->array == NULL);
- /*
- * Completely ignore some special files in source and destination.
- */
- if (strcmp(path, "postmaster.pid") == 0 ||
- strcmp(path, "postmaster.opts") == 0)
+ /* ignore any path matching the exclusion filters */
+ if (is_path_excluded(path, "source"))
return;
/*
@@ -260,6 +334,14 @@ process_target_file(const char *path, file_type_t type, size_t oldsize,
filemap_t *map = filemap;
file_entry_t *entry;
+ /*
+ * Ignore any path matching the exclusion filters. This is not actually
+ * mandatory for target files, but this does not hurt and let's be
+ * consistent with the source processing.
+ */
+ if (is_path_excluded(path, "target"))
+ return;
+
snprintf(localpath, sizeof(localpath), "%s/%s", datadir_target, path);
if (lstat(localpath, &statbuf) < 0)
{
@@ -286,13 +368,6 @@ process_target_file(const char *path, file_type_t type, size_t oldsize,
qsort(map->array, map->narray, sizeof(file_entry_t *), path_cmp);
}
- /*
- * Completely ignore some special files
- */
- if (strcmp(path, "postmaster.pid") == 0 ||
- strcmp(path, "postmaster.opts") == 0)
- return;
-
/*
* Like in process_source_file, pretend that xlog is always a directory.
*/
@@ -412,6 +487,51 @@ process_block_change(ForkNumber forknum, RelFileNode rnode, BlockNumber blkno)
}
}
+/*
+ * Completely ignore some special files in source and destination. This
+ * filters willingly any files matching an entry in the list of files to
+ * filter out. The input path is a relative path from a data directory,
+ * so comparisons need to compromise with that and do not check for
+ * slashes before the compared names. This does not matter in practice
+ * as files listed in the exclusion list do not have names mapping with
+ * existing PostgreSQL internal paths.
+ */
+static bool
+is_path_excluded(const char *path, const char *type)
+{
+ char localpath[MAXPGPATH];
+ int excludeIdx;
+
+ /* check individual files... */
+ for (excludeIdx = 0; excludeFiles[excludeIdx] != NULL; excludeIdx++)
+ {
+ if (strstr(path, excludeFiles[excludeIdx]) != NULL)
+ {
+ pg_log(PG_DEBUG, "entry \"%s\" excluded from %s file list\n",
+ path, type);
+ return true;
+ }
+ }
+
+ /*
+ * ... And check some directories. Note that this includes any contents
+ * within the directories themselves.
+ */
+ for (excludeIdx = 0; excludeDirContents[excludeIdx] != NULL; excludeIdx++)
+ {
+ snprintf(localpath, sizeof(localpath), "%s/",
+ excludeDirContents[excludeIdx]);
+ if (strstr(path, localpath) != NULL)
+ {
+ pg_log(PG_DEBUG, "entry \"%s\" excluded from %s file list\n",
+ path, type);
+ return true;
+ }
+ }
+
+ return false;
+}
+
/*
* Convert the linked list of entries in map->first/last to the array,
* map->array.
--
2.16.3
signature.asc
Description: PGP signature
