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

Thomas Munro <thomas.munro@gmail.com>

From: Thomas Munro <thomas.munro@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Nathan Bossart <nathandbossart@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Andres Freund <andres@anarazel.de>, "Bossart, Nathan" <bossartn@amazon.com>, Julien Rouhaud <rjuju123@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2022-08-09T03:41:14Z
Lists: pgsql-hackers
On Tue, Aug 9, 2022 at 3:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Actually, having now read the patch, I don't think there is any
> part of 0002 that is a good idea.  It's blithely removing the
> comments that explain why the existing coding is the way it is,
> and not providing a shred of justification for making checkpoints
> more brittle.

0002 also contradicts the original $SUBJECT and goal of this thread,
which is possibly why it was kept separate.  I was only thinking of
committing 0001 myself, which is the one I'd reviewed an earlier
version of.

> I have not tried to analyze the error-handling properties of 0001,
> but if it's being equally cavalier then it shouldn't be committed
> either.  Most of this behavior is the result of decades of hard-won
> experience; discarding it because it doesn't fit conveniently
> into some refactoring plan isn't smart.

0001 does introduce new errors, as mentioned in the commit message, in
the form of elevel ERROR passed into get_dirent_type(), which might be
thrown if your OS has no d_type and lstat() fails (also if you asked
to follow symlinks, but in those cases errors were already thrown).
But in those cases, it seems at least a little fishy that we ignored
errors from the existing lstat().  I wondered if that was because they
expected that any failure meant ENOENT and they wanted to tolerate
that, but that does not seem to be the case, so I considered the error
to be an improvement.



Commits

  1. Expand the use of get_dirent_type(), shaving a few calls to stat()/lstat()

  2. fsync pg_logical/mappings in CheckPointLogicalRewriteHeap().

  3. Introduce logical decoding.