Re: pg_execute_from_file review

On Thu, Nov 25, 2010 at 10:24:51PM +0100, Dimitri Fontaine wrote:
> Joshua Tolley <eggyknap(at)gmail(dot)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'll take a look ASAP.

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

Perhaps such docs will show up with the rest of the EXTENSION work, but I'd
like a brief mention somewhere.

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

Works for me.

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

My guess is "yes", because that was widely hailed as a good idea when I did
PL/LOLCODE. I suspect it would only matter if someone were using
pg_execute_from_file within some other function, which isn't entirely
unlikely.

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com