Itagaki Takahiro <itagaki.takah...@gmail.com> writes: > I have some comments and questions about pg_execute_from_file.v5.patch.
Thanks for reviewing it! > ==== Source code ==== > * OID=3627 is used by another function. Don't you expect 3927? Yes indeed. It took me some time to understand what's happening here, and it seems to be a case of git branches management from me. Here's the patch as I worked on it (that's much easier to test against the full branch, that's extension), then as it got cherry-picked into the branch I use to produce the patches: http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=commitdiff;h=b406fe35e36e6823e18f7c3157bc330b40b130d4 http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=commitdiff;h=b04eda8f8ee05c3bb5f4d6b693c5169aa7c3b9d1 I missed including a part of the following patch into the pg_execute_from_file branch: http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=commitdiff;h=7b3fe8384fb7636130e50f03a338f36495e15030 Sorry about that, will get fixed in v6 — already fixed in the git branch. > * There is a compiler warning: > genfile.c: In function ‘pg_execute_from_file_with_placeholders’: > genfile.c:427: warning: unused variable ‘query_string’ Fixed (in the git branches), sorry about that. > * I'd like to ask native speakers whether "from" is needed in names > of "pg_execute_from_file" and "pg_execute_from_query_string". Fair enough, will wait for some comments before producing a v6. > ==== 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.) pg_execute_from_file started as a very different animal, and I can see about it using pg_read_file() now, if we also change restrictions. Also, note that before using SPI an execute failure didn't ouput the whole input file. > * 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. What we agreed on doing in the extension's main patch was to change the client_encoding before to call into pg_execute_from_file(). So I'd hope that changing that client side just before to call pg_execute_from_file would be enough. Is it? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers