Re: Add default role 'pg_access_server_files'

On Wed, Mar 07, 2018 at 07:00:03AM -0500, Stephen Frost wrote:
> * Michael Paquier (michael(at)paquier(dot)xyz) wrote:
>> First, could it be possible to do a split for 1) and 2)?
>
> Done, because it was less work than arguing about it, but I'm not
> convinced that we really need to split out patches to this level of
> granularity. Perhaps something to consider for the future.

One patch should defend one idea, this makes the git history easier to
understand in my opinion.

> Consider a case like postgresql.conf residing outside of the data
> directory. For an application to be able to read that with
> pg_read_file() is very straight-forward and applications already exist
> which do. Forcing that application to go through COPY would require
> creating a TEMP table and then coming up with a COPY command that
> doesn't actually *do* what COPY is meant to do- that is, parse the file.
> By default, you'd get errors from such a COPY command as it would think
> there's extra columns defined in the "copy-format" or "csv" or
> what-have-you file.

Hm, OK...

>> Could you update the documentation of pg_rewind please? It seems to me
>> that with your patch then superuser rights are not necessary mandatory,
>
> This will require actual testing to be done before I'd make such a
> change. I'll see if I can do that soon, but I also wouldn't complain if
> someone else wanted to actually go through and set that up and test that
> it works.

That's what I did, manually, using the attached SQL script with your
patch 1 applied. You can check for the list of functions used by
pg_rewind in libpq_fetch.c where you just need to grant execution access
to those functions for a login user and you are good to go:
pg_ls_dir(text, boolean, boolean)
pg_stat_file(text, boolean)
pg_read_binary_file(text)
pg_read_binary_file(text, bigint, bigint, boolean)

So getting this documented properly would be nice. Of course feel free
to test this by yourself as you wish.

Could you send separate files for each patch by the way? This eases
testing, and I don't recall that git am has a way to enforce only a
subset of patches to be applied based on one file, though my git-fu is
limited in this area.

+ read or which program is run. In principle regular users could be allowed to
change the other options, but that's not supported at present.
Well, the parsing of file_fdw complains if "program" or "filename" is
not defined, so a user has to be in either pg_read_server_files to use
"filename" or in pg_execute_server_program to use "program", so I am not
sure that the last sentence of this paragraph makes much sense from the
beginning.

- Return the contents of a file.
+ Return the contents of a file. Restricted to superusers by
default, but other users can be granted EXECUTE to run the function.
</entry>
You should make those paragraphs spawn into multiple lines. EXECUTE
should use <literal> markup, and I think that you should use "EXECUTE
privilege to run the function" on those doc portions. That's all for
the nits.

Other than that the patch looks in pretty good shape to me.
--
Michael