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
signature.asc
Description: PGP signature
