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