Add pg_file_sync() to adminpack

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
29 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Add pg_file_sync() to adminpack

Fujii Masao-2
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

pg_file_sync_v1.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Julien Rouhaud
On Wed, Dec 25, 2019 at 2:01 PM Fujii Masao <[hidden email]> 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?


Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Artur Zakirov
Hello,

On 2019/12/25 23:12, Julien Rouhaud wrote:
> On Wed, Dec 25, 2019 at 2:01 PM Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Fujii Masao-2
On Mon, Jan 6, 2020 at 3:42 PM Michael Paquier <[hidden email]> 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

pg_file_sync_v2.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Fujii Masao-2
In reply to this post by Julien Rouhaud
On Wed, Dec 25, 2019 at 11:11 PM Julien Rouhaud <[hidden email]> wrote:

>
> On Wed, Dec 25, 2019 at 2:01 PM Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Julien Rouhaud
On Thu, Jan 9, 2020 at 7:43 AM Fujii Masao <[hidden email]> wrote:

>
> On Wed, Dec 25, 2019 at 11:11 PM Julien Rouhaud <[hidden email]> wrote:
> >
> > On Wed, Dec 25, 2019 at 2:01 PM Fujii Masao <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Julien Rouhaud
In reply to this post by Fujii Masao-2
On Thu, Jan 9, 2020 at 7:31 AM Fujii Masao <[hidden email]> wrote:

>
> On Mon, Jan 6, 2020 at 3:42 PM Michael Paquier <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Stephen Frost
In reply to this post by Julien Rouhaud
Greetings,

* Julien Rouhaud ([hidden email]) wrote:

> On Thu, Jan 9, 2020 at 7:43 AM Fujii Masao <[hidden email]> wrote:
> > On Wed, Dec 25, 2019 at 11:11 PM Julien Rouhaud <[hidden email]> wrote:
> > > On Wed, Dec 25, 2019 at 2:01 PM Fujii Masao <[hidden email]> 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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Julien Rouhaud
On Thu, Jan 9, 2020 at 6:16 PM Stephen Frost <[hidden email]> wrote:

>
> Greetings,
>
> * Julien Rouhaud ([hidden email]) wrote:
> > On Thu, Jan 9, 2020 at 7:43 AM Fujii Masao <[hidden email]> wrote:
> > > On Wed, Dec 25, 2019 at 11:11 PM Julien Rouhaud <[hidden email]> wrote:
> > > > On Wed, Dec 25, 2019 at 2:01 PM Fujii Masao <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Tom Lane-2
Julien Rouhaud <[hidden email]> writes:
> On Thu, Jan 9, 2020 at 6:16 PM Stephen Frost <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Fujii Masao-2
In reply to this post by Julien Rouhaud
On Thu, Jan 9, 2020 at 10:39 PM Julien Rouhaud <[hidden email]> wrote:

>
> On Thu, Jan 9, 2020 at 7:31 AM Fujii Masao <[hidden email]> wrote:
> >
> > On Mon, Jan 6, 2020 at 3:42 PM Michael Paquier <[hidden email]> 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

pg_file_sync_v3.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Julien Rouhaud
In reply to this post by Fujii Masao-2
On Fri, Jan 10, 2020 at 10:50 AM Fujii Masao <[hidden email]> wrote:
>
> On Thu, Jan 9, 2020 at 10:39 PM Julien Rouhaud <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Fujii Masao-2
In reply to this post by Michael Paquier-2
On Fri, Jan 10, 2020 at 8:16 PM Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Artur Zakirov
Hello,

On Sat, Jan 11, 2020 at 2:12 AM Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Michael Paquier-2
In reply to this post by Fujii Masao-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Julien Rouhaud
On Mon, Jan 13, 2020 at 2:46 PM Michael Paquier <[hidden email]> 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?


Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add pg_file_sync() to adminpack

Julien Rouhaud
On Tue, Jan 14, 2020 at 7:18 AM Michael Paquier <[hidden email]> 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.


12