public inbox for [email protected]
help / color / mirror / Atom feedRe: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work
8+ messages / 3 participants
[nested] [flat]
* Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work
@ 2022-02-01 23:55 Nathan Bossart <[email protected]>
2022-02-02 11:49 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Nathan Bossart @ 2022-02-01 23:55 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Andres Freund <[email protected]>; Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>
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?
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work
2022-02-01 23:55 Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
@ 2022-02-02 11:49 ` Bharath Rupireddy <[email protected]>
2022-02-02 18:37 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Bharath Rupireddy @ 2022-02-02 11:49 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>
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
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work
2022-02-01 23:55 Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
2022-02-02 11:49 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Bharath Rupireddy <[email protected]>
@ 2022-02-02 18:37 ` Nathan Bossart <[email protected]>
2022-02-03 04:15 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Nathan Bossart @ 2022-02-02 18:37 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Andres Freund <[email protected]>; Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Feb 02, 2022 at 05:19:26PM +0530, Bharath Rupireddy wrote:
> On Wed, Feb 2, 2022 at 5:25 AM Nathan Bossart <[email protected]> wrote:
>> 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?
ReadDirExtended() has the following comment:
* 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 there is a problem reading the directory, we will LOG and then exit the
loop. If we didn't scan through all the entries in the directory, there is
a chance that we didn't fsync() all the files that need it.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work
2022-02-01 23:55 Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
2022-02-02 11:49 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Bharath Rupireddy <[email protected]>
2022-02-02 18:37 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
@ 2022-02-03 04:15 ` Bharath Rupireddy <[email protected]>
2022-02-04 00:03 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Bharath Rupireddy @ 2022-02-03 04:15 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Feb 3, 2022 at 12:07 AM Nathan Bossart <[email protected]> wrote:
>
> On Wed, Feb 02, 2022 at 05:19:26PM +0530, Bharath Rupireddy wrote:
> > On Wed, Feb 2, 2022 at 5:25 AM Nathan Bossart <[email protected]> wrote:
> >> 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?
>
> ReadDirExtended() has the following comment:
>
> * 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 there is a problem reading the directory, we will LOG and then exit the
> loop. If we didn't scan through all the entries in the directory, there is
> a chance that we didn't fsync() all the files that need it.
Thanks. I get it. For syncing map files, we don't want to tolerate any
errors, whereas removal of the old map files (lesser than cutoff LSN)
can be tolerated in CheckPointLogicalRewriteHeap.
Here's the v7 version using ReadDir for CheckPointLogicalRewriteHeap.
Regards,
Bharath Rupireddy.
Attachments:
[application/octet-stream] v7-0001-Replace-ReadDir-with-ReadDirExtended.patch (4.1K, ../../CALj2ACXGwc5UBx1Jc2wacqEM1oBmFHjRHxvTDA+X48sE7ObcFQ@mail.gmail.com/2-v7-0001-Replace-ReadDir-with-ReadDirExtended.patch)
download | inline diff:
From aef9d67db637d9e9bd7e9b7c2381e952a891e141 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Thu, 3 Feb 2022 04:13:00 +0000
Subject: [PATCH v7] Replace ReadDir with ReadDirExtended
Replace ReadDir with ReadDirExtended (in CheckPointSnapBuild) and
get rid of lstat entirely. We still use ReadDir in CheckPointLogicalRewriteHeap
because unable to read directory would result a NULL from
ReadDirExtended and we may miss to fsync the remaining map files,
so here let's error out with ReadDir.
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 | 27 ++++++++++++++++-----
src/backend/replication/logical/snapbuild.c | 9 +------
2 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2a53826736..24e7a35881 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -1213,7 +1213,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,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
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work
2022-02-01 23:55 Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
2022-02-02 11:49 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Bharath Rupireddy <[email protected]>
2022-02-02 18:37 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
2022-02-03 04:15 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Bharath Rupireddy <[email protected]>
@ 2022-02-04 00:03 ` Nathan Bossart <[email protected]>
2022-02-10 16:00 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Nathan Bossart @ 2022-02-04 00:03 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Andres Freund <[email protected]>; Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Feb 03, 2022 at 09:45:08AM +0530, Bharath Rupireddy wrote:
> On Thu, Feb 3, 2022 at 12:07 AM Nathan Bossart <[email protected]> wrote:
>> If there is a problem reading the directory, we will LOG and then exit the
>> loop. If we didn't scan through all the entries in the directory, there is
>> a chance that we didn't fsync() all the files that need it.
>
> Thanks. I get it. For syncing map files, we don't want to tolerate any
> errors, whereas removal of the old map files (lesser than cutoff LSN)
> can be tolerated in CheckPointLogicalRewriteHeap.
LGTM. Andres noted upthread [0] that the comment above sscanf() about
skipping editors' lock files might not be accurate. I don't think it's a
huge problem if sscanf() matches those files, but perhaps we can improve
the comment.
[0] https://postgr.es/m/20220120194618.hmfd4kxkng2cgryh%40alap3.anarazel.de
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work
2022-02-01 23:55 Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
2022-02-02 11:49 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Bharath Rupireddy <[email protected]>
2022-02-02 18:37 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
2022-02-03 04:15 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Bharath Rupireddy <[email protected]>
2022-02-04 00:03 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
@ 2022-02-10 16:00 ` Bharath Rupireddy <[email protected]>
2022-02-10 19:39 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Bharath Rupireddy @ 2022-02-10 16:00 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>
On Fri, Feb 4, 2022 at 5:33 AM Nathan Bossart <[email protected]> wrote:
> > Thanks. I get it. For syncing map files, we don't want to tolerate any
> > errors, whereas removal of the old map files (lesser than cutoff LSN)
> > can be tolerated in CheckPointLogicalRewriteHeap.
>
> LGTM. Andres noted upthread [0] that the comment above sscanf() about
> skipping editors' lock files might not be accurate. I don't think it's a
> huge problem if sscanf() matches those files, but perhaps we can improve
> the comment.
>
> [0] https://postgr.es/m/20220120194618.hmfd4kxkng2cgryh%40alap3.anarazel.de
Andres comment from [0]:
> An editor's lock file that starts with map- would presumably be the whole
> filename plus an additional file-ending. But this check won't catch those.
Agreed. sscanf checks can't detect the files named "whole filename
plus an additional file-ending". I just checked with vi editor lock
state file .0-14ED3B8.snap.swp [1], the log generated is [2]. I'm not
sure exactly which editor would create a lockfile like "whole filename
plus an additional file-ending".
In any case, let's remove the editor's lock/state file from those
comments and have just only "We just log a message if a file doesn't
fit the pattern". Attached v8 patch with that change.
[1]
-rw------- 1 bharath bharath 12288 Feb 10 15:48 .0-14ED3B8.snap.swp
-rw------- 1 bharath bharath 128 Feb 10 15:48 0-14ED518.snap
-rw------- 1 bharath bharath 128 Feb 10 15:49 0-14ED518.snap.lockfile
-rw------- 1 bharath bharath 128 Feb 10 15:49 0-14ED550.snap
-rw------- 1 bharath bharath 128 Feb 10 15:49 0-14ED600.snap
[2]
2022-02-10 15:48:47.938 UTC [1121678] LOG: could not parse file name
"pg_logical/snapshots/.0-14ED3B8.snap.swp"
Regards,
Bharath Rupireddy.
Attachments:
[application/octet-stream] v8-0001-Replace-ReadDir-with-ReadDirExtended.patch (4.4K, ../../CALj2ACWRbT2LxQq+7so3NdKest0beoYUaZShDy7OvojcstnQOA@mail.gmail.com/2-v8-0001-Replace-ReadDir-with-ReadDirExtended.patch)
download | inline diff:
From 4801ff2c3b1e7bc7076205b676d4e3bc4a4ed308 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <[email protected]>
Date: Thu, 10 Feb 2022 15:58:58 +0000
Subject: [PATCH v8] Replace ReadDir with ReadDirExtended
Replace ReadDir with ReadDirExtended (in CheckPointSnapBuild) and
get rid of lstat entirely. We still use ReadDir in CheckPointLogicalRewriteHeap
because unable to read directory would result a NULL from
ReadDirExtended and we may miss to fsync the remaining map files,
so here let's error out with ReadDir.
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 | 24 +++++++++++++++------
src/backend/replication/logical/snapbuild.c | 12 ++---------
2 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 2a53826736..035cd6db70 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -1213,7 +1213,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,26 +1226,39 @@ 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. */
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..e2cdf17bee 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,20 +1954,13 @@ 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
* 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...
+ * We just log a message if a file doesn't fit the pattern.
*/
if (sscanf(snap_de->d_name, "%X-%X.snap", &hi, &lo) != 2)
{
--
2.25.1
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work
2022-02-01 23:55 Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
2022-02-02 11:49 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Bharath Rupireddy <[email protected]>
2022-02-02 18:37 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
2022-02-03 04:15 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Bharath Rupireddy <[email protected]>
2022-02-04 00:03 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
2022-02-10 16:00 ` Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Bharath Rupireddy <[email protected]>
@ 2022-02-10 19:39 ` Nathan Bossart <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Nathan Bossart @ 2022-02-10 19:39 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: Andres Freund <[email protected]>; Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; Julien Rouhaud <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Feb 10, 2022 at 09:30:45PM +0530, Bharath Rupireddy wrote:
> In any case, let's remove the editor's lock/state file from those
> comments and have just only "We just log a message if a file doesn't
> fit the pattern". Attached v8 patch with that change.
I've moved this one to ready-for-committer. I was under the impression
that Andres was firmly against this approach, but you did mention there was
an off-list discussion.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic
@ 2024-01-07 19:50 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Melanie Plageman @ 2024-01-07 19:50 UTC (permalink / raw)
To combine the prune and freeze records, freezing must be done before a
pruning WAL record is emitted. We will move the freeze execution into
heap_page_prune() in future commits. lazy_scan_prune() currently
executes freezing, updates vacrel->NewRelfrozenXid and
vacrel->NewRelminMxid, and resets the snapshotConflictHorizon that the
visibility map update record may use in the same block of if statements.
This commit starts reordering that logic so that the freeze execution
can be separated from the other updates which should not be done in
pruning. It also adds a helper calculating freeze snapshot conflict
horizon. This will be useful when the freeze execution is moved into
pruning because not all callers of heap_page_prune() have access to
VacuumCutoffs.
---
src/backend/access/heap/vacuumlazy.c | 112 ++++++++++++++++-----------
1 file changed, 67 insertions(+), 45 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4187c998d25..abbb7ab3ada 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -269,6 +269,8 @@ static void update_vacuum_error_info(LVRelState *vacrel,
static void restore_vacuum_error_info(LVRelState *vacrel,
const LVSavedErrInfo *saved_vacrel);
+static TransactionId heap_frz_conflict_horizon(PruneResult *presult,
+ HeapPageFreeze *pagefrz);
/*
* heap_vacuum_rel() -- perform VACUUM for one heap relation
@@ -1373,6 +1375,33 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
return false;
}
+/*
+ * Determine the snapshotConflictHorizon for freezing. Must only be called
+ * after pruning and determining if the page is freezable.
+ */
+static TransactionId
+heap_frz_conflict_horizon(PruneResult *presult, HeapPageFreeze *pagefrz)
+{
+ TransactionId result;
+
+ /*
+ * We can use frz_conflict_horizon as our cutoff for conflicts when the
+ * whole page is eligible to become all-frozen in the VM once we're done
+ * with it. Otherwise we generate a conservative cutoff by stepping back
+ * from OldestXmin.
+ */
+ if (presult->all_visible_except_removable && presult->all_frozen)
+ result = presult->frz_conflict_horizon;
+ else
+ {
+ /* Avoids false conflicts when hot_standby_feedback in use */
+ result = pagefrz->cutoffs->OldestXmin;
+ TransactionIdRetreat(result);
+ }
+
+ return result;
+}
+
/*
* lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
*
@@ -1421,6 +1450,7 @@ lazy_scan_prune(LVRelState *vacrel,
recently_dead_tuples;
HeapPageFreeze pagefrz;
bool hastup = false;
+ bool do_freeze;
int64 fpi_before = pgWalUsage.wal_fpi;
OffsetNumber deadoffsets[MaxHeapTuplesPerPage];
@@ -1580,10 +1610,15 @@ lazy_scan_prune(LVRelState *vacrel,
* freeze when pruning generated an FPI, if doing so means that we set the
* page all-frozen afterwards (might not happen until final heap pass).
*/
- if (pagefrz.freeze_required || presult.nfrozen == 0 ||
+ do_freeze = pagefrz.freeze_required ||
(presult.all_visible_except_removable && presult.all_frozen &&
- fpi_before != pgWalUsage.wal_fpi))
+ presult.nfrozen > 0 &&
+ fpi_before != pgWalUsage.wal_fpi);
+
+ if (do_freeze)
{
+ TransactionId snapshotConflictHorizon;
+
/*
* We're freezing the page. Our final NewRelfrozenXid doesn't need to
* be affected by the XIDs that are just about to be frozen anyway.
@@ -1591,52 +1626,39 @@ lazy_scan_prune(LVRelState *vacrel,
vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
- if (presult.nfrozen == 0)
- {
- /*
- * We have no freeze plans to execute, so there's no added cost
- * from following the freeze path. That's why it was chosen. This
- * is important in the case where the page only contains totally
- * frozen tuples at this point (perhaps only following pruning).
- * Such pages can be marked all-frozen in the VM by our caller,
- * even though none of its tuples were newly frozen here (note
- * that the "no freeze" path never sets pages all-frozen).
- *
- * We never increment the frozen_pages instrumentation counter
- * here, since it only counts pages with newly frozen tuples
- * (don't confuse that with pages newly set all-frozen in VM).
- */
- }
- else
- {
- TransactionId snapshotConflictHorizon;
+ vacrel->frozen_pages++;
- vacrel->frozen_pages++;
+ snapshotConflictHorizon = heap_frz_conflict_horizon(&presult, &pagefrz);
- /*
- * We can use frz_conflict_horizon as our cutoff for conflicts
- * when the whole page is eligible to become all-frozen in the VM
- * once we're done with it. Otherwise we generate a conservative
- * cutoff by stepping back from OldestXmin.
- */
- if (presult.all_visible_except_removable && presult.all_frozen)
- {
- /* Using same cutoff when setting VM is now unnecessary */
- snapshotConflictHorizon = presult.frz_conflict_horizon;
- presult.frz_conflict_horizon = InvalidTransactionId;
- }
- else
- {
- /* Avoids false conflicts when hot_standby_feedback in use */
- snapshotConflictHorizon = vacrel->cutoffs.OldestXmin;
- TransactionIdRetreat(snapshotConflictHorizon);
- }
+ /* Using same cutoff when setting VM is now unnecessary */
+ if (presult.all_visible_except_removable && presult.all_frozen)
+ presult.frz_conflict_horizon = InvalidTransactionId;
- /* Execute all freeze plans for page as a single atomic action */
- heap_freeze_execute_prepared(vacrel->rel, buf,
- snapshotConflictHorizon,
- presult.frozen, presult.nfrozen);
- }
+ /* Execute all freeze plans for page as a single atomic action */
+ heap_freeze_execute_prepared(vacrel->rel, buf,
+ snapshotConflictHorizon,
+ presult.frozen, presult.nfrozen);
+ }
+ else if (presult.all_frozen && presult.nfrozen == 0)
+ {
+ /* Page should be all visible except to-be-removed tuples */
+ Assert(presult.all_visible_except_removable);
+
+ /*
+ * We have no freeze plans to execute, so there's no added cost from
+ * following the freeze path. That's why it was chosen. This is
+ * important in the case where the page only contains totally frozen
+ * tuples at this point (perhaps only following pruning). Such pages
+ * can be marked all-frozen in the VM by our caller, even though none
+ * of its tuples were newly frozen here (note that the "no freeze"
+ * path never sets pages all-frozen).
+ *
+ * We never increment the frozen_pages instrumentation counter here,
+ * since it only counts pages with newly frozen tuples (don't confuse
+ * that with pages newly set all-frozen in VM).
+ */
+ vacrel->NewRelfrozenXid = pagefrz.FreezePageRelfrozenXid;
+ vacrel->NewRelminMxid = pagefrz.FreezePageRelminMxid;
}
else
{
--
2.40.1
--racicctn4wry6xe5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v3-0007-Execute-freezing-in-heap_page_prune.patch"
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2024-01-07 19:50 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-02-01 23:55 Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work Nathan Bossart <[email protected]>
2022-02-02 11:49 ` Bharath Rupireddy <[email protected]>
2022-02-02 18:37 ` Nathan Bossart <[email protected]>
2022-02-03 04:15 ` Bharath Rupireddy <[email protected]>
2022-02-04 00:03 ` Nathan Bossart <[email protected]>
2022-02-10 16:00 ` Bharath Rupireddy <[email protected]>
2022-02-10 19:39 ` Nathan Bossart <[email protected]>
2024-01-07 19:50 [PATCH v3 06/17] lazy_scan_prune reorder freeze execution logic Melanie Plageman <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox