Re: pg_execute_from_file review

Itagaki Takahiro <itagaki.takahiro@gmail.com> writes:
> I think there are two topics here:
>   1. Do we need to restrict locations in which sql files are executable?
>   2. Which is better, pg_execute_sql_file() or EXECUTE pg_read_file() ?
>
> There are no discussion yet for 1, but I think we need some restrictions

Well, as a first level of restrictions, the function is superuser
only. I understand and share your concerns, but as the main use for this
function is in the extension's patch which is superuser only too, I feel
like this discussion could (should) be taken to after commit. We would
issue the next alpha with current coding, and the one after that with
more security thoughts in. Is it possible to attack it this way?

(The fear being that it might be hard to get to a common decision hereand we have other patches to care about depending
onthis one, thatwill continue working fine --- that's the goal --- once the securitypolicy land in. This one is the
mechanismpatch)
 

> For 2, I'd like to monofunctionalize pg_execute_sql_file() into
> separated functions something like:
> - FUNCTION pg_execute_sql(sql text)
> - FUNCTION replace(str text, from1 text, to1 text, VARIADIC text)
> - FUNCTION pg_read_binary_file(path text, offset bigint, size bigint)
>     (size == -1 means whole-file)
>
> pg_read_binary_file() is the most important functions probably.
> pg_execute_sql_file() can be rewritten as follows. We can also use
> existing convert_from() to support encodings.
>
> SELECT pg_execute_sql(
>          replace(
>            convert_from(
>              pg_read_binary_file('path', 0, -1),
>              'encoding'),
>          'key1', 'value1', 'key2', 'value2')
>        );

I think the current pg_execute_sql_file() is a better user interface,
but it could be extended to support queries in a text argument too, of
course. I proposed it and Robert said he's not thinking that should
happen in this very patch, if at all, if I understood correctly.

Again, I'd like to see pg_read_binary_file() and it's easy to expose the
other derivatives you're proposing here: the support code is already in
my patch and is organised this way internally. Now, is there an
agreement that all those new SQL functions this should be in the
pg_execute_from_file patch? If so, I'll prepare v7 with that.

Overall, I'd like it if it'd be possible to separate the concerns
directly relevant to the extension patch to other legitimate ones, so
that the main patch gets its share of review in this commitfest. But
maybe I'm over-worried here.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support