Re: Incorrect errno used with %m for backend code

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