Horton

Thread

Commits

  1. Add pg_file_sync() to adminpack extension.

  1. Add pg_file_sync() to adminpack

    Fujii Masao <masao.fujii@gmail.com> — 2019-12-25T13:00:36Z

    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?

Regards,

-- 
Fujii Masao

  2. Re: Add pg_file_sync() to adminpack

    Julien Rouhaud <rjuju123@gmail.com> — 2019-12-25T14:12:42Z

    On Wed, Dec 25, 2019 at 2:01 PM Fujii Masao <masao.fujii@gmail.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?

  3. Re: Add pg_file_sync() to adminpack

    Artur Zakirov <zaartur@gmail.com> — 2020-01-06T06:20:13Z

    Hello,

On 2019/12/25 23:12, Julien Rouhaud wrote:
> On Wed, Dec 25, 2019 at 2:01 PM Fujii Masao <masao.fujii@gmail.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 too. I have a thought, but maybe it is just a nitpicking.

pg_file_sync() calls fsync_fname() function from fd.c. And I think it 
might bring problems because fsync_fname() uses data_sync_elevel() to 
get elevel. As a result if data_sync_retry GUC is false fsync_fname() 
might raise PANIC message.

It isn't case if a file doesn't exist. But if there are no permissions 
on the file:

PANIC:  could not open file "testfile": Permissions denied
server closed the connection unexpectedly

It could be fixed by implementing a function like 
pg_file_sync_internal() or by making the function fsync_fname_ext() 
external.

> +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 think it might be a different patch.

-- 
Arthur

  4. Re: Add pg_file_sync() to adminpack

    Michael Paquier <michael@paquier.xyz> — 2020-01-06T06:42:39Z

    On Mon, Jan 06, 2020 at 03:20:13PM +0900, Arthur Zakirov wrote:
> It isn't case if a file doesn't exist. But if there are no permissions on
> the file:
> 
> PANIC:  could not open file "testfile": Permissions denied
> server closed the connection unexpectedly
> 
> It could be fixed by implementing a function like pg_file_sync_internal() or
> by making the function fsync_fname_ext() external.

The patch uses stat() to make sure that the file exists and has no
issues.  Though it could be a problem with any kind of TOCTOU-like
issues (looking at you, Windows, for ENOPERM), so I agree that it
would make more sense to use pg_fsync() here with a fd opened first.
--
Michael

  5. Re: Add pg_file_sync() to adminpack

    Fujii Masao <masao.fujii@gmail.com> — 2020-01-09T06:31:47Z

    On Mon, Jan 6, 2020 at 3:42 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Jan 06, 2020 at 03:20:13PM +0900, Arthur Zakirov wrote:
> > It isn't case if a file doesn't exist. But if there are no permissions on
> > the file:
> >
> > PANIC:  could not open file "testfile": Permissions denied
> > server closed the connection unexpectedly
> >
> > It could be fixed by implementing a function like pg_file_sync_internal() or
> > by making the function fsync_fname_ext() external.
>
> The patch uses stat() to make sure that the file exists and has no
> issues.  Though it could be a problem with any kind of TOCTOU-like
> issues (looking at you, Windows, for ENOPERM), so I agree that it
> would make more sense to use pg_fsync() here with a fd opened first.

I agree that it's not good for pg_file_sync() to cause a PANIC.
I updated the patch so that pg_file_sync() uses fsync_fname_ext()
instead of fsync_fname() as Arthur suggested.

It's one of ideas to make pg_file_sync() open the file and directly call
pg_fsync(). But fsync_fname_ext() has already such code and
I'd like to avoid the code duplication.

Attached is the updated version of the patch.

Regards,

-- 
Fujii Masao

  6. Re: Add pg_file_sync() to adminpack

    Fujii Masao <masao.fujii@gmail.com> — 2020-01-09T06:43:03Z

    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:
> >
> > 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.

Regards,

-- 
Fujii Masao

  7. Re: Add pg_file_sync() to adminpack

    Julien Rouhaud <rjuju123@gmail.com> — 2020-01-09T13:34:10Z

    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:
> > >
> > > 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.

  8. Re: Add pg_file_sync() to adminpack

    Julien Rouhaud <rjuju123@gmail.com> — 2020-01-09T13:39:42Z

    On Thu, Jan 9, 2020 at 7:31 AM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Mon, Jan 6, 2020 at 3:42 PM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Mon, Jan 06, 2020 at 03:20:13PM +0900, Arthur Zakirov wrote:
