|From:||David Steele <david(at)pgmasters(dot)net>|
|To:||Robert Haas <robertmhaas(at)gmail(dot)com>|
|Cc:||"Tsunakawa, Takayuki" <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Stephen Frost <sfrost(at)snowman(dot)net>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Adam Brightwell <adam(dot)brightwell(at)crunchydata(dot)com>|
|Subject:||Re: PATCH: Configurable file mode mask|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
Thanks for looking at the patches.
On 12/31/17 1:27 PM, Robert Haas wrote:
> On Thu, Dec 28, 2017 at 2:36 PM, David Steele <david(at)pgmasters(dot)net> wrote:
>> Attached is a new patch set that should address various concerns raised in
>> this thread.
>> 1) group-access-v3-01-mkdir.patch
>> Abstracts all mkdir calls in the backend into a MakeDirectory() function
>> implemented in fd.c. This did not get committed in September as part of
>> 0c5803b450e but I still think it has value. However, I have kept it
>> separate to reduce noise in the second patch. The mkdir() calls could also
>> be modified to use PG_DIR_MODE_DEFAULT with equivalent results.
> + * Default mode for created directories, unless something else is specified
> + * using the MakeDirectoryPerm() function variant.
> + */
> +#define PG_DIR_MODE_DEFAULT (S_IRWXU)
> +#define PG_MODE_MASK_DEFAULT (S_IRWXG | S_IRWXO)
> There's only one comment here, but there are two constants that do
> different things.
Looks like a copy pasto - I didn't mean PG_MODE_MASK_DEFAULT to be in
patch 01 at all. Removed.
> Also, this hunk gets removed again by 02, which is probably OK, but 02
> adds the replacement stuff in a different part of the file, which
> seems not as good.
Agreed. I have moved that.
> > I think MakeDirectory() is a good wrapper, but isn't
> MakeDirectoryPerm() sort of silly?
There's one place in the backend (storage/ipc/ipc.c) that sets
non-default directory permissions. This function is intended to support
that and any extensions that need to set custom perms.
>> 2) group-access-v3-02-group.patch
>> This is a "GUC-less" implementation of group read access that depends on the
>> mode of the $PGDATA directory to determine which mode to use for subsequent
>> writes. The initdb option is preserved to allow group access to be enabled
>> when the cluster is initialized.
>> Only two modes are allowed (700, 750) and the error message on startup is
>> hard-coded to address translation concerns.
> + umask(~(stat_buf.st_mode & PG_DIR_MODE_DEFAULT));
> Hmm, I wonder if this is going to be 100% portable. Maybe some
> obscure platform won't like an argument with all the high bits set.
Sure - I have masked this with 0777 to clear any high bits. Sound OK?
> Also, why should we break what's now one #if into two? That seems
> like it's mildly more complicated for no gain.
There were already two #ifdef blocks so I added a third. I think we
should leave it as three or combine them all into one. There are no
runtime performance implications so I'm agnostic.
> + (These files can confuse <application>pg_ctl</application>.) If group read
> + access is enabled on the data directory and an unprivileged user in the
> + <productname>PostgreSQL</productname> group is performing the backup, then
> + <filename>postmaster.pid</filename> will not be readable and must be
> + excluded.
> Is that actually the behavior we want?
I think it is. Comments in the code are pretty specific about not
changing the permissions on postmaster.pid and I don't see any reason to
mess with it. Copying postmaster.pid as part of a backup is not a good
New patches attached.
|Next Message||Tom Lane||2018-01-02 16:47:45||Re: [HACKERS] SQL procedures|
|Previous Message||Robert Haas||2018-01-02 16:40:26||Re: [HACKERS] SQL procedures|