public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nathan Bossart <[email protected]>
To: Thomas Munro <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: Bossart, Nathan <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Julien Rouhaud <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work
Date: Fri, 8 Apr 2022 13:18:57 -0700
Message-ID: <20220408201857.GA1586536@nathanxps13> (raw)
In-Reply-To: <20220330162008.GA784267@nathanxps13>
References: <20220204000340.GA1164333@nathanxps13>
<CALj2ACWRbT2LxQq+7so3NdKest0beoYUaZShDy7OvojcstnQOA@mail.gmail.com>
<[email protected]>
<20220215175752.GA2413813@nathanxps13>
<[email protected]>
<20220215183758.GA2435469@nathanxps13>
<20220215231123.GA2584239@nathanxps13>
<CA+hUKGKFZ3GH8OXDDewamRNU5LBxkham=RVv3xTZd1kcqs5i-A@mail.gmail.com>
<20220329224832.GA560657@nathanxps13>
<20220330162008.GA784267@nathanxps13>
On Wed, Mar 30, 2022 at 09:21:30AM -0700, Nathan Bossart wrote:
> Here is an updated patch set.
rebased
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
[text/x-diff] v12-0001-make-more-use-of-get_dirent_type.patch (12.3K, ../20220408201857.GA1586536@nathanxps13/2-v12-0001-make-more-use-of-get_dirent_type.patch)
download | inline diff:
From a92ccf47c9c334eea5b8d07b8fcab7031181c37e Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Tue, 15 Feb 2022 09:40:53 -0800
Subject: [PATCH v12 1/2] make more use of get_dirent_type()
---
src/backend/access/heap/rewriteheap.c | 4 +-
.../replication/logical/reorderbuffer.c | 12 +++---
src/backend/replication/logical/snapbuild.c | 5 +--
src/backend/replication/slot.c | 4 +-
src/backend/storage/file/copydir.c | 21 +++-------
src/backend/storage/file/fd.c | 20 +++++----
src/backend/utils/misc/guc-file.l | 42 +++++++------------
src/timezone/pgtz.c | 8 +---
8 files changed, 48 insertions(+), 68 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2a53826736..d64d7aae2e 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -113,6 +113,7 @@
#include "access/xact.h"
#include "access/xloginsert.h"
#include "catalog/catalog.h"
+#include "common/file_utils.h"
#include "lib/ilist.h"
#include "miscadmin.h"
#include "pgstat.h"
@@ -1213,7 +1214,6 @@ CheckPointLogicalRewriteHeap(void)
mappings_dir = AllocateDir("pg_logical/mappings");
while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
{
- struct stat statbuf;
Oid dboid;
Oid relid;
XLogRecPtr lsn;
@@ -1227,7 +1227,7 @@ CheckPointLogicalRewriteHeap(void)
continue;
snprintf(path, sizeof(path), "pg_logical/mappings/%s", mapping_de->d_name);
- if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
+ if (get_dirent_type(path, mapping_de, false, ERROR) != PGFILETYPE_REG)
continue;
/* Skip over files that cannot be ours. */
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 5adc016d44..63ef55f3f7 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -91,6 +91,7 @@
#include "access/xact.h"
#include "access/xlog_internal.h"
#include "catalog/catalog.h"
+#include "common/file_utils.h"
#include "lib/binaryheap.h"
#include "miscadmin.h"
#include "pgstat.h"
@@ -4408,15 +4409,10 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
{
DIR *spill_dir;
struct dirent *spill_de;
- struct stat statbuf;
char path[MAXPGPATH * 2 + 12];
sprintf(path, "pg_replslot/%s", slotname);
- /* we're only handling directories here, skip if it's not ours */
- if (lstat(path, &statbuf) == 0 && !S_ISDIR(statbuf.st_mode))
- return;
-
spill_dir = AllocateDir(path);
while ((spill_de = ReadDirExtended(spill_dir, path, INFO)) != NULL)
{
@@ -4464,6 +4460,7 @@ StartupReorderBuffer(void)
{
DIR *logical_dir;
struct dirent *logical_de;
+ char path[MAXPGPATH * 2 + 12];
logical_dir = AllocateDir("pg_replslot");
while ((logical_de = ReadDir(logical_dir, "pg_replslot")) != NULL)
@@ -4476,6 +4473,11 @@ StartupReorderBuffer(void)
if (!ReplicationSlotValidateName(logical_de->d_name, DEBUG2))
continue;
+ /* we're only handling directories here, skip if it's not ours */
+ sprintf(path, "pg_replslot/%s", logical_de->d_name);
+ if (get_dirent_type(path, logical_de, false, ERROR) != PGFILETYPE_DIR)
+ continue;
+
/*
* ok, has to be a surviving logical slot, iterate and delete
* everything starting with xid-*
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 83fca8a77d..df50abfd98 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -123,6 +123,7 @@
#include "access/heapam_xlog.h"
#include "access/transam.h"
#include "access/xact.h"
+#include "common/file_utils.h"
#include "miscadmin.h"
#include "pgstat.h"
#include "replication/logical.h"
@@ -1947,15 +1948,13 @@ CheckPointSnapBuild(void)
uint32 hi;
uint32 lo;
XLogRecPtr lsn;
- struct stat statbuf;
if (strcmp(snap_de->d_name, ".") == 0 ||
strcmp(snap_de->d_name, "..") == 0)
continue;
snprintf(path, sizeof(path), "pg_logical/snapshots/%s", snap_de->d_name);
-
- if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
+ if (get_dirent_type(path, snap_de, false, ERROR) != PGFILETYPE_REG)
{
elog(DEBUG1, "only regular files expected: %s", path);
continue;
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index c35ea7c35b..5ee68e71b8 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -41,6 +41,7 @@
#include "access/transam.h"
#include "access/xlog_internal.h"
+#include "common/file_utils.h"
#include "common/string.h"
#include "miscadmin.h"
#include "pgstat.h"
@@ -1485,7 +1486,6 @@ StartupReplicationSlots(void)
replication_dir = AllocateDir("pg_replslot");
while ((replication_de = ReadDir(replication_dir, "pg_replslot")) != NULL)
{
- struct stat statbuf;
char path[MAXPGPATH + 12];
if (strcmp(replication_de->d_name, ".") == 0 ||
@@ -1495,7 +1495,7 @@ StartupReplicationSlots(void)
snprintf(path, sizeof(path), "pg_replslot/%s", replication_de->d_name);
/* we're only creating directories here, skip if it's not our's */
- if (lstat(path, &statbuf) == 0 && !S_ISDIR(statbuf.st_mode))
+ if (get_dirent_type(path, replication_de, false, ERROR) != PGFILETYPE_DIR)
continue;
/* we crashed while a slot was being setup or deleted, clean up */
diff --git a/src/backend/storage/file/copydir.c b/src/backend/storage/file/copydir.c
index 658fd95ba9..8a866191e1 100644
--- a/src/backend/storage/file/copydir.c
+++ b/src/backend/storage/file/copydir.c
@@ -22,6 +22,7 @@
#include <unistd.h>
#include <sys/stat.h>
+#include "common/file_utils.h"
#include "miscadmin.h"
#include "pgstat.h"
#include "storage/copydir.h"
@@ -50,7 +51,7 @@ copydir(char *fromdir, char *todir, bool recurse)
while ((xlde = ReadDir(xldir, fromdir)) != NULL)
{
- struct stat fst;
+ PGFileType xlde_type;
/* If we got a cancel signal during the copy of the directory, quit */
CHECK_FOR_INTERRUPTS();
@@ -62,18 +63,15 @@ copydir(char *fromdir, char *todir, bool recurse)
snprintf(fromfile, sizeof(fromfile), "%s/%s", fromdir, xlde->d_name);
snprintf(tofile, sizeof(tofile), "%s/%s", todir, xlde->d_name);
- if (lstat(fromfile, &fst) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not stat file \"%s\": %m", fromfile)));
+ xlde_type = get_dirent_type(fromfile, xlde, false, ERROR);
- if (S_ISDIR(fst.st_mode))
+ if (xlde_type == PGFILETYPE_DIR)
{
/* recurse to handle subdirectories */
if (recurse)
copydir(fromfile, tofile, true);
}
- else if (S_ISREG(fst.st_mode))
+ else if (xlde_type == PGFILETYPE_REG)
copy_file(fromfile, tofile);
}
FreeDir(xldir);
@@ -89,8 +87,6 @@ copydir(char *fromdir, char *todir, bool recurse)
while ((xlde = ReadDir(xldir, todir)) != NULL)
{
- struct stat fst;
-
if (strcmp(xlde->d_name, ".") == 0 ||
strcmp(xlde->d_name, "..") == 0)
continue;
@@ -101,12 +97,7 @@ copydir(char *fromdir, char *todir, bool recurse)
* We don't need to sync subdirectories here since the recursive
* copydir will do it before it returns
*/
- if (lstat(tofile, &fst) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not stat file \"%s\": %m", tofile)));
-
- if (S_ISREG(fst.st_mode))
+ if (get_dirent_type(tofile, xlde, false, ERROR) == PGFILETYPE_REG)
fsync_fname(tofile, false);
}
FreeDir(xldir);
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 14b77f2861..4a4a79b40a 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -2783,6 +2783,10 @@ TryAgain:
*
* The pathname passed to AllocateDir must be passed to this routine too,
* but it is only used for error reporting.
+ *
+ * If you need to discover the file type of an entry, consider using
+ * get_dirent_type() (instead of stat()/lstat()) to avoid extra system calls on
+ * many platforms.
*/
struct dirent *
ReadDir(DIR *dir, const char *dirname)
@@ -2798,6 +2802,10 @@ ReadDir(DIR *dir, const char *dirname)
* If elevel < ERROR, returns NULL after any error. With the normal coding
* pattern, this will result in falling out of the loop immediately as
* though the directory contained no (more) entries.
+ *
+ * If you need to discover the file type of an entry, consider using
+ * get_dirent_type() (instead of stat()/lstat()) to avoid extra system calls on
+ * many platforms.
*/
struct dirent *
ReadDirExtended(DIR *dir, const char *dirname, int elevel)
@@ -3234,17 +3242,11 @@ RemovePgTempFilesInDir(const char *tmpdirname, bool missing_ok, bool unlink_all)
PG_TEMP_FILE_PREFIX,
strlen(PG_TEMP_FILE_PREFIX)) == 0)
{
- struct stat statbuf;
+ PGFileType type = get_dirent_type(rm_path, temp_de, false, LOG);
- if (lstat(rm_path, &statbuf) < 0)
- {
- ereport(LOG,
- (errcode_for_file_access(),
- errmsg("could not stat file \"%s\": %m", rm_path)));
+ if (type == PGFILETYPE_ERROR)
continue;
- }
-
- if (S_ISDIR(statbuf.st_mode))
+ else if (type == PGFILETYPE_DIR)
{
/* recursively remove contents, then directory itself */
RemovePgTempFilesInDir(rm_path, false, true);
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index c70543fa74..8f10252fa4 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -14,6 +14,7 @@
#include <ctype.h>
#include <unistd.h>
+#include "common/file_utils.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
#include "storage/fd.h"
@@ -1018,7 +1019,7 @@ ParseConfigDirectory(const char *includedir,
while ((de = ReadDir(d, directory)) != NULL)
{
- struct stat st;
+ PGFileType de_type;
char filename[MAXPGPATH];
/*
@@ -1035,32 +1036,9 @@ ParseConfigDirectory(const char *includedir,
join_path_components(filename, directory, de->d_name);
canonicalize_path(filename);
- if (stat(filename, &st) == 0)
+ de_type = get_dirent_type(filename, de, true, elevel);
+ if (de_type == PGFILETYPE_ERROR)
{
- if (!S_ISDIR(st.st_mode))
- {
- /* Add file to array, increasing its size in blocks of 32 */
- if (num_filenames >= size_filenames)
- {
- size_filenames += 32;
- filenames = (char **) repalloc(filenames,
- size_filenames * sizeof(char *));
- }
- filenames[num_filenames] = pstrdup(filename);
- num_filenames++;
- }
- }
- else
- {
- /*
- * stat does not care about permissions, so the most likely reason
- * a file can't be accessed now is if it was removed between the
- * directory listing and now.
- */
- ereport(elevel,
- (errcode_for_file_access(),
- errmsg("could not stat file \"%s\": %m",
- filename)));
record_config_file_error(psprintf("could not stat file \"%s\"",
filename),
calling_file, calling_lineno,
@@ -1068,6 +1046,18 @@ ParseConfigDirectory(const char *includedir,
status = false;
goto cleanup;
}
+ else if (de_type != PGFILETYPE_DIR)
+ {
+ /* Add file to array, increasing its size in blocks of 32 */
+ if (num_filenames >= size_filenames)
+ {
+ size_filenames += 32;
+ filenames = (char **) repalloc(filenames,
+ size_filenames * sizeof(char *));
+ }
+ filenames[num_filenames] = pstrdup(filename);
+ num_filenames++;
+ }
}
if (num_filenames > 0)
diff --git a/src/timezone/pgtz.c b/src/timezone/pgtz.c
index 3c278dd0f3..72f9e3d5e6 100644
--- a/src/timezone/pgtz.c
+++ b/src/timezone/pgtz.c
@@ -17,6 +17,7 @@
#include <sys/stat.h>
#include <time.h>
+#include "common/file_utils.h"
#include "datatype/timestamp.h"
#include "miscadmin.h"
#include "pgtz.h"
@@ -429,7 +430,6 @@ pg_tzenumerate_next(pg_tzenum *dir)
{
struct dirent *direntry;
char fullname[MAXPGPATH * 2];
- struct stat statbuf;
direntry = ReadDir(dir->dirdesc[dir->depth], dir->dirname[dir->depth]);
@@ -447,12 +447,8 @@ pg_tzenumerate_next(pg_tzenum *dir)
snprintf(fullname, sizeof(fullname), "%s/%s",
dir->dirname[dir->depth], direntry->d_name);
- if (stat(fullname, &statbuf) != 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not stat \"%s\": %m", fullname)));
- if (S_ISDIR(statbuf.st_mode))
+ if (get_dirent_type(fullname, direntry, true, ERROR) == PGFILETYPE_DIR)
{
/* Step into the subdirectory */
if (dir->depth >= MAX_TZDIR_DEPTH - 1)
--
2.25.1
[text/x-diff] v12-0002-minor-improvements-to-replication-code.patch (2.4K, ../20220408201857.GA1586536@nathanxps13/3-v12-0002-minor-improvements-to-replication-code.patch)
download | inline diff:
From 96b3c44bf9850b106751facd1e7d2d77543fe9d4 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Tue, 15 Feb 2022 09:53:58 -0800
Subject: [PATCH v12 2/2] minor improvements to replication code
---
.../replication/logical/reorderbuffer.c | 2 +-
src/backend/replication/logical/snapbuild.c | 18 ++----------------
2 files changed, 3 insertions(+), 17 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 63ef55f3f7..532c3e4630 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -4414,7 +4414,7 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
sprintf(path, "pg_replslot/%s", slotname);
spill_dir = AllocateDir(path);
- while ((spill_de = ReadDirExtended(spill_dir, path, INFO)) != NULL)
+ while ((spill_de = ReadDir(spill_dir, path)) != NULL)
{
/* only look at names that can be ours */
if (strncmp(spill_de->d_name, "xid", 3) == 0)
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index df50abfd98..6a60e942b7 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1965,16 +1965,10 @@ CheckPointSnapBuild(void)
* everything but are postfixed by .$pid.tmp. We can just remove them
* the same as other files because there can be none that are
* currently being written that are older than cutoff.
- *
- * We just log a message if a file doesn't fit the pattern, it's
- * probably some editors lock/state file or similar...
*/
if (sscanf(snap_de->d_name, "%X-%X.snap", &hi, &lo) != 2)
- {
- ereport(LOG,
+ ereport(ERROR,
(errmsg("could not parse file name \"%s\"", path)));
- continue;
- }
lsn = ((uint64) hi) << 32 | lo;
@@ -1983,19 +1977,11 @@ CheckPointSnapBuild(void)
{
elog(DEBUG1, "removing snapbuild snapshot %s", path);
- /*
- * It's not particularly harmful, though strange, if we can't
- * remove the file here. Don't prevent the checkpoint from
- * completing, that'd be a cure worse than the disease.
- */
if (unlink(path) < 0)
- {
- ereport(LOG,
+ ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not remove file \"%s\": %m",
path)));
- continue;
- }
}
}
FreeDir(snap_dir);
--
2.25.1
view thread (40+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work
In-Reply-To: <20220408201857.GA1586536@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox