On Fri, Nov 26, 2010 at 06:24, Dimitri Fontaine <dimi...@2ndquadrant.fr> wrote:
> Thanks for your review. Please find attached a revised patch where I've
> changed the internals of the function so that it's split in two and that
> the opr_sanity check passes, per comments from David Wheeler and Tom Lane.

I have some comments and questions about pg_execute_from_file.v5.patch.

==== Source code ====
* OID=3627 is used by another function. Don't you expect 3927?

* There is a compiler warning:
  genfile.c: In function ‘pg_execute_from_file_with_placeholders’:
  genfile.c:427: warning: unused variable ‘query_string’

* I'd like to ask native speakers whether "from" is needed in names
  of "pg_execute_from_file" and "pg_execute_from_query_string".

==== Design and Implementation ====
* pg_execute_from_file() can execute any files even if they are not
  in $PGDATA. OTOH, we restrict pg_read_file() to read such files.
  What will be our policy?  Note that the contents of file might be
  logged or returned to the client on errors.

* Do we have any reasons to have pg_execute_from_file separated from
  pg_read_file ?  If we allow pg_read_file() to read files in $PGSHARE,
  pg_execute_from_file could be replaced with "EXECUTE pg_read_file()".
  (Note that pg_execute_from_file is implemented to do so even now.)

* I hope pg_execute_from_file (and pg_read_file) had an option
  to specify an character encoding of the file. Especially, SJIS
  is still used widely, but it is not a valid server encoding.

-- 
Itagaki Takahiro

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to