Re: pg_execute_from_file review

Hi,

Please find attached the v8 version of the patch, that fixes the following:

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> * 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.

I've changed pg_read_binary_file_internal() to return bytea*, which is
much cleaner, thanks for the suggestion!

> * 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's now using the _internal function directly, so that there's only one
code definition to care about here.

> * (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.

Well thinking about it, omitting the length parameter alltogether seems
like the more natural SQL level API for me, so I've made it happen:

=# \df pg_read_*|replace|pg_exe*
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 | normal
pg_catalog | pg_read_binary_file | bytea | text, bigint, bigint | normal
pg_catalog | pg_read_file | text | text, bigint | normal
pg_catalog | pg_read_file | text | text, bigint, bigint | normal
pg_catalog | replace | text | text, text, text | normal
pg_catalog | replace | text | text, text, text, VARIADIC text | normal
(11 rows)

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

Agreed, fixed the doc.

> * 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.

The same idea occured to me yesternight when reading through the patch
to send. It's now done in the way you can see above. The idea is not to
change the existing behavior at all, so I've not changed the
non-VARIADIC version of the function.

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

Well, I think I prefer reading the verbs in the order that things are
happening in the code, it's actually convert then execute. But again,
maybe some Native Speaker will have a say here, or maybe your proposed
name fits better in PostgreSQL. I'd leave it for commiter :)

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support