On Tue, Mar 27, 2018 at 04:21:09PM -0400, David Steele wrote:
> These updates address Michael's latest review and implement group access
> for pg_basebackup, pg_receivewal, and pg_recvlogical. A new internal
> GUC, data_directory_group_access, allows remote processes to determine
> the correct mode using the existing SHOW protocol command.
Neat idea. This is another use case where the SHOW command in the
replication protocol proves to be useful.
> I have dropped patch 01, which added the pg_resetwal tests. The tests
> Peter added recently are sufficient for this patch so I'll pursue adding
> the other tests separately to avoid noise on this thread.
That's fair.
Some nits from patch 1...
+ ok (check_mode_recursive($node_master->data_dir(), 0700, 0600));
[...]
+ ok (check_mode_recursive($node_master->data_dir(), 0700, 0600));
Incorrect indentations (space after "ok", yes that's a nit...).
+ /* Set dir/file mode mask */
+ umask(PG_MODE_MASK_DEFAULT);
I would still see that rather committed as a separate patch. I am fine
to delegate the decision to the committer who is hopefully going to pick
up this patch.
And more things about patch 1 which are not nits.
In pg_backup_tar.c, we still have a 0600 hardcoded. You should use
PG_FILE_MODE_DEFAULT there as well.
dsm_impl_posix() can create a new segment with shm_open using as mode
0600. This is present in pg_dynshmem, which would be included in
backups. First, it seems to me that this should use
PG_FILE_MODE_DEFAULT. Then, with patch 2 it seems to me that if an
instance is using DSM then there is a risk of breaking a simple backup
which uses for example "tar" without --exclude filters with group access
(sometimes scripts are not smart enough to skip the same contents as
base backups). So it seems to me that DSM should be also made more
aware of group access to ease the life of users.
And then for patch 2, a couple of issues spotted..
+ /* for previous versions set the default group access */
+ if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_GROUP_ACCESS)
+ {
+ DataDirGroupAccess = false;
+ return false;
+ }
Enforcing DataDirGroupAccess to false for servers older than v11 is
fine, but RetrieveDataDirGroupAccess() should return true. If I read
your patch correctly then support for pg_basebackup on older server
would just fail.
+#if !defined(WIN32) && !defined(__CYGWIN__)
+ if ((stat_buf.st_mode & PG_DIR_MODE_DEFAULT) == PG_DIR_MODE_DEFAULT)
+ {
+ umask(PG_MODE_MASK_ALLOW_GROUP);
+ DataDirGroupAccess = true;
+ }
This should use SetConfigOption() or I am missing something?
+/*
+ * Does the data directory allow group read access? The default is false, i.e.
+ * mode 0700, but may be changed in checkDataDir() to true, i.e. mode 0750.
+ */
+bool DataDirGroupAccess = false;
Instead of a boolean, I would suggest to use an integer, this is more
consistent with log_file_mode. Actually, about that, should we complain
if log_file_mode is set to a value incompatible?
--
Michael
