Re: PATCH: Configurable file mode mask - Mailing list pgsql-hackers
From | David Steele |
---|---|
Subject | Re: PATCH: Configurable file mode mask |
Date | |
Msg-id | 2e36b84d-a2b0-0159-7ad1-194c4b596891@pgmasters.net Whole thread Raw |
In response to | Re: Re: PATCH: Configurable file mode mask (Robert Haas <robertmhaas@gmail.com>) |
Responses |
Re: PATCH: Configurable file mode mask
|
List | pgsql-hackers |
Hi Robert, 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@pgmasters.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 idea anyway. New patches attached. Thanks! -- -David david@pgmasters.net
Attachment
pgsql-hackers by date: