Re: Review: Extensions Patch

On Dec 8, 2010, at 1:39 AM, Dimitri Fontaine wrote:

> "David E. Wheeler" <david(at)kineticode(dot)com> writes:
>>> What about unaccent? Or lo (1 domain, 2 functions)?
>>
>> Sure. Doesn't have to actually do anything.
>
> Ok, so that's already in the patch :)

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.

> I see that you're not too much into packaging, but here, we don't ever
> use `make install` on a production machine. This step happens on the
> packaging server, then we install and QA the stuff, then the package
> gets installed on the servers where we need it.
>
> Also, I don't see how make install is going to know which cluster it
> should talk to — it's quite easy and typicall to run this command on a
> server where you have several major versions installed, and several
> clusters per major version.

Yeah, one installs extensions into a PostgreSQL instal, not a cluster. I get that.

> So, again, the data that you so want to remove from the control files I
> have no idea at all where to put it.

Okay, keep the installed control files. But don't make me distribute them unless absolutely necessary.

>> Possibly. I'm not going to do it this week; seems like there are some
>> issues that still need shaking out in the implementation, to judge
>> from the "pg_execute_from_file review" thread.
>
> Yeah, dust ain't settled completely yet… working on that.

Right.

>> Each would get a separate control file. The mapping of one version
>> number to multiple extensions is potentially confusing.
>
> Funny, each already get a separate control file now.
>
> $ ls contrib/spi/*control.in
> autoinc.control.in auto_username.control.in moddatetime.control.in
> refint.control.in timetravel.control.in
>
> Then the idea behind the version number in the Makefile is that you
> generally are maintaining it there and don't want to have to maintain it
> in more than one place.

Sure. But you're mandating one version even if you have multiple extensions. That's the potentially confusing part.

>> Why is that? We currently manage multiple script files, test files,
>> etc. in a single Makefile. Wildcard operators are very useful for this
>> sort of thing.
>
> Well, that was you saying just above that using the same EXTVERSION Make
> variable for more than one control file is "potentially confusing". What
> about using all the other variables in the same way?

What? I don't follow what you're saying.

>> Yes, that would be preferable, but a one-step operation would of
>> course be ideal.
>
> Thinking about it, as proposed in the other thread, I now think that the
> 2-steps operation could be internal and not user exposed.

Maybe. I'm still not convinced that you need the replace() stuff at all, though I can see the utility of it.

>> Some do require shared_preload_libraries, no?
>
> One of them only, pg_stat_statements.

In contrib. You seem to forget that there are a lot of third-party extensions out there already.

>> SET client_min_messages TO warning;
>> SET log_min_messages TO warning;
>>
>> Though I think I'd rather that the warning still went to the log.
>
> (that's about hstore verbosity) ok will see about changing
> client_min_messages around the CREATE OPERATOR =>.

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.

Best,

David