Re: pg_execute_from_file review

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

Many thanks, that version is so much cleaner than the previous
one. Comments above.

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

So we now have the following:

List of functions
Schema | Name | Result data type | Argument data types | Type
------------+---------+------------------+---------------------+--------
pg_catalog | replace | text | text, VARIADIC text | normal
pg_catalog | replace | text | text, text, text | normal

My understanding is that the variadic form shadows the other one in a
way that it's now impossible to call it from SQL level. That's the
reason why I did the (text, text, text, VARIADIC text) version before,
but is it true? Also, is it worthwhile to keep the non VARIADIC
version exposed at SQL level?

The only other nitpicking I seem to be able to find is that you forgot
to remove the following from builtins.h:

+ extern Datum replace_placeholders(PG_FUNCTION_ARGS);

So I'll go update the commitfest to point to your version of the patch,
add an entry for this "comments" email, and mark as ready for commiter

> Other changes:

All for the best, thank you! I can't help but noticing that some of them
are fixing things that we could want to backpatch. Those:

> * Int64GetDatum((int64) fst.st_size) was broken.
> * An error checks for "could not read file" didn't work.

That's straight from master's branch code, IIRC.

> * Added some regression tests.
> * 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.

Not sure to understand this last point, because I already had 3 versions
of it, so surely you would call pg_execute_sql_file(path) in this case?

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

That could become a concern if it actually shadows the other version.

> Agreed. I also prefer pg_read_file_all rather than pg_read_whole_file :P

Going in this line of thought, maybe we should provide a third variant
here, the "real" pg_read_whole_file(path), then we have the existing
other variants, pg_read_file_to_end(path, offset) and the historic one,
pg_read_file(path, offset, bytes_to_read).

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