On Thu, Apr 05, 2018 at 12:08:15PM -0400, David Steele wrote: > On 4/5/18 2:55 AM, Michael Paquier wrote: >> On Wed, Apr 04, 2018 at 08:03:54PM -0400, David Steele wrote: >> >>> Instead I have created variables in file_perm.c >>> that hold the current file create mode, dir create mode, and mode mask. >>> All call sites use those variables (e.g. pg_dir_create_mode), which I >>> think are much clear. >> >> Hm. I personally find that even more confusing, especially with >> SetDataDirectoryCreatePerm which basically is the same as >> SetConfigOption for the backend. > > The GUC shows the current mode of the data directory, while the > variables in file_perm.c store the mode that should be used to create > new dirs/files. One is certainly based on the other but I thought it > best to split them for clarity.
Yeah, there are arguments to actually have both things split so as they can be concatenated. This makes the code more modular. >> You also forgot a call to SetDataDirectoryCreatePerm or >> pg_dir_create_mode remains to its default. My apologies, I forgot the last two words of this sentence: "for pg_rewind". Still, I am wrong because GetDataDirectoryCreatePerm calls internally SetDataDirectoryCreatePerm. So the API name is confusing because not only the permissions are fetched, but they are also applied. > Are saying *if* a call is forgotten? That applies as well. >> The interface of file_perm.h that you are introducing is not confusing >> anymore though.. > > Yes, that was the idea. I think it makes it clearer what we are trying > to do and centralizes variables related to create modes in one place. > > I've attached new patches that exclude Windows from permissions tests > and deal with bootstrap permissions in a better way. PostgresMain() and > AuxiliaryProcessMain() now use checkDataDir() to set permissions. GetDataDirectoryCreatePerm() is defined in file_perm.h but it is only declared for frontends. At the end, this patch brings in a new abstraction concept to src/common/ to control a set of pre-determined behaviors: - The mode to create directories. - The mode to create files. - The mode number which can be used for umask. I am not really convinced that we need to enforce all of them all the time with a API layer aimed at controlling the behavior of all things at the same time. Getting this abstraction down one level by allowing each frontend to set up things the way they want would be nicer in my opinion. Perhaps others feel differently... It could be also less confusing if the set of variables in file_perm.h was designed in such a way that we have the default, and then we can apply on top of it the set to allow grouping, so as allowing grouping access would be to do the operation of (default mask + group addition). The design on the backend is rather neat however there is also overlapping with SetDataDirectoryCreatePerm() and the GUC data_directory_mode which are heading toward the same types of goals. We could reduce that confusion by designing the interface as follows: - Have read-only GUCs for the directory and file masks on the backend which get set by the postmaster after looking at the permission of the data folder at startup. Then if a file or a folder needs to be created, then look at those values and apply permissions as granted. And also a GUC to decide the umask to apply but that should not be necessary, right? - Frontends are responsible for the decision-making of the permissions. Not all the variables are used for frontends as well. For example pg_resetwal and pg_upgrade only touch files. - For frontends, there are two cases: -- The client needs to connect to a live Postgres instance, in which case it can use the SHOW command to get the wanted information. This applies to pg_rewind with the remote mode (should apply to the second case actually), pg_basebackup, pg_receivexlog, etc. -- Binaries work on a local data folder, so permissions can be guessed from that: pg_rewind, pg_resetwal and pg_upgrade. Having an API in src/common/ which scans for what to apply is neat. This was in v12 and some older versions if I recall correctly. We are two days away from the end of the commit fest, and this patch set if not yet in a committable stagle, so perhaps at this stage we had better just retreat and come back to it in next cycle? -- Michael
signature.asc
Description: PGP signature
