Date: Mon, 14 Oct 2019 15:02:27 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Andres Freund <andres@anarazel.de>
Cc: Thomas Munro <thomas.munro@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Tighten error control for OpenTransientFile/CloseTransientFile
Message-ID: <20191014060227.GD25169@paquier.xyz>
References: <20190301023338.GD1348@paquier.xyz>
 <20190302004022.GA1602@paquier.xyz>
 <155188409238.16480.11082213460365887952.pgcf@coridan.postgresql.org>
 <20190307015625.GC17293@paquier.xyz>
 <20191004203938.7a34jv4lnrkb56qu@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="cHMo6Wbp1wrKhbfi"
Content-Disposition: inline
In-Reply-To: <20191004203938.7a34jv4lnrkb56qu@alap3.anarazel.de>
Precedence: bulk


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

On Fri, Oct 04, 2019 at 01:39:38PM -0700, Andres Freund wrote:
> but that reasoning seems bogus to me. For one, on just about any
> platform close always closes the fd, even when returning an error
> (unless you pass in a bad fd, in which case it obviously doesn't). So
> the reasoning that this fixes unnoticed fd leaks doesn't really seem to
> make sense. For another, even if it did, throwing an ERROR seems to
> achieve very little: We continue with a leaked fd *AND* we cause the
> operation to error out.

I have read again a couple of times the commit log, and this mentions
to let users know that a fd is leaking, not that it fixes things.
Still we get to know about it, while previously it was not possible.
In some cases we may see errors in close() after a previous write(2).
Of course this does not apply to all the code paths patched here, but
it seems to me that's a good habit to spread, no?

> I can see reasoning for:
> - LOG, so it can be noticed, but operations continue to work
> - FATAL, to fix the leak
> - PANIC, so we recover from the problem, in case of the close indicating
>   a durability issue

LOG or WARNING would not be visible enough and would likely be skipped
by users.  Not sure that this justifies a FATAL either, and PANIC
would cause more harm than necessary, so for most of them ERROR
sounded like a good compromise, still the elevel choice is not
innocent depending on the code paths patched, because the elevel used
is consistent with the error handling of the surroundings.

> And if your goal was just to achieve consistency, I also don't
> understand, because you left plenty close()'s unchecked? E.g. you added
> an error check in get_controlfile(), but not one in
> ReadControlFile(). alterSystemSetConfigFile() writes, but you didn't add
> one.

Because I have not considered these when looking at transient files.
That may be worth an extra lookup.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl2kD3MACgkQnvQgOdby
QH3QSg//essHgoklD7eRnlsRtkMQeDBb/MwNZujVzLI07Fr58NaecOtzOmMUFygk
Uiqp1uNP4rE/CXfwlJRA1BSNKwPI3fAiAeIFxToFQBmhmyeldBnS2hu1au1w5BEz
+Q3VQBIHST+XrymL83Dz63p3RJwUDtGZhIiHdrGe6EH6zc23wqHfDloB39pYGTaL
ra40QaFuzLUmyK0vAvj6Ou/x0BCjOFyNVNycwaOlc9zNF+/Tk9P/0/j1gHZsGU0m
iTOoBAMLgKnO4G2I5BgNdHeuM1xaGjD2DS4FBNquwWZEFp2OPkgqpeC3Upea9mKJ
JjtVUj4+T5C5MalMCo2kPEN0uwM6Sch4lVutpg2ScyUkK0BvmbynWtlv3wmg5MIH
oSO84/3YBYVu/xhlp3LPmTzrJMfVPR2RwTtEyMwWKD88KL1sh0wjKihN6wofeYxi
BYmPT3YJpNzC59GbRAo142lXRcR+3zZJt94/iHyOPQ6LkzQfR+TiTNl+DyKF2YFX
Q3118Vk0uxvUFcnY3kYIjrwpxTFAI7iiWYJuTCx6gjA7bXiD3EqSVLztVUqgzj9F
URzA0buutEFR44vh17V057jUTmWx2k5WwOpcMtD40eSoKjllX54pOW4cxRQhIRFX
ckpP9sgilsSCofQfLvcepNF9XqxlOyxYXm+W110hXMXYPAq9yJs=
=bnrN
-----END PGP SIGNATURE-----

--cHMo6Wbp1wrKhbfi--