Re: PATCH: Configurable file mode mask

On Mon, Mar 05, 2018 at 03:07:20PM -0500, David Steele wrote:
> On 2/28/18 2:28 AM, Michael Paquier wrote:
>> On Tue, Feb 27, 2018 at 03:52:32PM -0500, David Steele wrote:
>> I don't quite understand here. I have no objection into extending
>> setup_cluster() with a group_access argument so as the tests run both
>> the group access and without it, but I'd rather keep the low-level API
>> clean of anything fancy if we can use the facility which already
>> exists.
>
> I'm not sure what you mean by, "facility that already exists". I think
> I implemented this the same as the allows_streaming and has_archiving
> flags. The only difference is that this option must be set at initdb
> time rather than in postgresql.conf.
>
> Could you be more specific?

Let's remove has_group_access from PostgresNode::init and instead use
existing parameter called "extra". In your patch, this setup:
has_group_access => 1
is equivalent to that:
extra => [ -g ]
You can also guess the value of has_group_access by parsing the
arguments from the array "extra".

> I think I prefer grouping 1 and 2. It produces less churn, as you say,
> and I don't think MakeDirectoryDefaultPerm really needs its own patch.
> Based on comments so far, nobody has an objection to it.
>
> In theory, the first two patches could be applied just for refactoring
> without adding group permissions at all. There are a lot of new tests
> to make sure permissions are set correctly so it seems like a win
> right off.

Okay, that's fine for me.

>> check_pg_data_perm() looks useful even without $allow_group even if the
>> grouping facility is reverted at the end. S_ISLNK should be considered
>> as well for tablespaces or symlinks of pg_wal? We have such cases in
>> the regression tests.
>
> Hmmm, the link is just pointing to a directory whose permissions have
> been changed. Why do we need to worry about the link?

Oh, perhaps I misread your code here, but this ignores symlinks, for
example take an instance where pg_wal is symlinked, we'd likely want to
make sure that at least archive_status is using a correct set of
permissions, no? There is a "follow" option in File::Find which could
help.

>> - if (chmod(path, S_IRUSR | S_IWUSR) != 0)
>> - {
>> - fprintf(stderr, _("%s: could not change permissions of \"%s\": %s\n"),
>> - progname, path, strerror(errno));
>> - exit_nicely();
>> - }
>> Hm. Why are those removed?
>
> When I started working on this, pg_upgrade did not set umask and instead
> did chmod for each dir created. umask() has since been added (even
> before my patch) and so these are now noops. Seemed easier to remove
> them than to change them all.
>
>> setup_signals();
>>
>> - umask(S_IRWXG | S_IRWXO);
>> -
>> create_data_directory();
>> This should not be moved around.
>
> Hmmm - I moved it much earlier in the process which seems like a good
> thing. Consider if there was a option to fixup permissions, like there
> is to fsync. Isn't it best to set the mode as soon as possible to
> prevent code from being inserted before it?

Those two are separate issues. Could you begin a new thread on the
matter? This will attract more attention.

The indentation in RewindTest.pm is a bit wrong.

- if (chmod(location, S_IRWXU) != 0)
+ current_umask = umask(0);
+ umask(current_umask);
[...]
- if (chmod(pg_data, S_IRWXU) != 0)
+ if (chmod(pg_data, PG_DIR_MODE_DEFAULT & ~file_mode_mask) != 0)
Such modifications should be part of patch 3, not 2, where the group
features really apply.

my $test_mode = shift;

+ umask(0077);
+
RewindTest::setup_cluster($test_mode);
What's that for? A comment would be welcome.

+# make sure all directories and files have group permissions
+if [ $(find ${PGDATA} -type f ! -perm 600 | wc -l) -ne 0 ]; then
+ echo "files in PGDATA with permission != 600";
+ exit 1;
+fi
Perhaps on hold if we are able to move pg_upgrade tests to a TAP
suite... We'll see what happens.

Perhaps the tests should be cut into a separate patch? Those are not
directly related to the refactoring done in patch 2.

Patch 2 begins to look fine for me. I still need to look at patch 3 in
more details.
--
Michael