> > > It isn't case if a file doesn't exist. But if there are no permissions on
> > > the file:
> > >
> > > PANIC:  could not open file "testfile": Permissions denied
> > > server closed the connection unexpectedly
> > >
> > > It could be fixed by implementing a function like pg_file_sync_internal() or
> > > by making the function fsync_fname_ext() external.
> >
> > The patch uses stat() to make sure that the file exists and has no
> > issues.  Though it could be a problem with any kind of TOCTOU-like
> > issues (looking at you, Windows, for ENOPERM), so I agree that it
> > would make more sense to use pg_fsync() here with a fd opened first.
>
> I agree that it's not good for pg_file_sync() to cause a PANIC.
> I updated the patch so that pg_file_sync() uses fsync_fname_ext()
> instead of fsync_fname() as Arthur suggested.
>
> It's one of ideas to make pg_file_sync() open the file and directly call
> pg_fsync(). But fsync_fname_ext() has already such code and
> I'd like to avoid the code duplication.

This looks good to me.

> Attached is the updated version of the patch.

+    <row>
+     <entry><function>pg_catalog.pg_file_sync(filename text)</function></entry>
+     <entry><type>boolean</type></entry>
+     <entry>
+      Sync a file or directory
+     </entry>
+    </row>

"Flush to disk" looks better than "sync" here.

+/* ------------------------------------
+ * pg_file_sync
+ *
+ * We REVOKE EXECUTE on the function from PUBLIC.
+ * Users can then grant access to it based on their policies.
+ */

I think that pg_write_server_files should be allowed to call that
function by default.

  9. Re: Add pg_file_sync() to adminpack

    Stephen Frost <sfrost@snowman.net> — 2020-01-09T17:16:07Z

    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

  10. Re: Add pg_file_sync() to adminpack

    Julien Rouhaud <rjuju123@gmail.com> — 2020-01-09T17:31:27Z

    On Thu, Jan 9, 2020 at 6:16 PM Stephen Frost <sfrost@snowman.net> wrote:
>
> 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..?

Just a usual habit with durable property.

>  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.

I just checked other somehow related cases and saw that for instance
COPY TO doesn't flush data either, so it's indeed the expected
behavior.

  11. Re: Add pg_file_sync() to adminpack

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-01-09T17:51:06Z

    Julien Rouhaud <rjuju123@gmail.com> writes:
> On Thu, Jan 9, 2020 at 6:16 PM Stephen Frost <sfrost@snowman.net> wrote:
>> Why would you expect that when it isn't the case for the filesystem
>> itself..?

> Just a usual habit with durable property.

I tend to agree with Stephen on this, mainly because the point of
these adminpack functions is to expose filesystem access.  If these
functions were more "database-y" and less "filesystem-y", I'd agree
with trying to impose database-like consistency requirements.

We don't have to expose every wart of the filesystem semantics
--- for example, it would be reasonable to make pg_file_sync()
Do The Right Thing when applied to a directory, even if the
particular platform we're on doesn't behave sanely for that.
But having fsync separated from write is a pretty fundamental part
of most filesystems' semantics, so we ought not try to hide that.

			regards, tom lane

  12. Re: Add pg_file_sync() to adminpack

    Fujii Masao <masao.fujii@gmail.com> — 2020-01-10T09:50:12Z

    On Thu, Jan 9, 2020 at 10:39 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Thu, Jan 9, 2020 at 7:31 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > On Mon, Jan 6, 2020 at 3:42 PM Michael Paquier <michael@paquier.xyz> wrote:
