Re: pg_execute_from_file review

On Thu, Dec 2, 2010 at 20:00, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Please find attached the v8 version of the patch, that fixes the following:

I fixed and cleanup some of codes in it; v9 patch attached. Please check
my modifications, and set the status to "Ready to Committer" if you find
no problems. I think documentation and code comments might need to be
checked by native English speakers.

> 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:

Good idea. I re-added negative lengths checks in pg_read_file functions;
negative length is used internally, but not exposed as SQL functions.

>>   BTW, I think we can call it just "replace"
> 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.

You added replace(text, text, text, VARIADIC text), but I think
replace(text, VARIADIC text) also works. If we have the short form,
we can use it easily from execute functions with placeholders.

Other changes:
* Added some regression tests.
* Int64GetDatum((int64) fst.st_size) was broken.
* An error checks for "could not read file" didn't work.
* Read file contents into bytea buffer directly to avoid memcpy.
* Fixed bad usages of text and bytea values
because they are not null-terminated.
* I don't want to expose ArrayType to builtins.h.
So I call replace_text_variadic() from execute functions.
* pg_execute_sql_file(path, NULL) won't work because it's a STRICT function.
It returns NULL with no works when at least one of the argument is NULL.

BTW, we have many text from/to cstring conversions in the new codes.
It would be not an item for now, but we would need to improve them
if those functions are heavily used, Especially replace_text_variadic().

>> * 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 :)

Agreed. I also prefer pg_read_file_all rather than pg_read_whole_file :P

--
Itagaki Takahiro