Re: pg_execute_from_file review

Itagaki Takahiro <itagaki.takahiro@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