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
