Date: Mon, 2 May 2022 10:36:57 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, tgl@sss.pgh.pa.us,
	robertmhaas@gmail.com, pgsql-hackers@postgresql.org
Subject: Re: avoid multiple hard links to same WAL file after a crash
Message-ID: <20220502173657.GA3351307@nathanxps13>
References: 
 <CA+TgmobGi7b-5fjAWgEO0ebn516n0OyOi-j7+Hyz_j9sAG17og@mail.gmail.com>
 <289699.1649694527@sss.pgh.pa.us>
 <20220411165257.GB1915258@nathanxps13>
 <20220412.154631.417529439388886590.horikyota.ntt@gmail.com>
 <20220412162742.GB2062175@nathanxps13>
 <Ym6GZbqQdlalSKSG@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Ym6GZbqQdlalSKSG@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/20220502173657.GA3351307%40nathanxps13>
Precedence: bulk

On Sun, May 01, 2022 at 10:08:53PM +0900, Michael Paquier wrote:
> At the end, switching directly from durable_rename_excl() to
> durable_rename() should be fine for the WAL segment initialization,
> but we could do things a bit more carefully by adding a check on the
> file existence before calling durable_rename() and issue a elog(LOG)
> if a file is found, giving a mean for the WAL recycling to give up
> peacefully as it does now.  Per my analysis, the TLI history file
> created at the end of recovery ought to issue an elog(ERROR).

My only concern with this approach is that it inevitably introduces a race
condition.  In most cases, the file existence check will prevent
overwrites, but it might not always.  Furthermore, we believe that such
overwrites either 1) should not happen (e.g., WAL recycling) or 2) won't
cause problems if they happen (e.g., when the WAL receiver writes the TLI
history file).  Also, these races will be difficult to test, so we won't
know what breaks when they occur.

My instinct is to just let the overwrites happen.  That way, we are more
likely to catch breakage in tests, and we'll have one less race condition
to worry about.  I don't mind asserting that the file doesn't exist when we
don't expect it to, as that might help catch potential problems in
development without affecting behavior in production.  If we do want to
add file existence checks, I think we'd better add a comment about the
potential for race conditions.

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