From: Michael Paquier <michael@paquier.xyz>
To: Andres Freund <andres@anarazel.de>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Tighten error control for OpenTransientFile/CloseTransientFile
Date: Mon, 14 Oct 2019 15:02:27 +0900
Message-ID: <20191014060227.GD25169@paquier.xyz> (raw)
In-Reply-To: <20191004203938.7a34jv4lnrkb56qu@alap3.anarazel.de>
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>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../20191014060227.GD25169@paquier.xyz/2-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael@paquier.xyz, andres@anarazel.de, thomas.munro@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Tighten error control for OpenTransientFile/CloseTransientFile
In-Reply-To: <20191014060227.GD25169@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox