MIME-Version: 1.0
References: <20220215170952.2ruszfrv6qvirmhq@alap3.anarazel.de>
 <20220215175752.GA2413813@nathanxps13>
 <20220215181034.nyhd6fwzyg3vgxls@alap3.anarazel.de>
 <20220215183758.GA2435469@nathanxps13> <20220215231123.GA2584239@nathanxps13>
 <CA+hUKGKFZ3GH8OXDDewamRNU5LBxkham=RVv3xTZd1kcqs5i-A@mail.gmail.com>
 <20220329224832.GA560657@nathanxps13> <20220330162008.GA784267@nathanxps13>
 <20220408201857.GA1586536@nathanxps13>
 <CALj2ACXKYq4q6xinUNwA=ttQK_5QeSR3oxk6rLv5sW_9Hb28tg@mail.gmail.com>
 <20220708171439.GB2356733@nathanxps13>
In-Reply-To: <20220708171439.GB2356733@nathanxps13>
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Mon, 18 Jul 2022 16:53:18 +0530
Message-ID: 
 <CALj2ACUYMuO027XNJNsBfaPUhA+6fC=S0HjasQ4BpUQt0W9eSA@mail.gmail.com>
Subject: Re: Avoid erroring out when unable to remove or parse logical rewrite
 files to save checkpoint work
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Thomas Munro <thomas.munro@gmail.com>, Andres Freund <andres@anarazel.de>,
	"Bossart, Nathan" <bossartn@amazon.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	Julien Rouhaud <rjuju123@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CALj2ACUYMuO027XNJNsBfaPUhA%2B6fC%3DS0HjasQ4BpUQt0W9eSA%40mail.gmail.com>
Precedence: bulk

On Fri, Jul 8, 2022 at 10:44 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> > 0002 - I'm not quite happy with this patch, with the change,
> > checkpoint errors out, if it can't remove just a file - the comments
> > there says it all. Is there any strong reason for this change?
>
> Andres noted several concerns upthread.  In short, ignoring unexpected
> errors makes them harder to debug and likely masks bugs.
>
> FWIW I agree that it is unfortunate that a relatively non-critical error
> here leads to checkpoint failures, which can cause much worse problems down
> the road.  I think this is one of the reasons for moving tasks like this
> out of the checkpointer, as I'm trying to do with the proposed custodian
> process [0].
>
> [0] https://commitfest.postgresql.org/38/3448/

IMHO, we can keep it as-is and if required can be changed as part of
the patch set [0], as this change without [0] can cause a checkpoint
to fail. Furthermore, I would like it if we convert "could not parse
filename" and "could not remove file" ERRORs of
CheckPointLogicalRewriteHeap to LOGs until [0] gets in - others may
have different opinions though.

Just wondering - do we ever have a problem if we can't remove the
snapshot or mapping file?

May be unrelated, RemoveTempXlogFiles doesn't even bother to check if
the temp wal file is removed.

Regards,
Bharath Rupireddy.