Re: Add pg_file_sync() to adminpack

On Fri, Jan 24, 2020 at 8:20 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> On 2020/01/24 15:38, Arthur Zakirov wrote:
> > On 2020/01/24 14:56, Michael Paquier wrote:
> >> On Fri, Jan 24, 2020 at 01:28:29PM +0900, Arthur Zakirov wrote:
> >>> It is compiled and passes the tests. There is the documentation and
> >>> it is
> >>> built too without an error.
> >>>
> >>> It seems that consensus about the returned type was reached and I
> >>> marked the
> >>> patch as "Ready for Commiter".
> >>
> >> +       fsync_fname_ext(filename, S_ISDIR(fst.st_mode), false, ERROR);
> >> One comment here: should we warn better users in the docs that a fsync
> >> failule will not trigger a PANIC here?  Here, fsync failure on heap
> >> file => ERROR => potential data corruption.
> >
> > Ah, true. It is possible to add couple sentences that pg_file_sync()
> > doesn't depend on data_sync_retry GUC and doesn't raise a PANIC even for
> > database files.
>
> Thanks all for the review!
>
> So, what about the attached patch?
> In the patch, I added the following note to the doc.
>
> --------------------
> Note that
> <xref linkend="guc-data-sync-retry"/> has no effect on this function,
> and therefore a PANIC-level error will not be raised even on failure to
> flush database files.
> --------------------

We should explicitly mention that this can cause corruption.  How  about:

--------------------
Note that
<xref linkend="guc-data-sync-retry"/> has no effect on this function,
and therefore a PANIC-level error will not be raised even on failure to
flush database files.  If that happens, the underlying database
objects may be corrupted.
--------------------