Date: Tue, 15 Feb 2022 09:09:52 -0800
From: Andres Freund <andres@anarazel.de>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>,
	"Bossart, Nathan" <bossartn@amazon.com>,
	Tom Lane <tgl@sss.pgh.pa.us>, Julien Rouhaud <rjuju123@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Avoid erroring out when unable to remove or parse logical
 rewrite files to save checkpoint work
Message-ID: <20220215170952.2ruszfrv6qvirmhq@alap3.anarazel.de>
References: <867F2E29-2782-4869-970E-B984C6D35A8F@amazon.com>
 <20220121194956.br76car5qzqhji5s@alap3.anarazel.de>
 <20220127010109.GA352793@nathanxps13>
 <CALj2ACWgKS=ZUJ0xcXhbqieJXMQxncLMQRx6nwtVc9Fp5H58RA@mail.gmail.com>
 <20220201235538.GA740616@nathanxps13>
 <CALj2ACXzMf+8-KoREutAA40MrC4sMEY2P1aLFD_RYysC1M2CZQ@mail.gmail.com>
 <20220202183738.GA746893@nathanxps13>
 <CALj2ACXGwc5UBx1Jc2wacqEM1oBmFHjRHxvTDA+X48sE7ObcFQ@mail.gmail.com>
 <20220204000340.GA1164333@nathanxps13>
 <CALj2ACWRbT2LxQq+7so3NdKest0beoYUaZShDy7OvojcstnQOA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CALj2ACWRbT2LxQq+7so3NdKest0beoYUaZShDy7OvojcstnQOA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20220215170952.2ruszfrv6qvirmhq%40alap3.anarazel.de>
Precedence: bulk

Hi,

On 2022-02-10 21:30:45 +0530, Bharath Rupireddy wrote:
> From 4801ff2c3b1e7bc7076205b676d4e3bc4a4ed308 Mon Sep 17 00:00:00 2001
> From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
> 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.

I think this might be based on a slight misunderstanding / bad phrasing on my
part.  We can use get_dirent_type() to optimize away the lstat on most
platforms, ReadDirExtended itself doesn't do that automatically. I was trying
to reference removing lstat calls by using get_dirent_type() in more places...


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

Then why is this skipping the lstat?


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

I still doubt this is a good idea.

Greetings,

Andres Freund