Re: OpenFile() Permissions Refactor

From: David Steele <david(at)pgmasters(dot)net>
To: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: OpenFile() Permissions Refactor
Date: 2017-09-13 14:26:43
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hi Peter,

Here's a new patch based on your review. Where I had a question I made
a choice as described below:

On 9/1/17 1:58 PM, David Steele wrote:
> On 9/1/17 1:15 PM, Peter Eisentraut wrote:
>> On 8/29/17 12:15, David Steele wrote:
>> I wonder whether we even need that much flexibility. We already set a
>> global umask, so we could just open all files with 0666 and let the
>> umask sort it out. Then we don't need all the *Perm() variants.
> Well, there's one instance where the *Perm is used:
> diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
> - fd = OpenTransientFile(fnamebuf, O_CREAT | O_WRONLY | O_TRUNC | PG_BINARY,
> + fd = OpenTransientFilePerm(fnamebuf, O_CREAT | O_WRONLY | O_TRUNC |
> I also think it's worth leaving the variants for extensions to use.
> Even though there are no calls in the core extensions it's hard to say
> what might be out there in the field.

These have been left in.

>> I don't like the function-like macros in fd.h. We can use proper functions.
> I had them as functions originally but thought macros might be
> friendlier with compilers that don't inline. I'm happy to change them back.

Macros have been converted to functions.

>> I also wonder whether the umask save-and-restore code in copy.c and
>> be-fsstubs.c is sound. If the AllocateFile() call in between errors
>> out, then there is nothing that restores the original umask. This might
>> need a TRY/CATCH block, or maybe just a chmod() afterwards.
> Unless I'm mistaken this is a preexisting issue. Would you prefer I
> submit a different patch for that or combine it into this patch?
> The chmod() implementation seems the safer option to me and produces
> fewer code paths. It also prevents partially-written files from being
> accessible to any user but postgres.

I went with chmod(). The fix is incorporated in this patch but if you
want it broken out let me know.

>> The mkdir() calls could perhaps use some further refactoring so you
>> don't have to specify the mode everywhere.
> I thought about that but feared it would be considered an overreach.
> Does fd.c seem like a good place for the new function?

New functions MakeDirectory() and MakeDirectoryPerm() have been added to

MakeDirectoryPerm() is used in ipc.c.

>> This kind of code:
>> - if (stat_buf.st_mode & (S_IRWXG | S_IRWXO))
>> + if (stat_buf.st_mode & PG_MODE_MASK_DEFAULT)
>> ereport(FATAL,
>> errmsg("data directory \"%s\" has group or world access",
>> DataDir),
>> - errdetail("Permissions should be u=rwx (0700).")));
>> + errdetail("Permissions should be (%04o).",
>> can be problematic, because we are hoping that PG_MODE_MASK_DEFAULT,
>> PG_DIR_MODE_DEFAULT, and the wording in the error message can stay
>> consistent.
> Well, the eventual goal is to make the mask/mode configurable - at least
> to the extent that group access is allowed. However, I'm happy to leave
> that discussion for another day.

Changes to postmaster.c have been reverted (except to rename mkdir to

Patch v2 is attached.


Attachment Content-Type Size
file-open-perm-refactor-v2.patch text/plain 35.6 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Andreas Joseph Krogh 2017-09-13 14:31:09 Re: Clarification in pg10's pgupgrade.html step 10 (upgrading standby servers)
Previous Message Pavel Stehule 2017-09-13 14:18:10 Re: psql: new help related to variables are not too readable