> > >
> > > On Mon, Jan 06, 2020 at 03:20:13PM +0900, Arthur Zakirov wrote:
> > > > It isn't case if a file doesn't exist. But if there are no permissions on
> > > > the file:
> > > >
> > > > PANIC:  could not open file "testfile": Permissions denied
> > > > server closed the connection unexpectedly
> > > >
> > > > It could be fixed by implementing a function like pg_file_sync_internal() or
> > > > by making the function fsync_fname_ext() external.
> > >
> > > The patch uses stat() to make sure that the file exists and has no
> > > issues.  Though it could be a problem with any kind of TOCTOU-like
> > > issues (looking at you, Windows, for ENOPERM), so I agree that it
> > > would make more sense to use pg_fsync() here with a fd opened first.
> >
> > I agree that it's not good for pg_file_sync() to cause a PANIC.
> > I updated the patch so that pg_file_sync() uses fsync_fname_ext()
> > instead of fsync_fname() as Arthur suggested.
> >
> > It's one of ideas to make pg_file_sync() open the file and directly call
> > pg_fsync(). But fsync_fname_ext() has already such code and
> > I'd like to avoid the code duplication.
>
> This looks good to me.
>
> > Attached is the updated version of the patch.
>
> +    <row>
> +     <entry><function>pg_catalog.pg_file_sync(filename text)</function></entry>
> +     <entry><type>boolean</type></entry>
> +     <entry>
> +      Sync a file or directory
> +     </entry>
> +    </row>
>
> "Flush to disk" looks better than "sync" here.

I changed the doc that way. Thanks for the review!

> I think that pg_write_server_files should be allowed to call that
> function by default.

But pg_write_server_files users are not allowed to execute
other functions like pg_file_write() by default. So doing that
change only for pg_file_sync() looks strange to me.

Regards,

-- 
Fujii Masao

  13. Re: Add pg_file_sync() to adminpack

    Michael Paquier <michael@paquier.xyz> — 2020-01-10T11:16:20Z

    On Fri, Jan 10, 2020 at 06:50:12PM +0900, Fujii Masao wrote:
> I changed the doc that way. Thanks for the review!

+ <para>
+  <function>pg_file_sync</function> fsyncs the specified file or directory
+  named by <parameter>filename</parameter>.  Returns true on success,
+  an error is thrown otherwise (e.g., the specified file is not present).
+ </para>
What's the point of having a function that returns a boolean if it
just returns true all the time?  Wouldn't it be better to have a set
of semantics closer to the unlink() part, where the call of stat()
fails with an ERROR for (errno != ENOENT) and the fsync call returns
false with a WARNING?

+SELECT pg_file_sync('global'); -- sync directory
+ pg_file_sync
+--------------
+ t
+(1 row)
installcheck deployments may not like that.
--
Michael

  14. Re: Add pg_file_sync() to adminpack

    Julien Rouhaud <rjuju123@gmail.com> — 2020-01-10T11:22:53Z

    On Fri, Jan 10, 2020 at 10:50 AM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Thu, Jan 9, 2020 at 10:39 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > I think that pg_write_server_files should be allowed to call that
> > function by default.
>
> But pg_write_server_files users are not allowed to execute
> other functions like pg_file_write() by default. So doing that
> change only for pg_file_sync() looks strange to me.

Ah indeed.  I'm wondering if that's an oversight of the original
default role patch or voluntary.

  15. Re: Add pg_file_sync() to adminpack

    Fujii Masao <masao.fujii@gmail.com> — 2020-01-10T17:12:15Z

    On Fri, Jan 10, 2020 at 8:16 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Jan 10, 2020 at 06:50:12PM +0900, Fujii Masao wrote:
> > I changed the doc that way. Thanks for the review!

Thanks for the review!

> + <para>
> +  <function>pg_file_sync</function> fsyncs the specified file or directory
> +  named by <parameter>filename</parameter>.  Returns true on success,
> +  an error is thrown otherwise (e.g., the specified file is not present).
> + </para>
> What's the point of having a function that returns a boolean if it
> just returns true all the time?  Wouldn't it be better to have a set
> of semantics closer to the unlink() part, where the call of stat()
> fails with an ERROR for (errno != ENOENT) and the fsync call returns
> false with a WARNING?

I'm not sure if returning false with WARNING only in some error cases
is really good idea or not. At least for me, it's more intuitive to
return true on success and emit an ERROR otherwise. I'd like to hear
more opinions about this. Also if returning true on success is rather
confusing, we can change its return type to void.

> +SELECT pg_file_sync('global'); -- sync directory
> + pg_file_sync
> +--------------
> + t
> +(1 row)
> installcheck deployments may not like that.

Could you elaborate why? But if it's not good to sync the existing directory
in the regression test, we may need to give up testing the sync of directory.
Another idea is to add another function like pg_mkdir() into adminpack
and use the directory that we newly created by using that function,
for the test. Or better idea?

Regards,

