Re: Add pg_file_sync() to adminpack - Mailing list pgsql-hackers

From Stephen Frost
Subject Re: Add pg_file_sync() to adminpack
Date
Msg-id 20200109171606.GV3195@tamriel.snowman.net
Whole thread Raw
In response to Re: Add pg_file_sync() to adminpack  (Julien Rouhaud <rjuju123@gmail.com>)
Responses Re: Add pg_file_sync() to adminpack  (Julien Rouhaud <rjuju123@gmail.com>)
List pgsql-hackers
Greetings,

* Julien Rouhaud (rjuju123@gmail.com) wrote:
> On Thu, Jan 9, 2020 at 7:43 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> > On Wed, Dec 25, 2019 at 11:11 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > > On Wed, Dec 25, 2019 at 2:01 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> > > > 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.

Why would you expect that when it isn't the case for the filesystem
itself..?  I agree with Fujii on this- you should have to explicitly ask
for us to do more than the equivilant filesystem-level operation.  We
shouldn't be forcing that on you.

Thanks,

Stephen

Attachment

pgsql-hackers by date:

Previous
From: Andres Freund
Date:
Subject: Re: [Logical Replication] TRAP:FailedAssertion("rel->rd_rel->relreplident == REPLICA_IDENTITY_DEFAULT ||rel->rd_rel->relreplident == REPLICA_IDENTITY_FULL ||rel->rd_rel->relreplident == REPLICA_IDENTITY_INDEX"
Next
From: Robert Haas
Date:
Subject: Re: Make autovacuum sort tables in descending order of xid_age