Add pg_file_sync() to adminpack

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?

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

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

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

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

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.

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.

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

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.

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

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

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

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.

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

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

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

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?

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

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.