Thread: 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.
Attachment
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.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
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
Attachment
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)