public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nathan Bossart <[email protected]>
To: Andres Freund <[email protected]>
Cc: Bossart, Nathan <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Bharath Rupireddy <[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: Wed, 26 Jan 2022 17:01:09 -0800
Message-ID: <20220127010109.GA352793@nathanxps13> (raw)
In-Reply-To: <[email protected]>
References: <CALj2ACWUHL-pjFEVzPpxjauUCML2_i4u2k-=pmYmnRwc7zJoPQ@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
On Fri, Jan 21, 2022 at 11:49:56AM -0800, Andres Freund wrote:
> On 2022-01-20 20:41:16 +0000, Bossart, Nathan wrote:
>> Here's this part.
>
> And pushed to all branches. Thanks.
Thanks!
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
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com/
Attachments:
[text/x-diff] v4-0001-add-error-checking-for-calls-to-lstat-in-replicat.patch (3.3K, ../20220127010109.GA352793@nathanxps13/2-v4-0001-add-error-checking-for-calls-to-lstat-in-replicat.patch)
download | inline diff:
From df7a07d27c2858ec3ac383335af3e55e5428e3b1 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 26 Jan 2022 14:03:11 -0800
Subject: [PATCH v4 1/2] add error checking for calls to lstat() in replication
code
---
src/backend/access/heap/rewriteheap.c | 6 +++++-
src/backend/replication/logical/reorderbuffer.c | 6 +++++-
src/backend/replication/logical/snapbuild.c | 6 +++++-
src/backend/replication/slot.c | 6 +++++-
4 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2a53826736..81319e0c78 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -1227,7 +1227,11 @@ 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 (lstat(path, &statbuf) != 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not stat file \"%s\": %m", path)));
+ else if (!S_ISREG(statbuf.st_mode))
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 19b2ba2000..d8d784a42f 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -4407,7 +4407,11 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
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))
+ 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;
spill_dir = AllocateDir(path);
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 83fca8a77d..57f5a5e81f 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1955,7 +1955,11 @@ CheckPointSnapBuild(void)
snprintf(path, sizeof(path), "pg_logical/snapshots/%s", snap_de->d_name);
- if (lstat(path, &statbuf) == 0 && !S_ISREG(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_ISREG(statbuf.st_mode))
{
elog(DEBUG1, "only regular files expected: %s", path);
continue;
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index e5e0cf8768..21fb7536e2 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1413,7 +1413,11 @@ 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 (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))
continue;
/* we crashed while a slot was being setup or deleted, clean up */
--
2.25.1
[text/x-diff] v4-0002-minor-improvements-to-replication-code.patch (2.3K, ../20220127010109.GA352793@nathanxps13/3-v4-0002-minor-improvements-to-replication-code.patch)
download | inline diff:
From 28a06a2cb84102a92da3fca0f0055dd67902d7de Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 26 Jan 2022 16:24:04 -0800
Subject: [PATCH v4 2/2] minor improvements to replication code
---
src/backend/replication/logical/snapbuild.c | 19 ++-----------------
src/backend/replication/slot.c | 5 +----
2 files changed, 3 insertions(+), 21 deletions(-)
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 57f5a5e81f..ce6cb85c1a 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1970,16 +1970,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;
@@ -1987,20 +1981,11 @@ CheckPointSnapBuild(void)
if (lsn < cutoff || cutoff == InvalidXLogRecPtr)
{
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);
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 21fb7536e2..37ec3fa633 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1424,12 +1424,9 @@ StartupReplicationSlots(void)
if (pg_str_endswith(replication_de->d_name, ".tmp"))
{
if (!rmtree(path, true))
- {
- ereport(WARNING,
+ ereport(ERROR,
(errmsg("could not remove directory \"%s\"",
path)));
- continue;
- }
fsync_fname("pg_replslot", true);
continue;
}
--
2.25.1
view thread (17+ 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]
Subject: Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work
In-Reply-To: <20220127010109.GA352793@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