Re: pg_execute_from_file review

Joshua Tolley <eggyknap@gmail.com> writes:
> I've just looked at pg_execute_from_file[1]. The idea here is to execute all
> the SQL commands in a given file. My comments:

Thanks for your review. Please find attached a revised patch where I've
changed the internals of the function so that it's split in two and that
the opr_sanity check passes, per comments from David Wheeler and Tom Lane.

> * I'd like to see the docs slightly expanded, specifically to describe
>   parameter replacement. I wondered for a while if I needed to set of
>   parameters in any specific way, before reading the code and realizing they
>   can be whatever I want.

My guess is that you knew that in the CREATE EXTENSION context, it has
been proposed to use the notation @extschema@ as a placeholder, and
you've then been confused. I've refrained from imposing any style with
respect to what the placeholder would look like in the mecanism-patch.

Do we still want to detail in the docs that there's nothing expected
about the placeholder syntax of format?

> * Does anyone think it needs representation in the test suite?

Well the patch will get its tests with the arrival of the extension main
patch, where all contribs are installed using it.

> * Is it at all bad to include spi.h in genfile.c? IOW should this function
>   live elsewhere? It seems reasonable to me to do it as written, but I thought
>   I'd ask.

Well, using spi at this place has been asked by Álvaro and Tom, so my
guess is that's ok :)

> * In the snippet below, it seems best just to use palloc0():
>     query_string = (char *)palloc((fsize+1)*sizeof(char));
>     memset(query_string, 0, fsize+1);

Edited.

> * Shouldn't it include SPI_push() and SPI_pop()?

ENOCLUE

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