Thread: pg_rewind fails if there is a read only file.

pg_rewind fails if there is a read only file.

From
Paul Guo
Date:
Several weeks ago I saw this issue in a production environment. The
read only file looks like a credential file. Michael told me that
usually such kinds of files should be better kept in non-pgdata
directories in production environments. Thought further it seems that
pg_rewind should be more user friendly to tolerate such scenarios.

The failure error is "Permission denied" after open(). The reason is
open() fais with the below mode in open_target_file()

      mode = O_WRONLY | O_CREAT | PG_BINARY;
      if (trunc)
          mode |= O_TRUNC;
      dstfd = open(dstpath, mode, pg_file_create_mode);

The fix should be quite simple, if open fails with "Permission denied"
then we try to call chmod to add a S_IWUSR just before open() and call
chmod to reset the flags soon after open(). A stat() call to get
previous st_mode flags is needed.

Any other suggestions or thoughts?

Thanks,

-- 
Paul Guo (Vmware)



Re: pg_rewind fails if there is a read only file.

From
Andrew Dunstan
Date:
On 5/19/21 6:43 AM, Paul Guo wrote:
> Several weeks ago I saw this issue in a production environment. The
> read only file looks like a credential file. Michael told me that
> usually such kinds of files should be better kept in non-pgdata
> directories in production environments. Thought further it seems that
> pg_rewind should be more user friendly to tolerate such scenarios.
>
> The failure error is "Permission denied" after open(). The reason is
> open() fais with the below mode in open_target_file()
>
>       mode = O_WRONLY | O_CREAT | PG_BINARY;
>       if (trunc)
>           mode |= O_TRUNC;
>       dstfd = open(dstpath, mode, pg_file_create_mode);
>
> The fix should be quite simple, if open fails with "Permission denied"
> then we try to call chmod to add a S_IWUSR just before open() and call
> chmod to reset the flags soon after open(). A stat() call to get
> previous st_mode flags is needed.
>

Presumably the user has a reason for adding the file read-only to the
data directory, and we shouldn't lightly ignore that.

Michael's advice is reasonable. This seems like a case of:

    Patient: Doctor, it hurts when I do this.

    Doctor: Stop doing that.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com




Re: pg_rewind fails if there is a read only file.

From
Paul Guo
Date:
> Presumably the user has a reason for adding the file read-only to the
> data directory, and we shouldn't lightly ignore that.
>
> Michael's advice is reasonable. This seems like a case of:

I agree. Attached is a short patch to handle the case. The patch was
tested in my dev environment.

Attachment

Re: pg_rewind fails if there is a read only file.

From
Andrew Dunstan
Date:
On 5/20/21 6:17 AM, Paul Guo wrote:
>> Presumably the user has a reason for adding the file read-only to the
>> data directory, and we shouldn't lightly ignore that.
>>
>> Michael's advice is reasonable. This seems like a case of:
> I agree. Attached is a short patch to handle the case. The patch was
> tested in my dev environment.



You seem to have missed my point completely. The answer to this problem
IMNSHO is "Don't put a read-only file in the data directory".


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com




Re: pg_rewind fails if there is a read only file.

From
Paul Guo
Date:
> You seem to have missed my point completely. The answer to this problem
> IMNSHO is "Don't put a read-only file in the data directory".

Oh sorry. Well, if we really do not want this we may want to document this
and keep educating users, but meanwhile probably the product should be
more user friendly for the case, especially considering
that we know the fix would be trivial and I suspect it is inevitable that some
extensions put some read only files (e.g. credentials files) in pgdata.



Re: pg_rewind fails if there is a read only file.

From
Laurenz Albe
Date:
On Tue, 2021-05-25 at 16:57 +0800, Paul Guo wrote:
> > You seem to have missed my point completely. The answer to this problem
> > IMNSHO is "Don't put a read-only file in the data directory".
> 
> Oh sorry. Well, if we really do not want this we may want to document this
> and keep educating users, but meanwhile probably the product should be
> more user friendly for the case, especially considering
> that we know the fix would be trivial and I suspect it is inevitable that some
> extensions put some read only files (e.g. credentials files) in pgdata.

Good idea.  I suggest this documentation page:
https://www.postgresql.org/docs/current/creating-cluster.html

Perhaps something along the line of:

  It is not supported to manually create, delete or modify files in the
  data directory, unless they are configuration files or the documentation
  explicitly says otherwise (for example, <file>recovery.signal</file>
  for archive recovery).

Yours,
Laurenz Albe