-- 
Fujii Masao

  16. Re: Add pg_file_sync() to adminpack

    Artur Zakirov <zaartur@gmail.com> — 2020-01-11T10:04:50Z

    Hello,

On Sat, Jan 11, 2020 at 2:12 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> > + <para>
> > +  <function>pg_file_sync</function> fsyncs the specified file or directory
> > +  named by <parameter>filename</parameter>.  Returns true on success,
> > +  an error is thrown otherwise (e.g., the specified file is not present).
> > + </para>
> > What's the point of having a function that returns a boolean if it
> > just returns true all the time?  Wouldn't it be better to have a set
> > of semantics closer to the unlink() part, where the call of stat()
> > fails with an ERROR for (errno != ENOENT) and the fsync call returns
> > false with a WARNING?
>
> I'm not sure if returning false with WARNING only in some error cases
> is really good idea or not. At least for me, it's more intuitive to
> return true on success and emit an ERROR otherwise. I'd like to hear
> more opinions about this. Also if returning true on success is rather
> confusing, we can change its return type to void.

I think it would be more consistent to pg_file_unlink().

Other functions throw an ERROR and return a number or set of records
except pg_file_rename(), which in some cases throws a WARNING and
returns a boolean result.

-- 
Arthur

  17. Re: Add pg_file_sync() to adminpack

    Michael Paquier <michael@paquier.xyz> — 2020-01-13T13:46:00Z

    On Sat, Jan 11, 2020 at 02:12:15AM +0900, Fujii Masao wrote:
> I'm not sure if returning false with WARNING only in some error cases
> is really good idea or not. At least for me, it's more intuitive to
> return true on success and emit an ERROR otherwise. I'd like to hear
> more opinions about this. Also if returning true on success is rather
> confusing, we can change its return type to void.

An advantage of not issuing an ERROR if that when working on a list of
files (for example a WITH RECURSIVE on the whole data directory?), you
can then know which files could not be synced instead of seeing one
ERROR about one file, while being unsure about the state of the
others.

> Could you elaborate why? But if it's not good to sync the existing directory
> in the regression test, we may need to give up testing the sync of directory.
> Another idea is to add another function like pg_mkdir() into adminpack
> and use the directory that we newly created by using that function,
> for the test. Or better idea?

We should avoid potentially costly tests in any regression scenario if
we have a way to do so.  I like your idea of having a pg_mkdir(), that
feels more natural to have as there is already pg_file_write().
--
Michael

  18. Re: Add pg_file_sync() to adminpack

    Julien Rouhaud <rjuju123@gmail.com> — 2020-01-13T14:39:32Z

    On Mon, Jan 13, 2020 at 2:46 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sat, Jan 11, 2020 at 02:12:15AM +0900, Fujii Masao wrote:
> > I'm not sure if returning false with WARNING only in some error cases
> > is really good idea or not. At least for me, it's more intuitive to
> > return true on success and emit an ERROR otherwise. I'd like to hear
> > more opinions about this. Also if returning true on success is rather
> > confusing, we can change its return type to void.
>
> An advantage of not issuing an ERROR if that when working on a list of
> files (for example a WITH RECURSIVE on the whole data directory?), you
> can then know which files could not be synced instead of seeing one
> ERROR about one file, while being unsure about the state of the
> others.

Actually, can't it create a security hazard, for instance if you call
pg_file_sync() on a heap file and the calls errors out, since it's
bypassing data_sync_retry?

  19. Re: Add pg_file_sync() to adminpack

    Michael Paquier <michael@paquier.xyz> — 2020-01-14T06:17:56Z

    On Mon, Jan 13, 2020 at 03:39:32PM +0100, Julien Rouhaud wrote:
> Actually, can't it create a security hazard, for instance if you call
> pg_file_sync() on a heap file and the calls errors out, since it's
> bypassing data_sync_retry?

Are you mistaking security with durability here?  By default, the
function proposed is only executable by a superuser, so that's not
really a security concern..  But I agree that failing to detect a
PANIC on a fsync for a sensitive Postgres file could lead to
corruptions.  That's why we PANIC these days. 
--
Michael

  20. Re: Add pg_file_sync() to adminpack

    Julien Rouhaud <rjuju123@gmail.com> — 2020-01-14T06:26:07Z

    On Tue, Jan 14, 2020 at 7:18 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Jan 13, 2020 at 03:39:32PM +0100, Julien Rouhaud wrote:
