Re: pg_execute_from_file review

On Thu, Dec 2, 2010 at 07:00, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
> Here's the result:
>
> dim=# \df pg_exe*|replace_*|*binary*
>                                     List of functions
>    Schema   |         Name          | Result data type |    Argument data types    |  Type
> ------------+-----------------------+------------------+---------------------------+--------
>  pg_catalog | pg_execute_sql_file   | void             | text                      | normal
>  pg_catalog | pg_execute_sql_file   | void             | text, name                | normal
>  pg_catalog | pg_execute_sql_file   | void             | text, name, VARIADIC text | normal
>  pg_catalog | pg_execute_sql_string | void             | text                      | normal
>  pg_catalog | pg_execute_sql_string | void             | text, VARIADIC text       | normal
>  pg_catalog | pg_read_binary_file   | bytea            | text, bigint, bigint      | normal
>  pg_catalog | replace_placeholders  | text             | text, VARIADIC text       | normal
> (7 rows)

Thanks, good job!

* pg_read_binary_file_internal() should return not only the contents as char * but also the length, because the file
mightcontain 0x00. In addition, null-terminations for the contents buffer is useless. 

* The 1st argument of pg_convert must be bytea rather than cstring in pg_convert_and_execute_sql_file(). I think you
canfix both the bug and the above one if pg_read_binary_file_internal() returns bytea. 

* pg_read_file() has stronger restrictions than pg_read_binary_file(). (absolute path not allowed) and -1 length is not
supported.We could fix pg_read_file() to behave as like as pg_read_binary_file(). 

* (It was my suggestion, but) Is it reasonable to use -1 length to read the while file? It might fit with C, but NULL
mightbe better for SQL. 

* The doc says pg_execute_sql_string() is restricted for superusers, but is not restricted actually. I think we don't
haveto. 

* In docs, the example of replace_placeholders() is replace('abcdefabcdef', 'cd', 'XX', 'ef', 'YY'). ~~~~~~~ BTW, I
thinkwe can call it just "replace" because parser can handle them correctly even if we have both replace(text, text,
text)and replace(text, VARIADIC text[]). We will need only one doc for them. Note that if we call replace() with 3
args,the non-VARIADIC version is called. So, there is no performance penalty. 

* We might rename pg_convert_and_execute_sql_file() to pg_execute_sql_file_with_encoding() or so to have the same
prefix.

--
Itagaki Takahiro