Re: Review: Extensions Patch

On Dec 8, 2010, at 12:42 PM, Dimitri Fontaine wrote:

> Kineticode Billing <david@kineticode.com> writes:
>> No, it's not. There are no unit tests at all. You can call the contrib
>> modules and their tests acceptance tests, but that's not the same
>> thing.
>
> Ok, I need some more guidance here. All contrib extension (there are 38
> of them) are using the CREATE EXTENSION command and checking the result
> with the pg_regress framework. What are we missing?

unit tests. You add a bunch of functions. You need to test those functions.

> I can see about adding DROP EXTENSION for all the tests, but that's
> about it.

If you add that, you'll also need something to CREATE EXTENSION with, eh? And also, tests to make sure that WITH SCHEMA
worksproperly (however that shakes out). 

>> Okay, keep the installed control files. But don't make me distribute
>> them unless absolutely necessary.
>
> Yes you have to distribute them, that's necessary. Sorry about that.

I don't see why. Most of them are dead simple and could easily be Makefile variables.

>> Sure. But you're mandating one version even if you have multiple
>> extensions. That's the potentially confusing part.
>
> I see how confusing it is, because what you say ain't true. You can
> always put different version numbers in the control file and even skip
> the rule to produce the .control from the .control.in by providing the
> .control directly. That's just a facility here.

I see, okay.

>> What? I don't follow what you're saying.
>
> You're complaining that a single EXTVERSION applied to more than one
> extension's control file is confusing. What if we had EXTCOMMENT and
> EXTRELOCATABLE in there too? What exactly are you expecting the Makefile
> to look like?

Mostly these will all have only one setting. In more complex cases perhaps one *would* be required to distribute a
controlfile. 

>> In contrib. You seem to forget that there are a lot of third-party
>> extensions out there already.
>
> True. That's still not the common case, and it's still covered the same
> way as before, you need to restart to attach to shared memory.

Okay.

>> I would much rather retain that warning -- everyone should know about
>> it -- and somehow convince SPI to be much less verbose in reporting
>> issues. It should specify where the error came from (which query) and
>> what the error actually is.
>
> The problem is much more complex than that and could well kill the patch
> if we insist on fixing it as part of the extension's work, v1. The
> problem is exposing more internals of the SQL parser into SPI so that
> you can send a bunch of queries in an explicit way.  Mind you, the
> firsts version of the patch had something like that in there, but that
> wouldn't have supported this use case. I've been told to simply use SPI
> there.

I agree that SPI should be fixed in a different project/patch. Go with what you've got, it will just highlight the
problemwith SPI more. 

Best,

David