Re: Finer Extension dependencies

On Wed, Mar 28, 2012 at 3:09 PM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Could you possibly generate a new diff to save me the trouble of
>> fixing the one you sent before?
>
> Please find it attached, it looks better now, and I rebased it against
> master for good measure (no conflicts).

Comments:

I tested this out and it seems to work as designed.

I think the lack of pg_upgrade support is a must-fix before commit.
It seems to me that's a non-trivial problem, as I think you're going
to need to invent a way to jam the feature list into the stub
extension, either something like
binary_upgrade.add_feature_to_extension() or new syntax like ALTER
EXTENSION .. ADD FEATURE.

Don't we need some psql support for listing the features provided by
an installed extension? And an available extension?

get_extension_feature_oids seems to be misnamed, since it returns only
one OID, not more than one, and it also returns the feature OID. At a
minimum, I think you need to delete the s from the function name;
possibly it should be renamed to lookup_extension_feature or somesuch.

List *requires; /* names of prerequisite extensions */
+ List *provides; /* names of provided features */

Comment in requires line of above hunk should be updated to say
"prerequisite features".

@@ -4652,4 +4652,3 @@ DESCR("SP-GiST support for suffix tree over text");
#define PROARGMODE_TABLE 't'

#endif /* PG_PROC_H */
-

Useless hunk.

+ errmsg("parameter \"%s\" must be a
list of extension names",

This error, which relates to the parsing of the "provides" list, say
"extension features", and the existing code for "requires" needs to be
updated to say that as well.

+ errmsg("extension feature \"%s\"
already exists [%u]",
+ feature, featoid)));

That [%u] at the end there does not conform to our message style
guidelines, and I think the rest of the message could be improved a
bit as well. I think maybe it'd be appropriate to work the name of
the other extension into the message, maybe something like: extension
"%s" provides feature "%s", but existing extension "%s" already
provides this feature

+extern char * get_extension_feature_name(Oid featoid);

Extra space.

+ if (strcmp(curreq,pcontrol->name) != 0)

Missing space (after the comma).

OCLASS_EXTENSION_FEATURE should probable be added to the penultimate
switch case in ATExecAlterColumnType.

Gotta run, there may be more but I'm out of time to stare at this for
the moment.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company