> > Actually, can't it create a security hazard, for instance if you call
> > pg_file_sync() on a heap file and the calls errors out, since it's
> > bypassing data_sync_retry?
>
> Are you mistaking security with durability here?

Yes, data durability sorry.

>  By default, the
> function proposed is only executable by a superuser, so that's not
> really a security concern..  But I agree that failing to detect a
> PANIC on a fsync for a sensitive Postgres file could lead to
> corruptions.  That's why we PANIC these days.

Exactly.  My concern is that some superuser may not be aware that
pg_file_sync could actually corrupt data, so there should be a big red
warning explaining that.

  21. Re: Add pg_file_sync() to adminpack

    Atsushi Torikoshi <atorik@gmail.com> — 2020-01-14T15:08:06Z

    Hello,

> On Sut, Jan 11, 2020 at  2:12 Fujii Masao <masao.fujii@gmail.com>:
> I'm not sure if returning false with WARNING only in some error cases
> is really good idea or not. At least for me, it's more intuitive to
> return true on success and emit an ERROR otherwise. I'd like to hear
> more opinions about this.

+1.
As a user, I expect these adminpack functions to do similar behaviors
to the corresponding system calls.
System calls for flushing data to disk(fsync on Linux and FlushFileBuffers
 on Windows) return different codes on success and failure, and when it
fails we can get error messages. So it seems straightforward for me to
 'return true on success and emit an ERROR otherwise'.


> > On Thu, Jan 9, 2020 at 10:39 PM Julien Rouhaud <rjuju123@gmail.com>
wrote:
> > >
> > > I think that pg_write_server_files should be allowed to call that
> > > function by default.
> >
> > But pg_write_server_files users are not allowed to execute
> > other functions like pg_file_write() by default. So doing that
> > change only for pg_file_sync() looks strange to me.

> Ah indeed.  I'm wondering if that's an oversight of the original
> default role patch or voluntary.

It's not directly related to the patch, but as far as I read the
manual below, I expected pg_write_server_files users could execute
 adminpack functions.

  | Table 21.1 Default Roles
  | pg_write_server_files: Allow writing to files in any location the
database can access on the server with COPY and other file-access functions.


--
Atsushi Torikoshi

  22. Re: Add pg_file_sync() to adminpack

    Julien Rouhaud <rjuju123@gmail.com> — 2020-01-14T16:50:57Z

    On Tue, Jan 14, 2020 at 4:08 PM Atsushi Torikoshi <atorik@gmail.com> wrote:
>
> > On Sut, Jan 11, 2020 at  2:12 Fujii Masao <masao.fujii@gmail.com>:
> > > But pg_write_server_files users are not allowed to execute
> > > other functions like pg_file_write() by default. So doing that
> > > change only for pg_file_sync() looks strange to me.
>
> > Ah indeed.  I'm wondering if that's an oversight of the original
> > default role patch or voluntary.
>
> It's not directly related to the patch, but as far as I read the
> manual below, I expected pg_write_server_files users could execute
>  adminpack functions.
>
>   | Table 21.1 Default Roles
>   | pg_write_server_files: Allow writing to files in any location the database can access on the server with COPY and other file-access functions.

Actually, pg_write_server_files has enough privileges to execute those
functions anywhere on the FS as far as C code is concerned, provided
that the user running postgres daemon is allowed to (see
convert_and_check_filename), but won't be allowed to do so by default
as it won't have EXECUTE privilege on the functions.

  23. Re: Add pg_file_sync() to adminpack

    Stephen Frost <sfrost@snowman.net> — 2020-01-14T19:57:44Z

    Greetings,

* Julien Rouhaud (rjuju123@gmail.com) wrote:
> On Fri, Jan 10, 2020 at 10:50 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> > On Thu, Jan 9, 2020 at 10:39 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > > I think that pg_write_server_files should be allowed to call that
> > > function by default.
> >
> > But pg_write_server_files users are not allowed to execute
> > other functions like pg_file_write() by default. So doing that
> > change only for pg_file_sync() looks strange to me.
> 
> Ah indeed.  I'm wondering if that's an oversight of the original
> default role patch or voluntary.

It was intentional.

Thanks,

