Re: PATCH: Configurable file mode mask

On 2/28/18 2:28 AM, Michael Paquier wrote:
> On Tue, Feb 27, 2018 at 03:52:32PM -0500, David Steele wrote:
>> On 1/30/18 3:01 AM, Michael Paquier wrote:
>
>>> +command_ok(
>>> + ['chmod', "-R", 'g+rX', "$pgdata"],
>>> + 'add group perms to PGDATA');
>>>
>>> That would blow up on Windows. You should replace that by perl's chmod
>>> and look at its return result for sanity checks, likely with ($cnt != 0).
>>> And let's not use icacls please...
>>
>> Fixed with a new function, add_pg_data_group_perm().
>
> That's basically a recursive chmod, so chmod_recursive is more adapted?
> I could imagine that this is useful as well for removing group
> permissions, so the new mode could be specified as an argument.

The required package (File::chmod::Recursive) for chmod_recursive is not
in use anywhere else and was not installed when I installed build
dependencies.

I'm not sure what the protocol for introducing a new Perl module is? I
couldn't find packages for the major OSes. Are we OK with using CPAN?

>>> + if ($params{has_group_access})
>>> + {
>>> + push(@{$params{extra}}, '-g');
>>> + }
>>> No need to introduce a new parameter here, please use directly extra =>
>>> [ '-g' ] in the routines calling PostgresNode::init.
>>
>> Other code needs to know if group access is enabled on the node (see
>> RewindTest.pm) so it's not just a matter of passing the option.
>
> 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?

>> New patches are attached. The end result is almost identical to v6 but
>> code was moved from 03 to 02 as per Michael's suggestion.
>
> Thanks for the updated versions.
>
>> 1) 01-pgresetwal-test
>>
>> Adds a very basic test suite for pg_resetwal.
>
> Okay. This one looks fine to me. It could be passed down to a
> committer.

Agreed.

>> 2) 02-file-perm
>>
>> Adds a new header (file_perm.h) and makes all front-end utilities use
>> the new constants for setting umask. Converts mkdir() calls in the
>> backend to MakeDirectoryDefaultPerm() if the original
>> call used default permissions. Adds tests to make sure permissions in
>> PGDATA are set correctly by the front-end tools.
>
> I had a more simple (complicated?) thing in mind for the split, which
> would actually cause this patch to be split into two more:
> 1) Introduce file_perm.h and replace all the existing values for modes
> and masks to what the header introduces. This allows to check if the
> new header is not forgotten anywhere, especially for the frontends.
> 2) Introduce MakeDirectoryDefaultPerm, which switches the backend calls
> of mkdir() to the new API.
> 3) Add the grouping facilities, which updates the default masks and
> modes of file_perm.h.
>
> Grouping 1) and 2) together as you do makes sense as well I guess, as in
> my proposal the mkdir calls in the backend would be touched twice, still
> things get more incremental.

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.

>
> +/*
> + * Default mode for created files.
> + */
> +#define PG_FILE_MODE_DEFAULT (S_IRUSR | S_IWUSR | S_IRGRP)
> +
> +/*
> + * Default mode for directories.
> + */
> +#define PG_DIR_MODE_DEFAULT (S_IRWXU | S_IRGRP | S_IXGRP)
> For the first cut, this should not use S_IRGRP in the first declaration,
> and (S_IXGRP | S_IXGRP) in the second declaration.

Done.

>
> - if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
> + if (script == NULL && (script = fopen(output_path, "w")) == NULL)
> Why are those calls in pg_upgrade changed?

Done.

Those should have been removed already. Originally I had removed
fopen_priv() because it didn't serve a purpose anymore. In the
meantime, somebody else made it a macro to fopen(). I decided my patch
would be less noisy if I reverted to fopen_priv() but I missed a few places.

> TestLib.pm does not need the group-related extensions, right?

Done.

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

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

> What you need to do for frontend-only files in src/common/ is to define
> that at the top and not bother about FRONTEND cflags at all:
> #ifndef FRONTEND
> #error "This file is not expected to be compiled for backend code"
> #endif

Fixed.

> src/tools/msvc/Mkvcbuild.pm also needs to be updated with this new file,
> or you will break MSVC build.

Fixed.

New patches attached.

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