Re: PATCH: Configurable file mode mask

On 3/5/18 10:46 PM, Michael Paquier wrote:
> 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".

OK. extra is used to set -g and the group_access() function checks
pgdata directly since this can change after the cluster is created.

>>> 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.

Ah, I see what you mean now. Fixed using follow_fast. This variant
claims to be faster and it doesn't matter if files are occasionally
checked twice.

>>
>>> 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.

OK, I'll move it back for now, and make a note to raise the position as
a separate issue. I'd rather not do it in this fest, though.

> The indentation in RewindTest.pm is a bit wrong.

Fixed.

> - 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.

The goal of patch 2 is to refactor the way permissions are set by
replacing locally hard-coded values with centralized values, so I think
this makes sense here.

> my $test_mode = shift;
>
> + umask(0077);

Added. (Ensure that all files are created with default data dir
permissions).

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

Added. (Used to differentiate clusters).

> Perhaps the tests should be cut into a separate patch?

I prefer tests to be in the same patch as the code they test.

> Those are not
> directly related to the refactoring done in patch 2.

The point of the tests is to be sure there are no regressions in the
refactor so they seem directly related to me. Also, the tests
themselves were not to good about keeping permissions consistent.

> Patch 2 begins to look fine for me.

I also made chmod_recursive() generic.

New patches are attached.

Thanks!
--
-David
david(at)pgmasters(dot)net