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

Hi Michael,

Thanks for having a look at the patches.

On 1/30/18 3:01 AM, Michael Paquier wrote:
> On Mon, Jan 29, 2018 at 04:29:08PM -0500, David Steele wrote:
>>
>> Adds a *very* basic test suite for pg_resetwal.  I was able to make this
>> utility core dump (floating point errors) pretty easily with empty or
>> malformed pg_control files so I focused on WAL reset functionality plus
>> the basic help/command tests that every utility has.
>
> A little is better than nothing, so that's an improvement, thanks!
>
> +# Remove WAL from pg_wal and make sure it gets rebuilt
> +$node->stop;
> +
> +unlink("$pgwal/000000010000000000000001") == 1
> +   or die("unable to remove 000000010000000000000001");
> +
> +is_deeply(
> +   [sort(slurp_dir($pgwal))], [sort(qw(. .. archive_status))], 'no WAL');
> This is_deeply test serves little purpose.  The segment gets removed
> directly so I would just remove it.

The purpose of this test to be ensure nothing else is in the directory.
As the tests get more complex I think keeping track of the state will be
important.  In other words, this is really an assertion that the current
test state is correct, not that the prior command succeeded.

> Another test that could be easily included is with -o, then create a
> table and check for its OID value.  Also for -x, then just use
> txid_current to check the value consistency.  For -c, with
> track_commit_timestamp set to on, you can set a couple of values and
> then check for pg_last_committed_xact() which would be NULL if you use
> an XID older than the minimum set for example.  All those are simple
> enough so they could easily be added in the first version of this test
> suite.

I would prefer to leave this for another patch.

> You need to update src/bin/pg_resetxlog/.gitignore to include
> tmp_check/.

Added.

>> 2) 02-mkdir
>>
>> Converts mkdir() calls to MakeDirectoryDefaultPerm() if the original
>> call used default permissions.
>
> So the only call not converted to that API is in ipc.c...  OK, this one
> is pretty simple so nothing really to say about it, the real meat comes
> with patch 3.  I would rather see with a good eye if this patch
> introduces file_perm.h which includes both PG_FILE_MODE_DEFAULT and
> PG_DIR_MODE_DEFAULT, and have the frontends use those values.  This
> would make the switch to 0003 a bit easier if you look at pg_resetwal's
> diffs for example.

Agreed.  I thought about this originally but could not come up with a
good split.  I spent some time on it and came up with something that I
think works (and would make sense to commit separately).

>> 3) 03-group
>>
>> Allow group access on PGDATA.  This includes backend changes, utility
>> changes (initdb, pg_ctl, pg_upgrade, etc.) and tests for each utility.
>
> +++ b/src/include/common/file_perm.h
> + *
> + * This module is located in common so the backend can use the constants.
> + *
> This is the case of more or less all the content in src/common/, except
> for the things which are frontend-only, so this comment could just be
> removed.

Removed.

> +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().

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

> The efforts put in the tests are good.

Thanks!

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.

1) 01-pgresetwal-test

Adds a very basic test suite for pg_resetwal.

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.

3) 03-group

Allow group access on PGDATA.  This includes backend changes, utility
changes (initdb, pg_ctl, pg_upgrade, etc.) and tests for each utility.

Thanks!
--
-David
david@pgmasters.net