Stephen

  24. Re: Add pg_file_sync() to adminpack

    Julien Rouhaud <rjuju123@gmail.com> — 2020-01-14T22:12:18Z

    Le mar. 14 janv. 2020 à 22:57, Stephen Frost <sfrost@snowman.net> a écrit :

> Greetings,
>
> * Julien Rouhaud (rjuju123@gmail.com) wrote:
> > On Fri, Jan 10, 2020 at 10:50 AM Fujii Masao <masao.fujii@gmail.com>
> wrote:
> > > On Thu, Jan 9, 2020 at 10:39 PM Julien Rouhaud <rjuju123@gmail.com>
> wrote:
> > > > I think that pg_write_server_files should be allowed to call that
> > > > function by default.
> > >
> > > But pg_write_server_files users are not allowed to execute
> > > other functions like pg_file_write() by default. So doing that
> > > change only for pg_file_sync() looks strange to me.
> >
> > Ah indeed.  I'm wondering if that's an oversight of the original
> > default role patch or voluntary.
>
> It was intentional.
>

ok, thanks for the clarification.

>

  25. Re: Add pg_file_sync() to adminpack

    Atsushi Torikoshi <atorik@gmail.com> — 2020-01-15T12:59:59Z

    On Wed, Jan 15, 2020 at 1:49 Julien Rouhaud <rjuju123@gmail.com>:

> Actually, pg_write_server_files has enough privileges to execute those
> functions anywhere on the FS as far as C code is concerned, provided
> that the user running postgres daemon is allowed to (see
> convert_and_check_filename), but won't be allowed to do so by default
> as it won't have EXECUTE privilege on the functions.
>

I see, thanks for your explanation.

--
Regards,
Atsushi Torikoshi

  26. Re: Add pg_file_sync() to adminpack

    Robert Haas <robertmhaas@gmail.com> — 2020-01-16T14:51:24Z

    On Tue, Jan 14, 2020 at 10:08 AM Atsushi Torikoshi <atorik@gmail.com> wrote:
> fails we can get error messages. So it seems straightforward for me to
>  'return true on success and emit an ERROR otherwise'.

I agree with reporting errors using ERROR, but it seems to me that we
ought to then make the function return 'void' rather than 'bool'.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  27. Re: Add pg_file_sync() to adminpack

    Michael Paquier <michael@paquier.xyz> — 2020-01-17T04:36:52Z

    On Thu, Jan 16, 2020 at 09:51:24AM -0500, Robert Haas wrote:
> On Tue, Jan 14, 2020 at 10:08 AM Atsushi Torikoshi <atorik@gmail.com> wrote:
>> fails we can get error messages. So it seems straightforward for me to
>>  'return true on success and emit an ERROR otherwise'.
> 
> I agree with reporting errors using ERROR, but it seems to me that we
> ought to then make the function return 'void' rather than 'bool'.

Yeah, that should be either ERROR and return a void result, or issue a
WARNING/ERROR (depending on the code path, maybe PANIC?) with a
boolean status returned if there is a WARNING.  Mixing both concepts
with an ERROR all the time and always a true status is just weird,
because you know that if no errors are raised then the status will be
always true.  So there is no point to have a boolean status to begin
with.
--
Michael

  28. Re: Add pg_file_sync() to adminpack

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-01-17T07:05:03Z

    
On 2020/01/17 13:36, Michael Paquier wrote:
> On Thu, Jan 16, 2020 at 09:51:24AM -0500, Robert Haas wrote:
>> On Tue, Jan 14, 2020 at 10:08 AM Atsushi Torikoshi <atorik@gmail.com> wrote:
>>> fails we can get error messages. So it seems straightforward for me to
>>>   'return true on success and emit an ERROR otherwise'.
>>
>> I agree with reporting errors using ERROR, but it seems to me that we
>> ought to then make the function return 'void' rather than 'bool'.
> 
> Yeah, that should be either ERROR and return a void result, or issue a
> WARNING/ERROR (depending on the code path, maybe PANIC?) with a
> boolean status returned if there is a WARNING.  Mixing both concepts
> with an ERROR all the time and always a true status is just weird,
> because you know that if no errors are raised then the status will be
> always true.  So there is no point to have a boolean status to begin
> with.

