From: | Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>, Alvaro Herrera <alvherre(at)commandprompt(dot)com>, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Finer Extension dependencies |
Date: | 2012-03-29 09:21:44 |
Message-ID: | 87y5qjhidz.fsf@hi-media-techno.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
Thanks for your review!
Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I think the lack of pg_upgrade support is a must-fix before commit.
I though that would only be a TODO for 9.2 to 9.3 upgrades. When
upgrading from 9.1 to 9.2, pg_upgrade will directly stuff extensions
using InsertExtensionTuple() with an empty features list. That will call
insert_extension_features() which ensures that the extension name is
registered as a feature even when not explicitly listed (backward
compatibility with control files).
So I think we're covered now, but still need to revisit the issue later.
I edited the comment this way:
List *features = NIL; /* 9.3 should get features from catalogs */
> Don't we need some psql support for listing the features provided by
> an installed extension? And an available extension?
It's already in the pg_available_extension_versions system's view, which
is not covered by psql (yet). Do we want a new \dx derived command?
> 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.
Done.
> 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".
Done.
> -
>
> Useless hunk.
That must be my editor adding a final line when I visit files… and I
can't seem to be able to prevent this hunk from happening?
> + 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.
Done.
> + 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
Done.
> +extern char * get_extension_feature_name(Oid featoid);
>
> Extra space.
Fixed.
> + if (strcmp(curreq,pcontrol->name) != 0)
>
> Missing space (after the comma).
Fixed.
> OCLASS_EXTENSION_FEATURE should probable be added to the penultimate
> switch case in ATExecAlterColumnType.
Right, done now.
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
From | Date | Subject | |
---|---|---|---|
Next Message | Simon Riggs | 2012-03-29 09:37:54 | Re: Standbys, txid_current_snapshot, wraparound |
Previous Message | Andrey Velikoredchanin | 2012-03-29 08:57:38 | Re: Standbys, txid_current_snapshot, wraparound |