On Fri, Jun 22, 2018 at 11:15:53AM -0400, Alvaro Herrera wrote: > I wondered for a bit if it would be a better idea to have > CloseTransientFile restore the existing errno if there is any and close > works fine -- when I noticed that that function itself says that caller > should check errno for close() errors. Most callers seem to do it > correctly, but a few fail to check the return value.
Yeah, the API in its current shape is simpler to understand. Once you get used to the errno stanza of course... > A bunch of other places use CloseTransientFile while closing shop after > some other syscall failure, which is what your patch fixes. I didn't > review those; checking for close failure seems pointless. Agreed. > In some cases, we fsync the file and check that return value, then close > the file and do *not* check CloseTransientFile's return value -- > examples are CheckPointLogicalRewriteHeap, heap_xlog_logical_rewrite, > SnapBuildSerialize, SaveSlotToPath, durable_rename. I don't know if > it's reasonable for close() to fail after successfully fsyncing a file; > maybe this is not a problem. I would patch those nonetheless. And writeTimeLineHistory. > be_lo_export() is certainly missing a check: it writes and closes, > without intervening fsync. One problem at the same time if possible :) I think that those adjustments should be a separate patch. -- Michael
signature.asc
Description: PGP signature