Re: PATCH: Configurable file mode mask

On Mon, Jan 29, 2018 at 04:29:08PM -0500, David Steele wrote:
> OK, that being the case I have piggy-backed on the current pg_upgrade
> tests in the same way that --wal-segsize did.
>
> There are now three patches:
>
> 1) 01-pgresetwal-test
>
> 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.

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.

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

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

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

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

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

The efforts put in the tests are good.

Patch 0003 needs a very careful lookup... We don't want to introduce any
kind of race conditions here. I am not familiar enough with the
proposed patch but the patch is giving me some chills.

You may want to run pgindent once on your patch set.
--
Michael