On Tue, Mar 29, 2022 at 03:48:32PM -0700, Nathan Bossart wrote:
> On Thu, Mar 24, 2022 at 01:17:01PM +1300, Thomas Munro wrote:
>> /* we're only handling directories here, skip if it's not ours */
>> - if (lstat(path, &statbuf) == 0 && !S_ISDIR(statbuf.st_mode))
>> + if (lstat(path, &statbuf) != 0)
>> + ereport(ERROR,
>> + (errcode_for_file_access(),
>> + errmsg("could not stat file \"%s\": %m", path)));
>> + else if (!S_ISDIR(statbuf.st_mode))
>> return;
>>
>> Why is this a good place to silently ignore non-directories?
>> StartupReorderBuffer() is already in charge of skipping random
>> detritus found in the directory, so would it be better to do "if
>> (get_dirent_type(...) != PGFILETYPE_DIR) continue" there, and then
>> drop the lstat() stanza from ReorderBufferCleanupSeralizedTXNs()
>> completely? Then perhaps its ReadDirExtended() shoud be using ERROR
>> instead of INFO, so that missing/non-dir/b0rked directories raise an
>> error.
>
> My guess is that this was done because ReorderBufferCleanupSerializedTXNs()
> is also called from ReorderBufferAllocate() and ReorderBufferFree().
> However, it is odd that we just silently return if the slot path isn't a
> directory in those cases. I think we could use get_dirent_type() in
> StartupReorderBuffer() as you suggested, and then we could let ReadDir()
> ERROR for non-directories for the other callers of
> ReorderBufferCleanupSerializedTXNs(). WDYT?
>
>> I don't understand why it's reporting readdir() errors at INFO
>> but unlink() errors at ERROR, and as far as I can see the other paths
>> that reach this code shouldn't be sending in paths to non-directories
>> here unless something is seriously busted and that's ERROR-worthy.
>
> I agree. I'll switch it to ReadDir() in the next revision so that we ERROR
> instead of INFO.
Here is an updated patch set.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
>From 784419919fca8f157a1c7304b79567b3ba29f3bf Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandboss...@gmail.com>
Date: Tue, 15 Feb 2022 09:40:53 -0800
Subject: [PATCH v11 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 c2d9be81fa..489a8300fa 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -126,6 +126,7 @@
#include "access/xlog_internal.h"
#include "catalog/catalog.h"
#include "commands/sequence.h"
+#include "common/file_utils.h"
#include "lib/binaryheap.h"
#include "miscadmin.h"
#include "pgstat.h"
@@ -4806,15 +4807,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)
{
@@ -4862,6 +4858,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)
@@ -4874,6 +4871,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 ed4c8b3ad5..3748a5f314 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"
@@ -1471,7 +1472,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 ||
@@ -1481,7 +1481,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
>From 5f8a5562664a56fdd16e2983ba4f86692baa5217 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandboss...@gmail.com>
Date: Tue, 15 Feb 2022 09:53:58 -0800
Subject: [PATCH v11 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 489a8300fa..87c4ed3560 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -4812,7 +4812,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
