Re: PATCH: Configurable file mode mask

On Thu, Mar 01, 2018 at 09:32:58PM -0500, David Steele wrote:
> On 3/1/18 9:04 PM, Andres Freund wrote:
>> I'd suggest just using git format-patch -v to format series.
>
> Sure, I've been meaning to switch over to this format and just haven't
> gotten around to it yet. I do recognize the value, however, and will do it
> going forward.

Simple patches can bypass on that, but once you have at least three
patches this helps a lot with reviews. Explaining as well in the email
sending the patches what each patch actually does, even roughly helps in
focusing on one element or the other.

>> E.g. here it's far from obvious why something is done with
>> pg_resetwal.
> [reordered by me for clarity]
>
> Any front-end tool that writes into PGDATA is affected by this patch because
> mode must be set correctly.
>
> pg_resetwal had no tests, so I wrote a basic test suite to base the group
> permissions tests on. I included the basic test suite as a separate patch
> because I felt it should be committed even if the main feature wasn't. It's
> far from a comprehensive test suite, but certainly better than what we have
> currently.

Based on my recent lookup at code level for this feature, the patch for
pg_resetwal (which could have been discussed on its own thread as well),
would be fine for commit. The thing could be extended a bit more but
there is nothing opposing even a basic test suite to be in. Then you
have a set of refactoring patches, which still need some work. And
finally there is a rather invasive patch on top of the whole thing. The
refactoring work shows much more value only after the main feature is
in, still I think that unifying the default permissions allowed for
files and directories, as well as mkdir() calls has some value in
itself to think it as an mergeable, independent, change. I think that
it would be hard to get the whole patch set into the tree by the end of
the CF though, but cutting the refactoring pieces would be doable. At
least it would provide some base for integration in v12. And the
refactoring patch has some pieces that would be helpful for TAP tests as
well.
--
Michael