OK, so our consensus is to return void on success and throw an error
otherwise. Attached is the updated version of the patch.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

  29. Re: Add pg_file_sync() to adminpack

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-01-17T07:18:21Z

    
On 2020/01/13 22:46, Michael Paquier wrote:
> On Sat, Jan 11, 2020 at 02:12:15AM +0900, Fujii Masao wrote:
>> I'm not sure if returning false with WARNING only in some error cases
>> is really good idea or not. At least for me, it's more intuitive to
>> return true on success and emit an ERROR otherwise. I'd like to hear
>> more opinions about this. Also if returning true on success is rather
>> confusing, we can change its return type to void.
> 
> An advantage of not issuing an ERROR if that when working on a list of
> files (for example a WITH RECURSIVE on the whole data directory?), you
> can then know which files could not be synced instead of seeing one
> ERROR about one file, while being unsure about the state of the
> others.
> 
>> Could you elaborate why? But if it's not good to sync the existing directory
>> in the regression test, we may need to give up testing the sync of directory.
>> Another idea is to add another function like pg_mkdir() into adminpack
>> and use the directory that we newly created by using that function,
>> for the test. Or better idea?
> 
> We should avoid potentially costly tests in any regression scenario if
> we have a way to do so.  I like your idea of having a pg_mkdir(), that
> feels more natural to have as there is already pg_file_write().

BTW, in the latest patch that I posted upthread, I changed
the directory to sync for the test from "global" to "pg_stat"
because pg_stat is empty while the server is running,
and syncing it would not be so costly.
Introduing pg_mkdir() (maybe pg_rmdir() would be also necessary)
is an idea, but it's better to do that as a separate patch
if it's really necessary.

Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

  30. Re: Add pg_file_sync() to adminpack

    Artur Zakirov <zaartur@gmail.com> — 2020-01-24T04:28:29Z

    On 2020/01/17 16:05, Fujii Masao wrote:
> On 2020/01/17 13:36, Michael Paquier wrote:
>> Yeah, that should be either ERROR and return a void result, or issue a
>> WARNING/ERROR (depending on the code path, maybe PANIC?) with a
>> boolean status returned if there is a WARNING.  Mixing both concepts
>> with an ERROR all the time and always a true status is just weird,
>> because you know that if no errors are raised then the status will be
>> always true.  So there is no point to have a boolean status to begin
>> with.
> 
> OK, so our consensus is to return void on success and throw an error
> otherwise. Attached is the updated version of the patch.
Thank you for the new version!

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".

-- 
Arthur

  31. Re: Add pg_file_sync() to adminpack

    Michael Paquier <michael@paquier.xyz> — 2020-01-24T05:56:45Z

    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.
--
Michael

  32. Re: Add pg_file_sync() to adminpack

    Julien Rouhaud <rjuju123@gmail.com> — 2020-01-24T06:31:06Z

    On Fri, Jan 24, 2020 at 6:56 AM Michael Paquier <michael@paquier.xyz> 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.

Definitely yes.

  33. Re: Add pg_file_sync() to adminpack

    Artur Zakirov <zaartur@gmail.com> — 2020-01-24T06:38:11Z

    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.

-- 
Arthur

  34. Re: Add pg_file_sync() to adminpack

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-01-24T07:19:58Z

    
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.
--------------------


Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

  35. Re: Add pg_file_sync() to adminpack

    Julien Rouhaud <rjuju123@gmail.com> — 2020-01-24T07:56:47Z

    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.
--------------------

  36. Re: Add pg_file_sync() to adminpack

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-01-24T08:08:35Z

    
On 2020/01/24 16:56, Julien Rouhaud wrote:
> 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.
> --------------------

IMO that's overkill. If we really need such mention for pg_file_sync(),
we also need to add it for other functions like pg_read_file(),
pg_stat_file(), etc. But, again, which looks overkill.

Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

  37. Re: Add pg_file_sync() to adminpack

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-01-24T11:50:16Z

    
On 2020/01/24 17:08, Fujii Masao wrote:
> 
> 
> On 2020/01/24 16:56, Julien Rouhaud wrote:
>> 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.
>> --------------------
> 
> IMO that's overkill. If we really need such mention for pg_file_sync(),
> we also need to add it for other functions like pg_read_file(),
> pg_stat_file(), etc. But, again, which looks overkill.

I pushed the v5 of the patch. Thanks all for reviewing the patch!
If the current document is not good yet, let's keep discussing that!

Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters