Date: Wed, 27 Apr 2022 16:09:20 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Greg Stark <stark@mit.edu>, Tom Lane <tgl@sss.pgh.pa.us>,
	Robert Haas <robertmhaas@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: avoid multiple hard links to same WAL file after a crash
Message-ID: <YmjsIGw3SS2zb5Yd@paquier.xyz>
References: <20220407182954.GA1231544@nathanxps13>
 <CA+TgmoZ5VHuv_BzpH7jrgyLu1Kymo=Xom5xFOeGf6xXP9BLhkQ@mail.gmail.com>
 <20220408165312.GB1409034@nathanxps13>
 <CA+Tgmobjke2TBrYk=Zmo=zawkG9A4saAwbe-O5wLHqY9Bz1Fag@mail.gmail.com>
 <Yl0X0+/9lcGzcIui@paquier.xyz>
 <2370127.1650308835@sss.pgh.pa.us>
 <CAM-w4HMOcaApY8qiedkCKRGzfoviLgZrU_Eui=_wM1hV8k=d1Q@mail.gmail.com>
 <20220426200935.GA3175655@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="jhevKCbYrejNZCLR"
Content-Disposition: inline
In-Reply-To: <20220426200935.GA3175655@nathanxps13>
Archived-At: 
 <https://www.postgresql.org/message-id/YmjsIGw3SS2zb5Yd%40paquier.xyz>
Precedence: bulk


--jhevKCbYrejNZCLR
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Apr 26, 2022 at 01:09:35PM -0700, Nathan Bossart wrote:
> Here is an attempt at creating something that can be back-patched.  0001
> simply replaces calls to durable_rename_excl() with durable_rename() and is
> intended to be back-patched.  0002 removes the definition of
> durable_rename_excl() and is _not_ intended for back-patching.  I imagine
> 0002 will need to be held back for v16devel.

I would not mind applying 0002 on HEAD now to avoid more uses of this
API, and I can get behind 0001 after thinking more about it.

> I think back-patching 0001 will encounter a couple of small obstacles.  For
> example, the call in basic_archive won't exist on most of the
> back-branches, and durable_rename_excl() was named durable_link_or_rename()
> before v13.  I don't mind producing a patch for each back-branch if needed.

I am not sure that have any need to backpatch this change based on the
unlikeliness of the problem, TBH.  One thing that is itching me a bit,
like Robert upthread, is that we don't check anymore that the newfile
does not exist in the code paths because we never expect one.  It is
possible to use stat() for that.  But access() within a simple
assertion would be simpler?  Say something like:
Assert(access(path, F_OK) != 0 && errno == ENOENT);

The case for basic_archive is limited as the comment of the patch
states, but that would be helpful for the two calls in timeline.c and
the one in xlog.c in the long-term.  And this has no need to be part
of fd.c, this can be added before the durable_rename() calls.  What do
you think?
--
Michael

--jhevKCbYrejNZCLR
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmJo7CAACgkQnvQgOdby
QH0+kRAAk4vj1B7Zy4IZ2LGHIKPHqOB1q3S6q7vpR4ZT9IZDqEfm3oj0qzq8+RFV
oIBL9Llz/Vo0M54vAFxrAptUyezDhOqRW/tl8VQQMdpeilWKwyVNOid/iHHI+H53
0LCm67ACvEd2yALSBsjQj6iW2Z2Lk49wiy3gYugTKRfJRqJMKYQi6Ck/JXC/uxoe
ocxehGlCDh+8lQYjFNVBTbOqQLCkAzMOaGKOI5yAJ2e5LNkKR0d/0wSD+xBvpzet
TwaflVTHqocLycHDdUq6cVSbqLij8AiMVB3ZxuImZhILNFcsTlVEWQSXFDhNuTy1
m/486djCoowYv9CBbbDH56Rc4yJ7FZHMF/YVb3AwCco/m7sP+IIxAgnQglUSPwah
8MjkLjwPN+Y7CFbUew0dHLKluBZE+yEM4Fi58hOqZ9au4eE/F+lGAfrb5sySys/9
tXkJ1Pwh4mbTukqgMEhvlkmVA9JfzWeODPG4TCRYNcQeNrJlxhY5QVTz3yYFkFNT
QrTn4HA7YkPwbgp7I9sirJPasvUmNvwW+tSaD/tsFyWrInlNEG2LiExOZrNYtKuB
zoej+FM7ZlMXCy+WaR8W7asI5bT/TaPy0aQSzjroKXaIpKkqQbtFCyPdM4kRtGVM
YwwPuJUVX3nvQHeNkdPF79gjsYDdC2pTZdKpz+efNje3IynpnA4=
=0SYO
-----END PGP SIGNATURE-----

--jhevKCbYrejNZCLR--