Subject: Re: Avoid erroring out when unable to remove or parse logical rewrite files to save checkpoint work

From: Nathan Bossart <[email-masked]>
Date: Tue, 1 Feb 2022 15:55:38 -0800

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

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 2 Feb 2022 17:19:26 +0530

> 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.

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 2 Feb 2022 10:37:38 -0800

> 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.

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 3 Feb 2022 09:45:08 +0530

> 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.

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 3 Feb 2022 16:03:40 -0800

> 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

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 10 Feb 2022 21:30:45 +0530

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.

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 10 Feb 2022 11:39:30 -0800

> 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.