On Thu, Jan 27, 2022 at 6:31 AM Nathan Bossart <nathandboss...@gmail.com> wrote:
>
> I spent some time thinking about the right way to proceed here, and I came
> up with the attached patches. The first patch just adds error checking for
> various lstat() calls in the replication code. If lstat() fails, then it
> probably doesn't make sense to try to continue processing the file.
>
> The second patch changes some nearby calls to ereport() to ERROR. If these
> failures are truly unexpected, and we don't intend to support use-cases
> like concurrent manual deletion, then failing might be the right way to go.
> I think it's a shame that such failures could cause checkpointing to
> continually fail, but that topic is already being discussed elsewhere [0].
>
> [0] https://postgr.es/m/C1EE64B0-D4DB-40F3-98C8-0CED324D34CB%40amazon.com
After an off-list discussion with Andreas, proposing here a patch that
basically replaces ReadDir call with ReadDirExtended and gets rid of
lstat entirely. With this chance, the checkpoint will only care about
the snapshot and mapping files and not fail if it finds other files in
the directories. Removing lstat enables us to make things faster as we
avoid a bunch of extra system calls - one lstat call per each mapping
or snapshot file.
This patch also includes, converting "could not parse filename" and
"could not remove file" errors to LOG messages in
CheckPointLogicalRewriteHeap. This will enable checkpoint not to waste
the amount of work that it has done in case it is unable to
parse/remove the file, for whatever reasons.
Regards,
Bharath Rupireddy.
From b5358ad720caa4af5fe8635920ab5853afd5a327 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Mon, 31 Jan 2022 05:05:22 +0000
Subject: [PATCH v5] Replace ReadDir with ReadDirExtended
Replace ReadDir with ReadDirExtended and get rid of lstat entirely.
With this change, the checkpoint will only care about the snapshot
and mapping files and not fail if it finds other files in the
directories.
Removing lstat enables us to make things faster as we avoid extra
system calls.
Also, convert "could not parse filename" and "could not remove file"
errors to LOG messages in CheckPointLogicalRewriteHeap. This will
enable checkpoint not to waste the amount of work that it had done.
---
src/backend/access/heap/rewriteheap.c | 25 ++++++++++++++++-----
src/backend/replication/logical/snapbuild.c | 9 +-------
2 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2a53826736..2cb72a2a5c 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -1211,9 +1211,8 @@ CheckPointLogicalRewriteHeap(void)
cutoff = redo;
mappings_dir = AllocateDir("pg_logical/mappings");
- while ((mapping_de = ReadDir(mappings_dir, "pg_logical/mappings")) != NULL)
+ while ((mapping_de = ReadDirExtended(mappings_dir, "pg_logical/mappings", LOG)) != NULL)
{
- struct stat statbuf;
Oid dboid;
Oid relid;
XLogRecPtr lsn;
@@ -1227,26 +1226,40 @@ 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))
- continue;
/* Skip over files that cannot be ours. */
if (strncmp(mapping_de->d_name, "map-", 4) != 0)
continue;
+ /*
+ * 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(mapping_de->d_name, LOGICAL_REWRITE_FORMAT,
&dboid, &relid, &hi, &lo, &rewrite_xid, &create_xid) != 6)
- elog(ERROR, "could not parse filename \"%s\"", mapping_de->d_name);
+ {
+ elog(LOG, "could not parse filename \"%s\"", mapping_de->d_name);
+ continue;
+ }
lsn = ((uint64) hi) << 32 | lo;
if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
{
elog(DEBUG1, "removing logical rewrite file \"%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(ERROR,
+ {
+ ereport(LOG,
(errcode_for_file_access(),
errmsg("could not remove file \"%s\": %m", path)));
+ continue;
+ }
}
else
{
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 83fca8a77d..a4bf7a7fd9 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1942,12 +1942,11 @@ CheckPointSnapBuild(void)
cutoff = redo;
snap_dir = AllocateDir("pg_logical/snapshots");
- while ((snap_de = ReadDir(snap_dir, "pg_logical/snapshots")) != NULL)
+ while ((snap_de = ReadDirExtended(snap_dir, "pg_logical/snapshots", LOG)) != NULL)
{
uint32 hi;
uint32 lo;
XLogRecPtr lsn;
- struct stat statbuf;
if (strcmp(snap_de->d_name, ".") == 0 ||
strcmp(snap_de->d_name, "..") == 0)
@@ -1955,12 +1954,6 @@ CheckPointSnapBuild(void)
snprintf(path, sizeof(path), "pg_logical/snapshots/%s", snap_de->d_name);
- if (lstat(path, &statbuf) == 0 && !S_ISREG(statbuf.st_mode))
- {
- elog(DEBUG1, "only regular files expected: %s", path);
- continue;
- }
-
/*
* temporary filenames from SnapBuildSerialize() include the LSN and
* everything but are postfixed by .$pid.tmp. We can just remove them
--
2.25.1
