pg_rewind fails if there is a read only file.
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)
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
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.
Attachments:
v1-0001-Fix-pg_rewind-failure-due-to-read-only-file-open-.patchapplication/octet-stream; name=v1-0001-Fix-pg_rewind-failure-due-to-read-only-file-open-.patchDownload
From 816027e6856e6243697127a3c35693abf66301bd Mon Sep 17 00:00:00 2001
From: Paul Guo <paulguo@gmail.com>
Date: Thu, 20 May 2021 04:28:09 +0800
Subject: [PATCH v1] Fix pg_rewind failure due to read only file open() error.
The error message is "Permission denied" since open_target_file()->open() fails
with O_WRONLY for read only file. This issue could be simply fixed by adding
the write permission before open() and removing the permission after open().
---
src/bin/pg_rewind/file_ops.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c
index c50f283ede..ef5d1fccca 100644
--- a/src/bin/pg_rewind/file_ops.c
+++ b/src/bin/pg_rewind/file_ops.c
@@ -64,8 +64,25 @@ open_target_file(const char *path, bool trunc)
mode |= O_TRUNC;
dstfd = open(dstpath, mode, pg_file_create_mode);
if (dstfd < 0)
- pg_fatal("could not open target file \"%s\": %m",
- dstpath);
+ {
+ if (errno == EACCES)
+ {
+ struct stat buf;
+
+ /* For read-only file, open() needs the write permission on the file. */
+ if (stat(dstpath, &buf) == 0 &&
+ !(buf.st_mode & S_IWUSR) &&
+ chmod(dstpath, buf.st_mode | S_IWUSR) == 0)
+ {
+ dstfd = open(dstpath, mode, pg_file_create_mode);
+ if (chmod(dstpath, buf.st_mode) != 0)
+ pg_fatal("could not chmod target file \"%s\": %m", dstpath);
+ }
+ }
+
+ if (dstfd < 0)
+ pg_fatal("could not open target file \"%s\": %m", dstpath);
+ }
}
/*
--
2.14.3
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
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.
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
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.htmlPerhaps 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
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
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
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
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
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
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)