Re: pg_rewind fails if there is a read only file.

From
Andrew Dunstan
Date:
On 5/25/21 9:38 AM, Laurenz Albe wrote:
> On Tue, 2021-05-25 at 16:57 +0800, Paul Guo wrote:
>>> You seem to have missed my point completely. The answer to this problem
>>> IMNSHO is "Don't put a read-only file in the data directory".
>> Oh sorry. Well, if we really do not want this we may want to document this
>> and keep educating users, but meanwhile probably the product should be
>> more user friendly for the case, especially considering
>> that we know the fix would be trivial and I suspect it is inevitable that some
>> extensions put some read only files (e.g. credentials files) in pgdata.
> Good idea.  I suggest this documentation page:
> https://www.postgresql.org/docs/current/creating-cluster.html
>
> Perhaps something along the line of:
>
>   It is not supported to manually create, delete or modify files in the
>   data directory, unless they are configuration files or the documentation
>   explicitly says otherwise (for example, <file>recovery.signal</file>
>   for archive recovery).
>

Perhaps we should add that read-only files can be particularly problematic.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com




Re: pg_rewind fails if there is a read only file.

From
Tom Lane
Date:
Andrew Dunstan <andrew@dunslane.net> writes:
> Perhaps we should add that read-only files can be particularly problematic.

Given the (legitimate, IMO) example of a read-only SSL key, I'm not
quite convinced that pg_rewind doesn't need to cope with this.

            regards, tom lane



Re: pg_rewind fails if there is a read only file.

From
Andrew Dunstan
Date:
On 5/25/21 10:29 AM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> Perhaps we should add that read-only files can be particularly problematic.
> Given the (legitimate, IMO) example of a read-only SSL key, I'm not
> quite convinced that pg_rewind doesn't need to cope with this.
>
>             


If we do decide to do something the question arises what should it do?
If we're to allow for it I'm wondering if the best thing would be simply
to ignore such a file.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com




Re: pg_rewind fails if there is a read only file.

From
Michael Paquier
Date:
On Tue, May 25, 2021 at 03:17:37PM -0400, Andrew Dunstan wrote:
> If we do decide to do something the question arises what should it do?
> If we're to allow for it I'm wondering if the best thing would be simply
> to ignore such a file.

Enforcing assumptions that any file could be ready-only is a very bad
idea, as that could lead to weird behaviors if a FS is turned as
becoming read-only suddenly while doing a rewind.  Another idea that
has popped out across the years was to add an option to pg_rewind so
as users could filter files manually.  That could be easily dangerous
though in the wrong hands, as one could think that it is a good idea
to skip a control file, for example.

The thing is that here we actually know the set of files we'd like to
ignore most of the time, and we still want to have some automated
control what gets filtered.  So here is a new idea: we build a list of
files based on a set of GUC parameters using postgres -C on the target
data folder, and assume that these are safe enough to be skipped all
the time, if these are in the data folder.
--
Michael

Attachment

Re: pg_rewind fails if there is a read only file.

From
Laurenz Albe
Date:
On Wed, 2021-05-26 at 08:57 +0900, Michael Paquier wrote:
> On Tue, May 25, 2021 at 03:17:37PM -0400, Andrew Dunstan wrote:
> > If we do decide to do something the question arises what should it do?
> > If we're to allow for it I'm wondering if the best thing would be simply
> > to ignore such a file.
> 
> Enforcing assumptions that any file could be ready-only is a very bad
> idea, as that could lead to weird behaviors if a FS is turned as
> becoming read-only suddenly while doing a rewind.  Another idea that
> has popped out across the years was to add an option to pg_rewind so
> as users could filter files manually.  That could be easily dangerous
> though in the wrong hands, as one could think that it is a good idea
> to skip a control file, for example.
> 
> The thing is that here we actually know the set of files we'd like to
> ignore most of the time, and we still want to have some automated
> control what gets filtered.  So here is a new idea: we build a list of
> files based on a set of GUC parameters using postgres -C on the target
> data folder, and assume that these are safe enough to be skipped all
> the time, if these are in the data folder.

That sounds complicated, but should work.
There should be a code comment somewhere that warns people not to forget
to look at that when they add a new GUC.

I can think of two alternatives to handle this:

- Skip files that cannot be opened for writing and issue a warning.
  That is simple, but coarse.
  A slightly more sophisticated version would first check if files
  are the same on both machines and skip the warning for those.

- Paul's idea to try and change the mode on the read-only file
  and reset it to the original state after pg_rewind is done.

Yours,
Laurenz Albe




