On Thu, Nov 25, 2010 at 10:24:51PM +0100, Dimitri Fontaine wrote: > Joshua Tolley <eggyk...@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'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
signature.asc
Description: Digital signature