Stephen Frost <sfr...@snowman.net> writes:
> * Michael Paquier (mich...@paquier.xyz) wrote:
>> If the problem is parsing, it could as well be more portable to put that
>> in the control file, no?
> Then we'd need a tool to allow changing it for people who do want to
> change it. There's no reason we should have to have an extra tool for
> this- an administrator who chooses to change the privileges on the data
> folder should be able to do so and I don't think anyone is going to
> thank us for requiring them to use some special tool to do so for
> PostgreSQL.
FWIW, I took a quick look through this patch and don't have any problem
with the approach, which appears to be "use the data directory's
startup-time permissions as the status indicator". I am kinda wondering
about this though:
+ (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.
If we're allowing group read on the data directory, why should that not
include postmaster.pid? There's nothing terribly security-relevant in
there, and being able to find out the postmaster PID seems useful. I can
see the point of restricting server key files, as documented elsewhere,
but it's possible to keep those somewhere else so they don't cause
problems for simple backup software.
Also, in general, this patch desperately needs a round of copy-editing,
particularly with attention to the comments. For instance, there are a
minimum of three grammatical errors in this one comment:
+ * Group read/execute may optional be enabled on PGDATA so any frontend tools
+ * That write into PGDATA must know what mask to set and the permissions to
+ * use for creating files and directories.
In a larger sense, this fails to explain the operating principle,
namely what I stated above, that we allow the existing permissions
on PGDATA to decide what we allow as group permissions. If you
don't already understand that, you're going to have a hard time
extracting it from either file_perm.h or file_perm.c. (The latter
fails to even explain what the argument of DataDirectoryMask is.)
regards, tom lane
