Re: client-side fsync() error handling

On Thu, Feb 13, 2020 at 10:02:31AM +0100, Peter Eisentraut wrote:
> On 2020-02-12 06:28, Michael Paquier wrote:
>> Now, don't we need to care about durable_rename() and make the
>> panic-like failure an optional choice? From what I can see, this
>> routine is used now in the backend for pg_basebackup to rename
>> temporary history files or partial WAL segments.
>
> durable_rename() calls fsync_fname(), so it would be covered by this change.
> The other file access calls in there can be handled by normal error
> handling, I think. Is there any specific scenario you have in mind?

The old file flush is handled by your patch, but not the new one if
it exists, and it seems to me that we should handle failures
consistently to reason easier about it, actually as the top of the
function says :)

Another point that we could consider is if fsync_fname() should have
an option to not trigger an immediate exit when facing a failure. The
backend has that option thanks to fsync_fname_ext() with its elevel
argument. Your choice to default to a failure is fine for most cases
because that's what we want. However, I am questioning if this change
would be surprising for some client applications or not, and if we
should have the option to choose one behavior or the other.
--
Michael