On 2018-Jun-22, Michael Paquier wrote:
> A couple of places use CloseTransientFile without bothering much that
> this can overwrite errno. I was tempted in keeping errno saved and kept
> if set to a non-zero value, but refrained from doing so as some callers
> may rely on the existing behavior, and the attached shows better the
> intention.
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.
Quite some places open files O_RDONLY, so lack of error checking after
closing seems ok. (Unless there's some funny interaction with the fsync
fiasco, 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.
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.
be_lo_export() is certainly missing a check: it writes and closes,
without intervening fsync.
I don't understand the logic in RestoreSlotFromDisk that fsync the file
prior to reading it. What are we protecting against?
Anyway, while I think it would be nice to have CloseTransientFile
restore the original errno if there was one and close goes well, it
probably unduly complicates its API contract.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services