Re: pg_rewind fails if there is a read only file.

From
Andrew Dunstan
Date:
On 5/26/21 2:43 AM, Laurenz Albe wrote:
> On Wed, 2021-05-26 at 08:57 +0900, Michael Paquier wrote:
>> On Tue, May 25, 2021 at 03:17:37PM -0400, Andrew Dunstan wrote:
>>> If we do decide to do something the question arises what should it do?
>>> If we're to allow for it I'm wondering if the best thing would be simply
>>> to ignore such a file.
>> Enforcing assumptions that any file could be ready-only is a very bad
>> idea, as that could lead to weird behaviors if a FS is turned as
>> becoming read-only suddenly while doing a rewind.  Another idea that
>> has popped out across the years was to add an option to pg_rewind so
>> as users could filter files manually.  That could be easily dangerous
>> though in the wrong hands, as one could think that it is a good idea
>> to skip a control file, for example.
>>
>> The thing is that here we actually know the set of files we'd like to
>> ignore most of the time, and we still want to have some automated
>> control what gets filtered.  So here is a new idea: we build a list of
>> files based on a set of GUC parameters using postgres -C on the target
>> data folder, and assume that these are safe enough to be skipped all
>> the time, if these are in the data folder.
> That sounds complicated, but should work.
> There should be a code comment somewhere that warns people not to forget
> to look at that when they add a new GUC.
>
> I can think of two alternatives to handle this:
>
> - Skip files that cannot be opened for writing and issue a warning.
>   That is simple, but coarse.
>   A slightly more sophisticated version would first check if files
>   are the same on both machines and skip the warning for those.
>
> - Paul's idea to try and change the mode on the read-only file
>   and reset it to the original state after pg_rewind is done.
>

How about we only skip (with a warning) read-only files if they are in
the data root, not a subdirectory? That would save us from silently
ignoring read-only filesystems and not involve using a GUC.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com




Re: pg_rewind fails if there is a read only file.

From
Paul Guo
Date:
On Wed, May 26, 2021 at 10:32 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 5/26/21 2:43 AM, Laurenz Albe wrote:
> > On Wed, 2021-05-26 at 08:57 +0900, Michael Paquier wrote:
> >> On Tue, May 25, 2021 at 03:17:37PM -0400, Andrew Dunstan wrote:
> >>> If we do decide to do something the question arises what should it do?
> >>> If we're to allow for it I'm wondering if the best thing would be simply
> >>> to ignore such a file.
> >> Enforcing assumptions that any file could be ready-only is a very bad
> >> idea, as that could lead to weird behaviors if a FS is turned as
> >> becoming read-only suddenly while doing a rewind.  Another idea that
> >> has popped out across the years was to add an option to pg_rewind so
> >> as users could filter files manually.  That could be easily dangerous
> >> though in the wrong hands, as one could think that it is a good idea
> >> to skip a control file, for example.
> >>
> >> The thing is that here we actually know the set of files we'd like to
> >> ignore most of the time, and we still want to have some automated
> >> control what gets filtered.  So here is a new idea: we build a list of
> >> files based on a set of GUC parameters using postgres -C on the target
> >> data folder, and assume that these are safe enough to be skipped all
> >> the time, if these are in the data folder.
> > That sounds complicated, but should work.
> > There should be a code comment somewhere that warns people not to forget
> > to look at that when they add a new GUC.
> >
> > I can think of two alternatives to handle this:
> >
> > - Skip files that cannot be opened for writing and issue a warning.
> >   That is simple, but coarse.
> >   A slightly more sophisticated version would first check if files
> >   are the same on both machines and skip the warning for those.
> >
> > - Paul's idea to try and change the mode on the read-only file
> >   and reset it to the original state after pg_rewind is done.
> >
>
> How about we only skip (with a warning) read-only files if they are in
> the data root, not a subdirectory? That would save us from silently

The assumption seems to limit the user scenario.

> ignoring read-only filesystems and not involve using a GUC.

How about this:
By default, change and reset the file mode before and after open() for
read only files,
but we also allow to pass skip-file names to pg_rewind (e.g.
pg_rewind --skip filenameN or --skip-list-file listfile) in case users really
want to skip some files (probably not just read only files).
Presumably the people
who run pg_rewind should be DBA or admin that have basic knowledge of this
so we should not worry too much about that the user skips some important files
(we could even set a deny list for this). Also this solution works
fine for a read only
file system since pg_rewind soon quits when adding the write
permission for any read only file.

--
Paul Guo (Vmware)