public inbox for [email protected]
help / color / mirror / Atom feedFrom: Bharath Rupireddy <[email protected]>
To: Nathan Bossart <[email protected]>
Cc: Andres Freund <[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: Wed, 2 Feb 2022 17:19:26 +0530
Message-ID: <CALj2ACXzMf+8-KoREutAA40MrC4sMEY2P1aLFD_RYysC1M2CZQ@mail.gmail.com> (raw)
In-Reply-To: <20220201235538.GA740616@nathanxps13>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<20220127010109.GA352793@nathanxps13>
<CALj2ACWgKS=ZUJ0xcXhbqieJXMQxncLMQRx6nwtVc9Fp5H58RA@mail.gmail.com>
<20220201235538.GA740616@nathanxps13>
On Wed, Feb 2, 2022 at 5:25 AM Nathan Bossart <[email protected]> wrote:
>
> On Mon, Jan 31, 2022 at 10:42:54AM +0530, Bharath Rupireddy wrote:
> > 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.
>
> I think removing the lstat() is probably reasonable. We currently aren't
> doing proper error checking, and the chances of a non-regular file matching
> the prefix are likely pretty low. In the worst case, we'll LOG or ERROR
> when unlinking or fsyncing fails.
>
> However, I'm not sure about the change to ReadDirExtended(). That might be
> okay for CheckPointSnapBuild(), which is just trying to remove old files,
> but CheckPointLogicalRewriteHeap() is responsible for ensuring that files
> are flushed to disk for the checkpoint. If we stop reading the directory
> after an error and let the checkpoint continue, isn't it possible that some
> mappings files won't be persisted to disk?
Unless I mis-read your above statement, with LOG level in
ReadDirExtended, I don't think we stop reading the files in
CheckPointLogicalRewriteHeap. Am I missing something here?
Since, we also continue in CheckPointLogicalRewriteHeap if we can't
parse/delete some files with the change of "could not parse
filename"/"could not remove file" messages to LOG level
I'm attaching v6, just changed elog(LOG, to ereport(LOG in
CheckPointLogicalRewriteHeap, other things remain the same.
Regards,
Bharath Rupireddy.
Attachments:
[application/octet-stream] v6-0001-Replace-ReadDir-with-ReadDirExtended.patch (4.0K, ../CALj2ACXzMf+8-KoREutAA40MrC4sMEY2P1aLFD_RYysC1M2CZQ@mail.gmail.com/2-v6-0001-Replace-ReadDir-with-ReadDirExtended.patch)
download | inline diff:
From 0716e03efb41a7d99872889cafbed802e326e92a Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Wed, 2 Feb 2022 11:47:10 +0000
Subject: [PATCH v6] 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 | 29 ++++++++++++++++-----
src/backend/replication/logical/snapbuild.c | 9 +------
2 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2a53826736..b028b98eaa 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,42 @@ 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);
+ {
+ ereport(LOG,
+ (errmsg("could not parse file name \"%s\"", path)));
+ 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)));
+ 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
view thread (8+ 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: <CALj2ACXzMf+8-KoREutAA40MrC4sMEY2P1aLFD_RYysC1M2CZQ@mail.gmail.com>
* 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