Skip site navigation (1) Skip section navigation (2)

Re: pg_execute_from_file review

From: Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr>
To: Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com>
Cc: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>, Joshua Tolley <eggyknap(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: pg_execute_from_file review
Date: 2010-11-29 09:26:35
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-hackers
Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)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:;a=commitdiff;h=b406fe35e36e6823e18f7c3157bc330b40b130d4;a=commitdiff;h=b04eda8f8ee05c3bb5f4d6b693c5169aa7c3b9d1

I missed including a part of the following patch into the
pg_execute_from_file branch:;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?

Dimitri Fontaine     PostgreSQL : Expertise, Formation et Support

In response to


pgsql-hackers by date

Next:From: Heikki LinnakangasDate: 2010-11-29 09:55:27
Subject: Re: [trivial patch] Ellipsis whitespace in SQL docs
Previous:From: Christoph BergDate: 2010-11-29 08:43:02
Subject: [trivial patch] Ellipsis whitespace in SQL docs

Privacy Policy | About PostgreSQL
Copyright © 1996-2017 The PostgreSQL Global Development Group