Re: PATCH: Configurable file mode mask

On Tue, Feb 27, 2018 at 03:52:32PM -0500, David Steele wrote:
> On 1/30/18 3:01 AM, Michael Paquier wrote:
> 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.

Good point. Objection removed.

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

OK, no problems with that either.

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

OK, thanks.

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

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

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

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

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

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

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

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.

- if (chmod(pg_data, S_IRWXU) != 0)
+ if (chmod(pg_data, PG_DIR_MODE_DEFAULT & ~file_mode_mask) != 0)
[...]
- if (chmod(xlog_dir, S_IRWXU) != 0)
+ if (chmod(xlog_dir, PG_DIR_MODE_DEFAULT & ~file_mode_mask) != 0)
Those seems incorrect, as they consider the default value only.

snprintf(path, sizeof(path), "%s/postgresql.conf", pg_data);

writefile(path, conflines);
- 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?

setup_signals();

- umask(S_IRWXG | S_IRWXO);
-
create_data_directory();
This should not be moved around.

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

I have not looked much in depth into this one, but I spotted two issues
on a quick read:
+ * Errors are not handled here and should be checked by the frontend
+ * application.
+ */
+#ifdef FRONTEND

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

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