Re: Add pg_file_sync() to adminpack

On Thu, Jan 9, 2020 at 7:43 AM Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> On Wed, Dec 25, 2019 at 11:11 PM Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
> >
> > On Wed, Dec 25, 2019 at 2:01 PM Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> > >
> > > Hi,
> > >
> > > I'd like to propose to add pg_file_sync() function into contrib/adminpack.
> > > This function fsyncs the specified file or directory named by its argument.
> > > IMO this is useful, for example, when you want to fsync the file that
> > > pg_file_write() writes out or that COPY TO exports the data into,
> > > for durability. Thought?
> >
> > +1, that seems like a useful wrapper. Looking at existing functions,
> > I see that there's a pg_file_rename() in adminpack, but it doesn't use
> > durable_rename nor does it try to perform any fsync. Same for
> > pg_file_unlink vs. durable_unlink. It's probably worth fixing that at
> > the same time?
>
> I don't think that's a bug. I'm not sure if every users of those functions
> need durable rename and unlink at the expense of performance.
> So IMO it's better to add new argument like "durable" to those functions
> and durable_rename or _unlink is used only if it's true.

It's probably a POLA violation. I'm pretty sure that most people
using those functions would expect that a successful call to
pg_file_unlink() mean that the file cannot raise from the dead even
with certain unlikely circumstances, at least I'd expect so. If
performance is a problem here, I'd rather have a new wrapper with a
sync flag that defaults to true so it's possible to disable it if
needed instead of calling a different function. That being said, I
agree with Arthur, it should be handled in a different patch.