Re: PATCH: Configurable file mode mask - Mailing list pgsql-hackers

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