Re: pg_execute_from_file review

On Thu, Dec 2, 2010 at 07:00, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)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 might contain 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 can fix 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 might be better for SQL.

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

* In docs, the example of replace_placeholders() is
replace('abcdefabcdef', 'cd', 'XX', 'ef', 'YY').
~~~~~~~
BTW, I think we 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