On 2020-02-13 12:52, Michael Paquier wrote:
>> 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 :)
OK, added in new patch.
> 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.
The option in the backend is between panicking and retrying. The old
behavior was to always retry but we have learned that that usually
doesn't work.
The frontends do neither right now, or at least the error handling is
very inconsistent and inscrutable. It would be possible in theory to
add a retry option, but that would be a very different patch, and given
what we have learned about fsync(), it probably wouldn't be widely useful.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services