Re: Review: Extensions Patch

Kineticode Billing <david(at)kineticode(dot)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?

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

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

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

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.

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

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?

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

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

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