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

On Tue, Aug 09, 2022 at 05:26:39PM +1200, Thomas Munro wrote:
> The changes were not from elog-LOG to elog-ERROR, they were changes
> from silently eating errors, but I take your (plural) general point.

I think this was in reference to 0002.  My comment was, at least.

> OK, so there aren't many places in 0001 that error out where we
> previously did not.
> 
> For the one in CheckPointLogicalRewriteHeap(), if it fails, you don't
> just want to skip -- it might be in the range that we need to fsync().
> So what if we just deleted that get_dirent_type() != PGFILETYPE_REG
> check altogether?  Depending on the name range check, we'd either
> attempt to unlink() and LOG if that fails, or we'd attempt to
> open()-or-ERROR and then fsync()-or-PANIC.

It looks like CheckPointLogicalRewriteHeap() presently ERRORs when unlink()
fails.  However, CheckPointSnapBuild() only LOGs when unlink() fails.
Since mappings files contain transaction IDs, persistent unlink() failures
might lead to wraparound, but I don't think failing checkpoints will help
improve matters.

Bharath's original patch changed CheckPointLogicalRewriteHeap() to LOG on
sscanf() and unlink() failures.  IIUC things would work the way you suggest
if that was applied.

> What functionality have we
> lost without the DT_REG check?  Well, now if someone ill-advisedly
> puts an important character device, socket, symlink (ie any kind of
> non-DT_REG) into that directory with a name conforming to the
> unlinkable range, we'll try to unlink it and possibly succeed, where
> previously we'd have skipped, and if it's in the checkpointable range,
> we'd try to fsync() it and likely fail, which is good because this is
> a supposed to be a checkpoint.

I don't know that I agree that the last part is good.  Presumably we don't
want checkpointing to fail forever because there's a random non-regular
file that can't be fsync'd.  But it's not clear why such files would exist
in the first place.  Presumably this isn't the sort of thing that Postgres
should be expected to support.

> In other words, if we're going to LOG and press on, maybe we should
> just press on anyway.  Would the error message be any less
> informative?  Would the risk of unlinking a character device that you
> accidentally stored in
> /clusters/pg16-main/pgdata/pg_logical/mappings/map-1234-5678 be a
> problem for anyone?

Probably not.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com