Thread: Finer Extension dependencies
Hi, The extensions work we began in 9.1 is not yet finished entirely (*cough*), so I'm opening a new patch series here by attacking the dependency problems. Some people want us to manage extension version numbers with sorting semantics so that we are able to depend on foo >= 1.2 and crazy things like this, and I think the need is reasonable and easier than that to address. I'm proposing a patch that implements a very simple concept, yet powerful enough to express very complex dependencies: - extensions are allowed to provide a list of named features - extensions now require one or more feature names With that tool in hands, you can reliably depend on some feature you need rather than having to check a feature matrix to see for yourself which version number implements it then paste that number into your dependency rules. It's also easier to deprecate a feature, just remove its name from the provide list in your extension's control file (remember you can have one of those per version, with overriding parameters). alter extension update will then fire the dependency resolution mechanism and bail out when another extension requires a feature you want to remove. The default behavior for an extension having its control file "provides" parameter not set is to provide only one feature named the same as the extension itself. In fact whatever happens, the extension always provides at least that feature. Which means that by default, it all works the same as it does in 9.1. So please find attached the said patch, which you can also prefer to browse online: https://github.com/dimitri/postgres/compare/master...extfeats I think it would be good to commit a follow-up patch exposing the PostgreSQL core feature matrix with the proposed support here, so that extension can require core features easily too. Maybe those documents would be a start: http://www.postgresql.org/docs/9.1/static/features-sql-standard.html http://www.postgresql.org/about/featurematrix/ Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachment
On Sun, Dec 18, 2011 at 6:36 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Hi, > > The extensions work we began in 9.1 is not yet finished entirely > (*cough*), so I'm opening a new patch series here by attacking the > dependency problems. > > Some people want us to manage extension version numbers with sorting > semantics so that we are able to depend on foo >= 1.2 and crazy things > like this, and I think the need is reasonable and easier than that to > address. The patch applies with one reject, which I could fix easily. The make check passed. extension-provides.v1.patch extensions.docx haradh1-mac:postgresql-head haradh1$ patch -p1 < ~/Downloads/extension-provides.v1.patch patching file contrib/pg_upgrade_support/pg_upgrade_support.c patching file doc/src/sgml/catalogs.sgml Hunk #2 succeeded at 3063 (offset 11 lines). Hunk #3 succeeded at 6865 (offset 11 lines). patching file doc/src/sgml/extend.sgml patching file src/backend/catalog/Makefile patching file src/backend/catalog/dependency.c patching file src/backend/catalog/system_views.sql patching file src/backend/commands/extension.c patching file src/include/catalog/dependency.h patching file src/include/catalog/indexing.h patching file src/include/catalog/pg_extension_feature.h patching file src/include/catalog/pg_proc.h Hunk #1 succeeded at 4341 (offset 25 lines). patching file src/include/commands/extension.h patching file src/test/regress/expected/rules.out patching file src/test/regress/expected/sanity_check.out Hunk #1 succeeded at 103 (offset 1 line). Hunk #2 FAILED at 163. 1 out of 2 hunks FAILED -- saving rejects to file src/test/regress/expected/sanity_check.out.rej What this patch does is basically: - add pg_extension_feature to store "feature" (name) provided by extensions - extension control file now has "provide" parameter to indicate "feature", which is comma separated - when creating an extension, the backend looks for "feature" required in control file - the installed extension has dependency on "feature" So, the first thing is catalog. db1=# \d pg_extension_feature; Table "pg_catalog.pg_extension_feature" Column | Type | Modifiers ------------+------+-----------extoid | oid | not nullextfeature | name | not null Indexes: "pg_extension_feature_index" UNIQUE, btree (extoid, extfeature) "pg_extension_feature_oid_index" UNIQUE, btree(oid) * I'm not quit sure why pg_extension_feature_index needs extoid column. * I have a big question to add two-column catalog. I don't mind the actual number of columns, but if the table has only two columns, it implies the design may be bad. Only two column catalog other than this is pg_largeobject_metadata. Next, some questions: - Why is the finer dependency needed? Do you have tangible example that struggles with the dependency granularity? I feel so good about the existing dependency on extension as an extension developer of several ones. - What happens if DROP EXTENSION ... CASCADE? Does it work? - How does pg_upgrade interact with this? The dependency changes. A minor memo. list_extension_features(): I guess the size argument for repalloc is bogus. So, that's pretty much I've reviewed quickly. I'll need more time to look in detail, but I'd like more inputs for the high-level design and direction. Thanks, -- Hitoshi Harada
Hi, Thank you for reviewing this patch! Hitoshi Harada <umi.tanuki@gmail.com> writes: > The patch applies with one reject, which I could fix easily. The make > check passed. Bitrot happens fast in this season… will produce another version of the patch. > Table "pg_catalog.pg_extension_feature" > Column | Type | Modifiers > ------------+------+----------- > extoid | oid | not null > extfeature | name | not null > Indexes: > "pg_extension_feature_index" UNIQUE, btree (extoid, extfeature) > "pg_extension_feature_oid_index" UNIQUE, btree (oid) > > * I'm not quit sure why pg_extension_feature_index needs extoid column. That allows managing features per extension: you need to know which extension is providing which feature to be able to solve dependencies. > * I have a big question to add two-column catalog. I don't mind the > actual number of columns, but if the table has only two columns, it > implies the design may be bad. Only two column catalog other than this > is pg_largeobject_metadata. We need each feature to be a full PostgreSQL object so that we can use the dependency tracking. That allows to manage DROP EXTENSION foo and cascade to extensions that depend on feature(s) provided by foo. It also allows to track at update time when the feature set changes if we're removing a feature that some other installed extension is depending on, allowing our users to know that they should either drop this other extension or update it too. > Next, some questions: > - Why is the finer dependency needed? Do you have tangible example > that struggles with the dependency granularity? I feel so good about > the existing dependency on extension as an extension developer of > several ones. The problem is not yet very apparent only because extensions are very new. The main thing we address with this patch is depending on a feature that appeared while developing an extension or that gets removed down the line. It allows to depend on features and avoid needing to compare version numbers and maintain a list of which version number is providing which feature. This feature has been asked by several extension users, beginning even before 9.1 got released. > - What happens if DROP EXTENSION ... CASCADE? Does it work? It should, what happens when you try? :) > - How does pg_upgrade interact with this? The dependency changes. We're compatible because the extension name itself is always considered as a feature and added to the catalogs, so that e.g. require = 'cube' will now target a feature rather than an extension, and this feature defaults to being provided by the 'create extension cube' statement that pg_upgrade issues given 9.1 dependency tracking. > A minor memo. > list_extension_features(): I guess the size argument for repalloc is > bogus. Oh, a quick review of repalloc call points seems to indicate you're right, I'll fix that in the next version of the patch. > So, that's pretty much I've reviewed quickly. I'll need more time to > look in detail, but I'd like more inputs for the high-level design and > direction. I hope to be answering to your questions here, please ask for more details as you need them! Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On 21 Leden 2012, 18:20, Dimitri Fontaine wrote: > Hi, > > Thank you for reviewing this patch! > > Hitoshi Harada <umi.tanuki@gmail.com> writes: >> >> Next, some questions: >> - Why is the finer dependency needed? Do you have tangible example >> that struggles with the dependency granularity? I feel so good about >> the existing dependency on extension as an extension developer of >> several ones. > > The problem is not yet very apparent only because extensions are very > new. The main thing we address with this patch is depending on a feature > that appeared while developing an extension or that gets removed down > the line. It allows to depend on features and avoid needing to compare > version numbers and maintain a list of which version number is providing > which feature. > > This feature has been asked by several extension users, beginning even > before 9.1 got released. It's also about several extension providing the same sort of functionality implemented in different ways. Think about extensions that allow you to send e-mails right from the database. One could do that directly, the other one could implement queuing, another one could be optimized for a specific SMTP server. With features, you could just say 'require = mail' and use the extension that's installed. Yes, this needs a common API. I personally see this as a major step towards fully-fledged package management, similar to those available in modern Linux distributions (apt, yum, portage, ...). Tomas
On Sat, Jan 21, 2012 at 9:20 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Hi, > > Thank you for reviewing this patch! > > Hitoshi Harada <umi.tanuki@gmail.com> writes: >> The patch applies with one reject, which I could fix easily. The make >> check passed. > > Bitrot happens fast in this season… will produce another version of the > patch. > >> Table "pg_catalog.pg_extension_feature" >> Column | Type | Modifiers >> ------------+------+----------- >> extoid | oid | not null >> extfeature | name | not null >> Indexes: >> "pg_extension_feature_index" UNIQUE, btree (extoid, extfeature) >> "pg_extension_feature_oid_index" UNIQUE, btree (oid) >> >> * I'm not quit sure why pg_extension_feature_index needs extoid column. > > That allows managing features per extension: you need to know which > extension is providing which feature to be able to solve dependencies. Do you mean you want UNIQUE constraint by this index? I found the usage is to search feature by (only) its name, so I wondered if extoid is not necessary. >> * I have a big question to add two-column catalog. I don't mind the >> actual number of columns, but if the table has only two columns, it >> implies the design may be bad. Only two column catalog other than this >> is pg_largeobject_metadata. > > We need each feature to be a full PostgreSQL object so that we can use > the dependency tracking. That allows to manage DROP EXTENSION foo and > cascade to extensions that depend on feature(s) provided by foo. I guess if we spend more time, we'll figure out what is "feature" actually, and then will see what kind of columns/attributes are needed to represent it. Although I agree we can add them later, again, this may imply the design is premature. (it's ok if i am the only person who thinks so) >> Next, some questions: >> - Why is the finer dependency needed? Do you have tangible example >> that struggles with the dependency granularity? I feel so good about >> the existing dependency on extension as an extension developer of >> several ones. > > The problem is not yet very apparent only because extensions are very > new. The main thing we address with this patch is depending on a feature > that appeared while developing an extension or that gets removed down > the line. It allows to depend on features and avoid needing to compare > version numbers and maintain a list of which version number is providing > which feature. > > This feature has been asked by several extension users, beginning even > before 9.1 got released. > >> - What happens if DROP EXTENSION ... CASCADE? Does it work? > > It should, what happens when you try? :) I just tried DROP EXTENSION now, and found it broken :( db1=# create extension kmeans; CREATE EXTENSION db1=# drop extension kmeans; ERROR: cannot drop extension kmeans because extension feature kmeans requires it HINT: You can drop extension feature kmeans instead. db1=# drop extension kmeans cascade; ERROR: cannot drop extension kmeans because extension feature kmeans requires it HINT: You can drop extension feature kmeans instead. Am I missing something? I'm confused why this happens. Thanks, -- Hitoshi Harada
Hitoshi Harada <umi.tanuki@gmail.com> writes: >>> "pg_extension_feature_index" UNIQUE, btree (extoid, extfeature) > Do you mean you want UNIQUE constraint by this index? I found the > usage is to search feature by (only) its name, so I wondered if extoid > is not necessary. I guess you're right and that's something I've just left when I should have cleaned it. We need to find which extension is providing which feature, and we need feature names to be globally unique. I'll remove extoid from this index in the next revision on the patch. I'm not in a position to provide that next revision just now, that would happen before the end of the week though. > I guess if we spend more time, we'll figure out what is "feature" > actually, and then will see what kind of columns/attributes are needed > to represent it. Although I agree we can add them later, again, this > may imply the design is premature. (it's ok if i am the only person > who thinks so) You might be right that a feature is more than just a unique name but as things are, that's their only useful property. >>> - What happens if DROP EXTENSION ... CASCADE? Does it work? >> >> It should, what happens when you try? :) > > I just tried DROP EXTENSION now, and found it broken :( > > db1=# create extension kmeans; > CREATE EXTENSION > db1=# drop extension kmeans; > ERROR: cannot drop extension kmeans because extension feature kmeans > requires it > HINT: You can drop extension feature kmeans instead. Can you provide me the test case you've been using? That looks like a bug I need to fix, indeed (unless the problem lies in the test case, which would mean I need to tighten things some more). Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Mon, Jan 23, 2012 at 2:00 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Hitoshi Harada <umi.tanuki@gmail.com> writes: >>>> - What happens if DROP EXTENSION ... CASCADE? Does it work? >>> >>> It should, what happens when you try? :) >> >> I just tried DROP EXTENSION now, and found it broken :( >> >> db1=# create extension kmeans; >> CREATE EXTENSION >> db1=# drop extension kmeans; >> ERROR: cannot drop extension kmeans because extension feature kmeans >> requires it >> HINT: You can drop extension feature kmeans instead. > > Can you provide me the test case you've been using? That looks like a > bug I need to fix, indeed (unless the problem lies in the test case, > which would mean I need to tighten things some more). The test case is just above; createdb db1 and create and drop an extension. The kmean extension is on pgxn. I tried my small test extension named ext1 which contains only one plpgsql function, and created it then dropped it, reproduced. Thanks, -- Hitoshi Harada
On Mon, Jan 23, 2012 at 3:06 AM, Hitoshi Harada <umi.tanuki@gmail.com> wrote: > On Mon, Jan 23, 2012 at 2:00 AM, Dimitri Fontaine > <dimitri@2ndquadrant.fr> wrote: >> Hitoshi Harada <umi.tanuki@gmail.com> writes: >>>>> - What happens if DROP EXTENSION ... CASCADE? Does it work? >>>> >>>> It should, what happens when you try? :) >>> >>> I just tried DROP EXTENSION now, and found it broken :( >>> >>> db1=# create extension kmeans; >>> CREATE EXTENSION >>> db1=# drop extension kmeans; >>> ERROR: cannot drop extension kmeans because extension feature kmeans >>> requires it >>> HINT: You can drop extension feature kmeans instead. >> >> Can you provide me the test case you've been using? That looks like a >> bug I need to fix, indeed (unless the problem lies in the test case, >> which would mean I need to tighten things some more). > > The test case is just above; createdb db1 and create and drop an > extension. The kmean extension is on pgxn. I tried my small test > extension named ext1 which contains only one plpgsql function, and > created it then dropped it, reproduced. > Ping. In case you don't have updates soon, I'll mark Returned with Feedback. Thanks, -- Hitoshi Harada
Hitoshi Harada <umi.tanuki@gmail.com> writes: > Ping. In case you don't have updates soon, I'll mark Returned with Feedback. Pong. Sorry about my recent silence. I've not been in a position to work on this recently, and am done with those other duties now. I intend to be posting an updated patch soon now. Please wait some more before punting this patch out of the current commit fest. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Hi, Sorry for the delays, I'm back on PostgreSQL related work again. Hitoshi Harada <umi.tanuki@gmail.com> writes: >>> I just tried DROP EXTENSION now, and found it broken :( Please find v2 of the patch. I did change the dependency management in between the simple cases and the more challenging ones and forgot that I had to retest it all in between, which is what happen on a tight schedule and when working at night, I guess. So the best option I've found here had me add a new function in pg_depend.c, it's working as intended now. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachment
On Mon, Feb 13, 2012 at 3:18 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Hi, > > Sorry for the delays, I'm back on PostgreSQL related work again. > > Hitoshi Harada <umi.tanuki@gmail.com> writes: >>>> I just tried DROP EXTENSION now, and found it broken :( > > Please find v2 of the patch. I did change the dependency management in > between the simple cases and the more challenging ones and forgot that I > had to retest it all in between, which is what happen on a tight > schedule and when working at night, I guess. > The patch is partially rejected due to the pg_proc column changes from leakproof, but I could apply manually. I confirmed DROP EXTENSION is fixed now. In turn, it seems to me "requires" doesn't work. My test ext2.control looks like: comment = 'sample1' default_version = '1.0' requires = 'featZ' relocatable = true And simply this extension can be installed against cleanly-initialized database. I double-checked there's no entry for featz in pg_extension_feature. Also, I found that if control file has duplicate names in "provides", the error is not friendly ("duplicate entry for pg_extension_feature", or something). This is same if "provides" has the extension name itself. I'll have a look more but give comments so far so that you can find solutions to them soon. Thanks, -- Hitoshi Harada
Hitoshi Harada <umi.tanuki@gmail.com> writes: > I confirmed DROP EXTENSION is fixed now. In turn, it seems to me > "requires" doesn't work. My test ext2.control looks like: I'm very sorry about that. It's all about playing with pg_depend and I've failed to spend enough time on that very topic to send a patch that just works, it seems. I'm going to fix that over the week-end. Thanks for your reviewing so far. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Fri, Feb 24, 2012 at 2:09 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Hitoshi Harada <umi.tanuki@gmail.com> writes: >> I confirmed DROP EXTENSION is fixed now. In turn, it seems to me >> "requires" doesn't work. My test ext2.control looks like: > > I'm very sorry about that. It's all about playing with pg_depend and > I've failed to spend enough time on that very topic to send a patch that > just works, it seems. > > I'm going to fix that over the week-end. Thanks for your reviewing so > far. Quickly reviewed the patch and found some issues. - There are some mixture of pg_extension_feature and pg_extension_feature"s" - The doc says pg_extension_feature"s" has four columns but it's not true. - Line 608 is bad. In the loop, provides_itself is repeatedly changed to true and false and I guess that's not what you meant. - Line 854+, you can fold two blocks into one. The two blocks are similar and by giving provides list with list_make1 when control->provides == NIL you can do it in one block. - s/trak/track/ - Line 960, you missed updating classId for dependency. That's pretty much from me. I just looked at the patch and have no idea about grand architecture. Marking Waiting on Author. Thanks, -- Hitoshi Harada
On Tue, Feb 28, 2012 at 5:34 AM, Hitoshi Harada <umi.tanuki@gmail.com> wrote: > Quickly reviewed the patch and found some issues. > > - There are some mixture of pg_extension_feature and pg_extension_feature"s" > - The doc says pg_extension_feature"s" has four columns but it's not true. > - Line 608 is bad. In the loop, provides_itself is repeatedly changed > to true and false and I guess that's not what you meant. > - Line 854+, you can fold two blocks into one. The two blocks are > similar and by giving provides list with list_make1 when > control->provides == NIL you can do it in one block. > - s/trak/track/ > - Line 960, you missed updating classId for dependency. > > That's pretty much from me. I just looked at the patch and have no > idea about grand architecture. Marking Waiting on Author. Dimitri, are you going to post an updated patch for this CF? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > Dimitri, are you going to post an updated patch for this CF? Yes, I intend to do that. Not sure about diverting from the command trigger patch while Thom is full speed on reviewing and helping me write the full covering test cases, though. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Thu, Mar 8, 2012 at 9:39 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> Dimitri, are you going to post an updated patch for this CF? > > Yes, I intend to do that. Not sure about diverting from the command > trigger patch while Thom is full speed on reviewing and helping me write > the full covering test cases, though. I don't think we can wait any longer for this; we're now more than two months in to this CommitFest, and command triggers is still in full swing. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > I don't think we can wait any longer for this; we're now more than two > months in to this CommitFest, and command triggers is still in full > swing. Is it possible to have another day to send out a revised patch? The problem reported is either a show stopper or a less-than-one-hour fix, I would hate to miss 9.2 for having been swamped so much as to miss the time to qualify the problem. Baring objections, I'll send a new revision later tonight or tomorrow, or a notification that the patch is really dead for 9.2. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Wed, Mar 21, 2012 at 11:11 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I don't think we can wait any longer for this; we're now more than two >> months in to this CommitFest, and command triggers is still in full >> swing. > > Is it possible to have another day to send out a revised patch? The > problem reported is either a show stopper or a less-than-one-hour fix, I > would hate to miss 9.2 for having been swamped so much as to miss the > time to qualify the problem. > > Baring objections, I'll send a new revision later tonight or tomorrow, > or a notification that the patch is really dead for 9.2. Sounds reasonable to me. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, Again, thanks very much for the review. Here's an updated patch (just merged against master) fixing most of your comments here. I couldn't reproduce previous problems with the attached: - DROP EXTENSION was broken, asking to cascade to self - CREATE EXTENSION was bypassing "requires" I could reproduce the second problem then fix it with the following one liner. I missed it because my test case still fails for not finding the cube type rather than the cube extension without this fix: - if (!OidIsValid(featoid) && !missing_ok) + if (!OidIsValid(*featoid) && !missing_ok) Thank you all for your patience while I was busy elsewhere, it's definitely not a show stopper in my book :) dim=# create extension earthdistance; ERROR: feature "cube" is not currently provided HINT: Please install an extension that provides it first dim=# create extension cube; CREATE EXTENSION dim=# create extension earthdistance; CREATE EXTENSION dim=# drop extension cube cascade; NOTICE: drop cascades to extension earthdistance DROP EXTENSION Hitoshi Harada <umi.tanuki@gmail.com> writes: > - There are some mixture of pg_extension_feature and pg_extension_feature"s" Fixed. > - The doc says pg_extension_feature"s" has four columns but it's not true. Well the SGML table describing the catalog has 4 cols :) > - Line 608 is bad. In the loop, provides_itself is repeatedly changed > to true and false and I guess that's not what you meant. Fixed. > - Line 854+, you can fold two blocks into one. The two blocks are > similar and by giving provides list with list_make1 when > control->provides == NIL you can do it in one block. Fixed. > - s/trak/track/ Fixed, I guess the English would need rephrasing. > - Line 960, you missed updating classId for dependency. I don't think so. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachment
Excerpts from Dimitri Fontaine's message of jue mar 22 14:38:29 -0300 2012: > Hi, > > Again, thanks very much for the review. Here's an updated patch (just > merged against master) fixing most of your comments here. I couldn't > reproduce previous problems with the attached: get_available_versions_for_extension seems to contain a bunch of commented-out lines ... -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes: > get_available_versions_for_extension seems to contain a bunch of > commented-out lines ... Damn. Sorry about that. Here's a cleaned-up version of the patch. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachment
Excerpts from Dimitri Fontaine's message of jue mar 22 15:08:27 -0300 2012: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > get_available_versions_for_extension seems to contain a bunch of > > commented-out lines ... > > Damn. Sorry about that. Here's a cleaned-up version of the patch. Hmm .. feature names should be globally unique, right? If so I think you're missing an UNIQUE index on the new catalog, covering just the feature name. You have a two column index (extoid, featurename), so you could have two different extensions providing the same feature. Is this okay? If it is, then there is a bit of a bogus code in get_extension_feature_oids because it says it assumes that "there is only one row". Now maybe you just want to return the first one found and that's okay, but in that case the comment is bogus. I noticed that you've left unspecified whether an extension should have a "provides" entry for itself or not -- I mean the code adds one if it's not there. I'm not sure about this, maybe it's okay. But this makes it impossible for it to say "provides: extname-0.5" and have a dependent extension fail if it only requires "extname", because that one will be provided automatically whether extname's author wants it or not. Again, maybe this is okay. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes: > Hmm .. feature names should be globally unique, right? If so I think > you're missing an UNIQUE index on the new catalog, covering just the > feature name. You have a two column index (extoid, featurename), so you > could have two different extensions providing the same feature. Is this > okay? If it is, then there is a bit of a bogus code in You're right, this looks like something I forgot to go back to, the unique index should be global on the feature's name. I will go make that happen (tomorrow). > I noticed that you've left unspecified whether an extension should have > a "provides" entry for itself or not -- I mean the code adds one if it's > not there. I'm not sure about this, maybe it's okay. But this makes it > impossible for it to say "provides: extname-0.5" and have a dependent > extension fail if it only requires "extname", because that one will be > provided automatically whether extname's author wants it or not. > Again, maybe this is okay. I think it is ok, at least that's how I intended the feature to work. The use case I want to allow is for the other extension's author to say its extension depends on the extname-0.5 feature. In fact I guess that you would rather provide feature names, not version, so as not to have to look up a feature matrix each time. For the use case you're concerned with, I think that if an extension's upgrade is not compatible with the previous version, the extension name itself should be changed (extname2 or extname-1.0 or whatever). Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes: > Alvaro Herrera <alvherre@commandprompt.com> writes: >> Hmm .. feature names should be globally unique, right? If so I think >> you're missing an UNIQUE index on the new catalog, covering just the >> feature name. You have a two column index (extoid, featurename), so you >> could have two different extensions providing the same feature. Please find v5 of the patch attached, where =# \d pg_extension_feature Table "pg_catalog.pg_extension_feature" Column | Type | Modifiers ------------+------+----------- extoid | oid | not null extfeature | name | not null Indexes: "pg_extension_feature_name_index" UNIQUE, btree (extfeature) "pg_extension_feature_oid_index" UNIQUE, btree (oid) "pg_extension_feature_extoid_name_index" btree (extoid, extfeature) We could maybe get rid of the (extoid, extfeature) index which is only used to get sorted output in list_extension_features() function, but I don't know how to do an ORDER BY scan without index in C (yet). The ordering is then used to maintain pg_depend when the list of provided features changes at upgrade time. We fetch the ordered list of “old” feature names then for each newly provided feature name we bsearch() the old list, which then needs to be properly ordered. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachment
Excerpts from Dimitri Fontaine's message of vie mar 23 11:05:37 -0300 2012: > =# \d pg_extension_feature > Table "pg_catalog.pg_extension_feature" > Column | Type | Modifiers > ------------+------+----------- > extoid | oid | not null > extfeature | name | not null > Indexes: > "pg_extension_feature_name_index" UNIQUE, btree (extfeature) > "pg_extension_feature_oid_index" UNIQUE, btree (oid) > "pg_extension_feature_extoid_name_index" btree (extoid, extfeature) > > We could maybe get rid of the (extoid, extfeature) index which is only > used to get sorted output in list_extension_features() function, but I > don't know how to do an ORDER BY scan without index in C (yet). > > The ordering is then used to maintain pg_depend when the list of > provided features changes at upgrade time. We fetch the ordered list of > “old” feature names then for each newly provided feature name we > bsearch() the old list, which then needs to be properly ordered. Hm, couldn't it be done simply with a qsort()? Presumably there aren't many feature entries to sort ... -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes: > Excerpts from Dimitri Fontaine's message of vie mar 23 11:05:37 -0300 2012: > >> =# \d pg_extension_feature >> Table "pg_catalog.pg_extension_feature" >> Column | Type | Modifiers >> ------------+------+----------- >> extoid | oid | not null >> extfeature | name | not null >> Indexes: >> "pg_extension_feature_name_index" UNIQUE, btree (extfeature) >> "pg_extension_feature_oid_index" UNIQUE, btree (oid) >> "pg_extension_feature_extoid_name_index" btree (extoid, extfeature) >> >> We could maybe get rid of the (extoid, extfeature) index which is only >> used to get sorted output in list_extension_features() function, but I >> don't know how to do an ORDER BY scan without index in C (yet). >> >> The ordering is then used to maintain pg_depend when the list of >> provided features changes at upgrade time. We fetch the ordered list of >> “old” feature names then for each newly provided feature name we >> bsearch() the old list, which then needs to be properly ordered. > > Hm, couldn't it be done simply with a qsort()? Presumably there aren't > many feature entries to sort ... Mmmm… Then we would need an index on extoid to be able to list features of a given extension, and that would be the only usage of such an index. I guess that having it include the feature's name is not so expensive as to try avoiding it and qsort() in the code rather than scan the index in order? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Excerpts from Dimitri Fontaine's message of vie mar 23 12:26:47 -0300 2012: > > Alvaro Herrera <alvherre@commandprompt.com> writes: > > Excerpts from Dimitri Fontaine's message of vie mar 23 11:05:37 -0300 2012: > > > >> =# \d pg_extension_feature > >> Table "pg_catalog.pg_extension_feature" > >> Column | Type | Modifiers > >> ------------+------+----------- > >> extoid | oid | not null > >> extfeature | name | not null > >> Indexes: > >> "pg_extension_feature_name_index" UNIQUE, btree (extfeature) > >> "pg_extension_feature_oid_index" UNIQUE, btree (oid) > >> "pg_extension_feature_extoid_name_index" btree (extoid, extfeature) > >> > >> We could maybe get rid of the (extoid, extfeature) index which is only > >> used to get sorted output in list_extension_features() function, but I > >> don't know how to do an ORDER BY scan without index in C (yet). > >> > >> The ordering is then used to maintain pg_depend when the list of > >> provided features changes at upgrade time. We fetch the ordered list of > >> “old” feature names then for each newly provided feature name we > >> bsearch() the old list, which then needs to be properly ordered. > > > > Hm, couldn't it be done simply with a qsort()? Presumably there aren't > > many feature entries to sort ... > > Mmmm… Then we would need an index on extoid to be able to list features > of a given extension, and that would be the only usage of such an index. > I guess that having it include the feature's name is not so expensive as > to try avoiding it and qsort() in the code rather than scan the index in > order? Well, as far as I can see the only use of pg_extension_feature_extoid_name_index right now is the same as the only use for the extoid index. I mean, what you really want is to find out the features of an extension, right? The extfeature column is just there to provide you with the ordering, which should be easy to determine outside of the index. Why do features have OIDs? Is this for pg_depend entries? If so, would it work to have pg_depend entries point to extensions instead? -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes: > Why do features have OIDs? Is this for pg_depend entries? If so, would > it work to have pg_depend entries point to extensions instead? Yes, for pg_depend, no I don't know how to make that work with pointing to the extensions directly, because the whole point here is to be able to depend on a feature rather than the whole extension. Use cases: - depend on a feature f that appeared in version y of the extension (bugfix, new capability) - deprecate a feature: alter extension update removes a feature, you want to know that the dependent extensions need processing(cascade to remove them in the operation, or update them before hand, etc) (still manual operation though) I don't see how to handle those cases with a direct dependency on the extension rather than one of its features. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Excerpts from Dimitri Fontaine's message of vie mar 23 13:12:22 -0300 2012: > > Alvaro Herrera <alvherre@commandprompt.com> writes: > > Why do features have OIDs? Is this for pg_depend entries? If so, would > > it work to have pg_depend entries point to extensions instead? > > Yes, for pg_depend, no I don't know how to make that work with pointing > to the extensions directly, because the whole point here is to be able > to depend on a feature rather than the whole extension. Yes, I understand that -- but would it work to have the feature resolution be done at install/upgrade time, and once it's resolved, you record it by storing the extension than contains the feature? That way it correctly breaks when the extension gets removed; and since we ensure that upgrading an extension means delete its features and then insert them anew, it would also correctly break at that point if some feature is no longer provided. I'm not wedded to this idea, so if we think it doesn't work for some reason, I have no problem going back to the idea of having direct dependencies to features instead. But I think it's worth considering. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes: >> Yes, for pg_depend, no I don't know how to make that work with pointing >> to the extensions directly, because the whole point here is to be able >> to depend on a feature rather than the whole extension. > > Yes, I understand that -- but would it work to have the feature > resolution be done at install/upgrade time, and once it's resolved, you > record it by storing the extension than contains the feature? That way I don't think so, because at upgrade time you then typically only have the new .control file with the new set of features, and you need to act on the difference between the old and new features compared to the current other packages dependencies towards them. For that to work you need to remember the exact set of per feature dependencies in between extensions. You can't trust the control files to reflect the reality you saw when installing or last updating. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Excerpts from Dimitri Fontaine's message of vie mar 23 16:51:57 -0300 2012: > > Alvaro Herrera <alvherre@commandprompt.com> writes: > >> Yes, for pg_depend, no I don't know how to make that work with pointing > >> to the extensions directly, because the whole point here is to be able > >> to depend on a feature rather than the whole extension. > > > > Yes, I understand that -- but would it work to have the feature > > resolution be done at install/upgrade time, and once it's resolved, you > > record it by storing the extension than contains the feature? That way > > I don't think so, because at upgrade time you then typically only have > the new .control file with the new set of features, and you need to > act on the difference between the old and new features compared to the > current other packages dependencies towards them. Aha, right. So you still need an index on (oid), one on (extoid), and one on (extfeature). -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes: > So you still need an index on (oid), one on (extoid), and one on > (extfeature). Yes. And the main use case for the index on (extoid) is listing a given extension's features, that we want to order by their name, then the set of indexes I've been defining is now: Indexes: "pg_extension_feature_name_index" UNIQUE, btree (extfeature) "pg_extension_feature_oid_index" UNIQUE, btree(oid) "pg_extension_feature_extoid_name_index" btree (extoid, extfeature) Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Thu, Mar 22, 2012 at 2:08 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: >> get_available_versions_for_extension seems to contain a bunch of >> commented-out lines ... > > Damn. Sorry about that. Here's a cleaned-up version of the patch. I'm not completely convinced that the case has been made that this is a useful thing to have. It is true that systems like RPM have (and need) something like this, but that doesn't seem like a sufficient argument. The difference is that any set of RPMs which is designed to be used as a set has been packaged by a single project which (hopefully) has a consistent set of policies for how things are tagged and labeled and has done some integration testing to makes sure that every package which provides the wumpus feature actually is sufficient for the needs of all packages that depend on wumpus. On the other hand, PostgreSQL extensions are a free-for-all. Anybody can publish an extension on PGXN (or elsewhere), and they can stuff whatever junk they want in their control file - or more likely, fail to stuff anything in there at all, so that a package author who might want to depend on feature X may well find that not all the packages that provide feature X are tagged as providing it. If that turns out to be the case, are they going to (a) contact all of those package authors and convince them to update their control files or (b) punt? I'll bet on (b). Even if we suppose that the above is not a problem, it seems to me that there's a further difficulty. What exactly does it mean to provide a feature like "smtp"? Presumably, it means that you have a way to send mail. Fine. But, what exactly is that way? It presumably involves calling a function. It is very likely that if there are multiple mail-sending extension packages for PostgreSQL, they don't all create a function with exactly the same name and signature. Even if they did, the extension that is depending on this functionality has no way of knowing what schema that function is going to be in, unless all of those extensions are not-relocatable, which I think extension authors will be reluctant to do for entirely valid reasons. Now, the extension author can hack around all this by writing code (in PL/pgsql, for example) to search through the system catalogs and figure out which extension is providing the smtp feature and in what schema it's located; and then, having done that, they can even deduce the function name and signature, build a dynamically generated SQL query, and send mail. Woohoo! In practice, however, that sounds like a real pain in the neck. I would expect most people who were packaging extensions to handle a situation like this by forcing the user to provide the name of the function to be called, either via a control table or via a GUC. And once you've done that, it makes no sense to shove a feature dependency into the extension, because the user might very well just write an appropriate function themselves and tell the extension to call that. Theory aside, I am, like Alvaro, a bit skeptical of making extension features their own first-class objects. I think that part of the point of this mechanism in other package management systems is to allow a user to execute an RPM (say) transaction that drops an extension which provides feature X but makes up for it by installing, in the same transaction, a new extension that provides the same feature X. I suspect that this won't work with the design you have right now. In fact, I suspect it also won't work to install the new extension first and then drop the old one; I might be wrong, but I don't think our dependency mechanism has any support for depending on either A or B, which is really what is needed here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > I'm not completely convinced that the case has been made that this is > a useful thing to have. You're basically saying that the current lack of extension distribution is a good reason for not building the tools allowing to create said distribution. WTF? (PGXN uses the word distribution with yet another meaning, not to be mistaken for the kind of work done by debian or fedorateams) > Even if we suppose that the above is not a problem, it seems to me > that there's a further difficulty. What exactly does it mean to > provide a feature like "smtp"? Presumably, it means that you have a Now you're saying that you want another feature built on top of this one, which is the ability to normalize features so that each provider is following up on the same standard as soon as they happen to provide the same feature. Then you say that having a features mechanism without the whole policy and standardisation and normalisation is not going to fly. WTF? > In practice, however, that sounds like a real pain in the neck. I > would expect most people who were packaging extensions to handle a > situation like this by forcing the user to provide the name of the > function to be called, either via a control table or via a GUC. And > once you've done that, it makes no sense to shove a feature dependency > into the extension, because the user might very well just write an > appropriate function themselves and tell the extension to call that. I don't know what you're talking about here, all I can say is that is has nothing to do with what the patch is implementing. What's in the patch is a way to depend on known versions of an extension rather than the extension wholesale, whatever the version. Using feature dependency allow to avoid 2 particularly messy things: - imposing a version numbering scheme with a comparator- maintaining a separate feature matrix So instead of having to say "foo version 1.2 is now doing buzz" and having an extension depend on foo >= 1.2, you can say that your extension depends on the buzz feature. That's about it. > Theory aside, I am, like Alvaro, a bit skeptical of making extension > features their own first-class objects. I think that part of the > point of this mechanism in other package management systems is to > allow a user to execute an RPM (say) transaction that drops an > extension which provides feature X but makes up for it by installing, > in the same transaction, a new extension that provides the same > feature X. I suspect that this won't work with the design you have As far as I know, RPM and deb and other popular packaging systems are imposing mutually incompatible version number policies and tools to compare them, and then packagers have to manually care about translating a feature matrix into version number CHECK clauses. The require/provide facility is something more spread into Emacs Lisp and Common Lisp, if you really need to see it in action someplace else. But again, really, it's all about being able to have an extension depend on « the new hstore » or « the new pg_stat_statement ». That's it. About having a new catalog to host extension features, the problems I'm trying to solve with that are about extension upgrade. You can add new features at upgrade, you can also drop some. I don't know how to tell the user that extension X has to be removed to upgrade foo to 1.3 where it's not providing feature "buzz" anymore, without pg_extension_feature. > right now. In fact, I suspect it also won't work to install the new > extension first and then drop the old one; I might be wrong, but I > don't think our dependency mechanism has any support for depending on > either A or B, which is really what is needed here. We have ALTER EXTENSION foo UPDATE, you know. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Wed, Mar 28, 2012 at 11:28 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: >> In practice, however, that sounds like a real pain in the neck. I >> would expect most people who were packaging extensions to handle a >> situation like this by forcing the user to provide the name of the >> function to be called, either via a control table or via a GUC. And >> once you've done that, it makes no sense to shove a feature dependency >> into the extension, because the user might very well just write an >> appropriate function themselves and tell the extension to call that. > > I don't know what you're talking about here, all I can say is that is > has nothing to do with what the patch is implementing. > > What's in the patch is a way to depend on known versions of an extension > rather than the extension wholesale, whatever the version. Using feature > dependency allow to avoid 2 particularly messy things: > > - imposing a version numbering scheme with a comparator > - maintaining a separate feature matrix > > So instead of having to say "foo version 1.2 is now doing buzz" > and having an extension depend on foo >= 1.2, you can say that your > extension depends on the buzz feature. That's about it. Based on this information, it seems that I've misinterpreted the purpose of the patch. Since extension features seem to live in a global namespace, I assumed that the purpose of the patch was to allow both extension A and extension B to provide feature F, and extension C to depend on F rather than A or B specifically. What I understand you to be saying here is that's not really what you're trying to accomplish. Instead, you're just concerned about allowing some but not all versions of package A to provide feature F, so that other extensions can depend on F to get the specific version of A that they need (and not, as I had assumed, so that they can get either A or B). Let me think more about that. Maybe I'm just easily confused here, or maybe there is something that should be changed in the code or docs; I'm not sure yet. On a more prosaic note, you seem to have made a mistake when generating the v5 diff. It includes reverts of a couple of unrelated, recent patches. > WTF? WTF? On a further note, I have spent a heck of a lot more time reviewing other people's patches this CommitFest than you have, and I don't appreciate this. If you'd rather that I didn't spend time on this patch, I have plenty of other things to do with my time. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > accomplish. Instead, you're just concerned about allowing some but > not all versions of package A to provide feature F, so that other > extensions can depend on F to get the specific version of A that they > need (and not, as I had assumed, so that they can get either A or B). Exactly. > Let me think more about that. Maybe I'm just easily confused here, or > maybe there is something that should be changed in the code or docs; > I'm not sure yet. Agreed, there's certainly something to expand on here. > On a more prosaic note, you seem to have made a mistake when > generating the v5 diff. It includes reverts of a couple of unrelated, > recent patches. Ouch. It seems to happen to me too often. I probably need to get the shallow clones setup where you have different directories hosting each a different git branch so that you don't need to checkout just to update your local master, etc. >> WTF? WTF? > > On a further note, I have spent a heck of a lot more time reviewing > other people's patches this CommitFest than you have, and I don't > appreciate this. If you'd rather that I didn't spend time on this > patch, I have plenty of other things to do with my time. Sorry about that. I'm on a crazy schedule and too tired, and I wanted to be sure to attract your attention on a misunderstanding here. It's also not clear to me what level of language WTF really is, or “dude” to take another example. I'll be sure not to use that again when I aim at being polite yet dense. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Wed, Mar 28, 2012 at 12:11 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: >> On a more prosaic note, you seem to have made a mistake when >> generating the v5 diff. It includes reverts of a couple of unrelated, >> recent patches. > > Ouch. It seems to happen to me too often. I probably need to get the > shallow clones setup where you have different directories hosting each a > different git branch so that you don't need to checkout just to update > your local master, etc. Could you possibly generate a new diff to save me the trouble of fixing the one you sent before? >>> WTF? WTF? >> >> On a further note, I have spent a heck of a lot more time reviewing >> other people's patches this CommitFest than you have, and I don't >> appreciate this. If you'd rather that I didn't spend time on this >> patch, I have plenty of other things to do with my time. > > Sorry about that. I'm on a crazy schedule and too tired, and I wanted to > be sure to attract your attention on a misunderstanding here. It's also > not clear to me what level of language WTF really is, or “dude” to take > another example. I'll be sure not to use that again when I aim at being > polite yet dense. I don't seriously feel that "WTF" is unacceptable language for a public mailing list populated mostly by hackers; I try not to use it myself, but I'm not puritanical enough to get annoyed with other people for doing so. I was reacting to the overall tone rather than that specific expression. And I can sympathize with the crazy schedule and too tired thing; I have been there. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.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). > I was reacting to the overall tone rather than > that specific expression. And I can sympathize with the crazy > schedule and too tired thing; I have been there. Yeah. I feel stupid. And tired. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachment
On Wed, Mar 28, 2012 at 3:09 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Robert Haas <robertmhaas@gmail.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
On Wed, Mar 28, 2012 at 8:52 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Mar 28, 2012 at 11:28 AM, Dimitri Fontaine > <dimitri@2ndquadrant.fr> wrote: >>> In practice, however, that sounds like a real pain in the neck. I >>> would expect most people who were packaging extensions to handle a >>> situation like this by forcing the user to provide the name of the >>> function to be called, either via a control table or via a GUC. And >>> once you've done that, it makes no sense to shove a feature dependency >>> into the extension, because the user might very well just write an >>> appropriate function themselves and tell the extension to call that. >> >> I don't know what you're talking about here, all I can say is that is >> has nothing to do with what the patch is implementing. >> >> What's in the patch is a way to depend on known versions of an extension >> rather than the extension wholesale, whatever the version. Using feature >> dependency allow to avoid 2 particularly messy things: >> >> - imposing a version numbering scheme with a comparator >> - maintaining a separate feature matrix >> >> So instead of having to say "foo version 1.2 is now doing buzz" >> and having an extension depend on foo >= 1.2, you can say that your >> extension depends on the buzz feature. That's about it. > > Based on this information, it seems that I've misinterpreted the > purpose of the patch. Since extension features seem to live in a > global namespace, I assumed that the purpose of the patch was to allow > both extension A and extension B to provide feature F, and extension C > to depend on F rather than A or B specifically. What I understand you > to be saying here is that's not really what you're trying to > accomplish. Instead, you're just concerned about allowing some but > not all versions of package A to provide feature F, so that other > extensions can depend on F to get the specific version of A that they > need (and not, as I had assumed, so that they can get either A or B). > > Let me think more about that. Maybe I'm just easily confused here, or > maybe there is something that should be changed in the code or docs; > I'm not sure yet. > > On a more prosaic note, you seem to have made a mistake when > generating the v5 diff. It includes reverts of a couple of unrelated, > recent patches. > >> WTF? WTF? > > On a further note, I have spent a heck of a lot more time reviewing > other people's patches this CommitFest than you have, and I don't > appreciate this. If you'd rather that I didn't spend time on this > patch, I have plenty of other things to do with my time. > Frankly I'm still against this patch. Since I started to review it I've never been convinced with the use case. Yeah, someone said it'd be useful to him, but as a developer of some of PGXN modules I don't see it. I totally agree with Robert's point that one feature is not standardized and nobody can tell how you can depend on the feature in the end. Mind you, I've never heard about building dependency by its name as a string in other packaging system. If you want to introduce the concept of version dependency not feature name dependency, do *it*; I don't think feature dependency solves it. I know you Dimitri is working so hard for this and other patches, but it seems to me that the quality of both of the design and patch code are not adequate at this point of time. I think I agree we are not 100% happy with the current dependency system of extensions, but we need more time to think and make it mature idea rather than rushing and pushing and dropping something premature. The cost we would pay if we rushed this to this release will be higher than what we'd get from it, I think. Thanks, -- Hitoshi Harada
Hitoshi Harada <umi.tanuki@gmail.com> writes: > Frankly I'm still against this patch. Since I started to review it > I've never been convinced with the use case. Yeah, someone said it'd > be useful to him, but as a developer of some of PGXN modules I don't > see it. I totally agree with Robert's point that one feature is not > standardized and nobody can tell how you can depend on the feature in > the end. Mind you, I've never heard about building dependency by its Ok, we might need to find another word for the concept here. Will think, would appreciate native speakers' thought. > name as a string in other packaging system. If you want to introduce > the concept of version dependency not feature name dependency, do > *it*; I don't think feature dependency solves it. I don't want to introduce version dependency, because I don't think we need it. If you want to compare what we're doing here with say debian packaging, then look at how they package libraries. The major version number is now part of the package name and you depend on that directly. So let's take the shortcut to directly depend on the “feature” name. For a PostgreSQL extension example, we could pick ip4r. That will soon include support for ipv6 (it's already done code wise, missing docs update). If you want to use ip4r for storing ipv6, you will simply require “ip6r” or whatever feature name is provided by the extension including it. If you really think this can not be made to work in your use cases, please provide us with an example where it fails. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Thu, Mar 29, 2012 at 12:51 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Hitoshi Harada <umi.tanuki@gmail.com> writes: >> Frankly I'm still against this patch. Since I started to review it >> I've never been convinced with the use case. Yeah, someone said it'd >> be useful to him, but as a developer of some of PGXN modules I don't >> see it. I totally agree with Robert's point that one feature is not >> standardized and nobody can tell how you can depend on the feature in >> the end. Mind you, I've never heard about building dependency by its > > Ok, we might need to find another word for the concept here. Will think, > would appreciate native speakers' thought. > >> name as a string in other packaging system. If you want to introduce >> the concept of version dependency not feature name dependency, do >> *it*; I don't think feature dependency solves it. > > I don't want to introduce version dependency, because I don't think we > need it. If you want to compare what we're doing here with say debian > packaging, then look at how they package libraries. The major version > number is now part of the package name and you depend on that directly. > > So let's take the shortcut to directly depend on the “feature” name. > > For a PostgreSQL extension example, we could pick ip4r. That will soon > include support for ipv6 (it's already done code wise, missing docs > update). If you want to use ip4r for storing ipv6, you will simply > require “ip6r” or whatever feature name is provided by the extension > including it. So my question is why you cannot depend on ip4r in that case. If some version of the module introduces ipv6, then let's depend on that version. It doesn't explain why a string feature name is needed. Thanks, -- Hitoshi Harada
Hitoshi Harada <umi.tanuki@gmail.com> writes: > So my question is why you cannot depend on ip4r in that case. If some > version of the module introduces ipv6, then let's depend on that > version. It doesn't explain why a string feature name is needed. The only operator we have to compare version strings in PostgreSQL extensions is string equality. That's because we don't need more, and designing a version scheme policy would be far more work that any benefit I could imagine. And as we didn't enforce version naming policy in 9.1, it could be argued that it's too late, too. When you say 'require = ip6r' you are depending on *any* version of the extension that is providing it, whatever its version string. You don't have to know that '1.05' < '1.06' < '1.1' and you don't have to know that the first version with ipv6 support was called '1.06'. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Hi, Thanks for your review! Robert Haas <robertmhaas@gmail.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
On Thu, Mar 29, 2012 at 4:37 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Hitoshi Harada <umi.tanuki@gmail.com> writes: >> So my question is why you cannot depend on ip4r in that case. If some >> version of the module introduces ipv6, then let's depend on that >> version. It doesn't explain why a string feature name is needed. > > The only operator we have to compare version strings in PostgreSQL > extensions is string equality. That's because we don't need more, and > designing a version scheme policy would be far more work that any > benefit I could imagine. And as we didn't enforce version naming policy > in 9.1, it could be argued that it's too late, too. > > When you say 'require = ip6r' you are depending on *any* version of the > extension that is providing it, whatever its version string. You don't > have to know that '1.05' < '1.06' < '1.1' and you don't have to know > that the first version with ipv6 support was called '1.06'. If I recall previous discussion correctly, there's fairly broad consensus among people Tom talks to that dependencies on specific versions are inferior to dependencies on features provided by those versions. That having been said, it might be hard for package authors to know what features other packages want to depend on; and perhaps any version of a package might add a new feature that someone might want. So it might be that, after we add this feature, nearly all packagers will do one of two things: 1. Not add a provides line at all, thus making it impossible for anyone else to depend on specific versions; or 2. Add a new feature to the provides line with every release that does anything other than fix bugs, leading to: provides = foobar-1.1, foobar-1.2, foobar-1.3, foobar-1.4, foobar-1.5, foobar-1.6, foobar-2.0, foobar-2.1, foobar-2.2, foobar-2.3, foobar-3.0, foobar-3.1 If that turns out to be the case, we may regret designing the feature this way. Again, the fact that packaging is part of shipping a PostgreSQL extension rather than something that gets tacked on after the fact is not an irrelevant detail. Red Hat can rearrange all the tags they ship at will every time they put out a new version of their distribution, they can enforce uniform standards for how those tags are named, and, maybe most important of all, they don't need to add a tag for every version of the feature that someone *might* want to depend on - only the things that someone *does* want to depend on. Now in spite of all that I'm not sure this is a bad way to go: maybe trying to solve the problem and ending up with something imperfect is better than throwing our hands up in the air and doing nothing. But on the other hand I don't think Harada-san's comments are entirely ill-founded either: how sure are we that this is the right way to go, and what feedback do we have from people who are using this feature that leads us to think this is adequate? Does anyone else have an opinion on this? I think that technically this patch can be polished well enough to commit in the time we have available, but the question of whether it's the right design is harder, and I don't want that to be my call alone. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas wrote: > I think that technically this patch can be polished well enough to > commit in the time we have available, but the question of whether > it's the right design is harder, and I don't want that to be my > call alone. I gather from previous posts that the intent isn't to allow different packages from different authors to provide a common and compatible feature; but what happens in the current design if someone accidentally or maliciously produces an extension which provides the same feature name as another extension? Would we need some registry? -Kevin
On Thu, Mar 29, 2012 at 1:01 PM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > I gather from previous posts that the intent isn't to allow different > packages from different authors to provide a common and compatible > feature; but what happens in the current design if someone > accidentally or maliciously produces an extension which provides the > same feature name as another extension? > > Would we need some registry? A good (documented) convention should make that unnecessary such as: packagename.feature for example provides hstore.populate_record Or something along those lines. Or maybe even prefix it with java like inverse url of author of package. Cheers, Bene
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes: > I gather from previous posts that the intent isn't to allow different > packages from different authors to provide a common and compatible > feature; but what happens in the current design if someone > accidentally or maliciously produces an extension which provides the > same feature name as another extension? It's not about that, it's more like the features/require/provide concepts in Lisp, except that we're not using them to load files. The goal really is to avoid a feature matrix and a version policy with comparators, yet be able to depend on features that got implemented after the first release of an extension. > Would we need some registry? That being said, we still have a single namespace for extensions and their features, so a registry would help, yes. -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Thu, Mar 29, 2012 at 8:01 AM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > Robert Haas wrote: > >> I think that technically this patch can be polished well enough to >> commit in the time we have available, but the question of whether >> it's the right design is harder, and I don't want that to be my >> call alone. > > I gather from previous posts that the intent isn't to allow different > packages from different authors to provide a common and compatible > feature; but what happens in the current design if someone > accidentally or maliciously produces an extension which provides the > same feature name as another extension? > > Would we need some registry? One thing I was thinking about was whether we should restrict feature names to be of some specific form, like extension_name:feature_name. That would address this issue, and would also keep people from thinking of this as an alternatives mechanism, as I did. Of course, that doesn't prevent someone from publishing an ip4r module that erases your hard disk, but there's nothing much we can do about that problem from within core PostgreSQL. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mar 29, 2012, at 4:42 AM, Robert Haas wrote: > 2. Add a new feature to the provides line with every release that does > anything other than fix bugs, leading to: > > provides = foobar-1.1, foobar-1.2, foobar-1.3, foobar-1.4, foobar-1.5, > foobar-1.6, foobar-2.0, foobar-2.1, foobar-2.2, foobar-2.3, > foobar-3.0, foobar-3.1 This is what I have expected to do. In new releases of pgTAP, I’d probably just add version lines. I might give certain releasesnames, but probably not. I’m too lazy, and if a given release has more than one new feature, it’d be a bit silly. I’ve never been very keen on this approach, but then I don’t understand packaging systems very well, so it might rock, andI just don’t know how to use it properly. But I cannot tell. Best, David
On Thu, Mar 29, 2012 at 1:48 PM, David E. Wheeler <david@justatheory.com> wrote: > On Mar 29, 2012, at 4:42 AM, Robert Haas wrote: > >> 2. Add a new feature to the provides line with every release that does >> anything other than fix bugs, leading to: >> >> provides = foobar-1.1, foobar-1.2, foobar-1.3, foobar-1.4, foobar-1.5, >> foobar-1.6, foobar-2.0, foobar-2.1, foobar-2.2, foobar-2.3, >> foobar-3.0, foobar-3.1 > > This is what I have expected to do. In new releases of pgTAP, I’d probably just add version lines. I might give certainreleases names, but probably not. I’m too lazy, and if a given release has more than one new feature, it’d be a bitsilly. > > I’ve never been very keen on this approach, but then I don’t understand packaging systems very well, so it might rock,and I just don’t know how to use it properly. But I cannot tell. So the idea is that you're actually supposed to separately catalog each feature you added (e.g. each new function), so that people can depend specifically on those features. Then if you remove the function again in some distant future, you stop advertising that feature (but you can still advertise any other features you added in the same release). If you're not going to do that, then this feature as proposed is strictly worse than figuring out a way to compare version numbers, because it's more work, some people will not bother to update the provides line, and other people will sometimes forget it. I don't really have the foggiest idea how people using other packaging systems handle this. It seems like it would be a huge pain in the rear end to be continually adding Provides: lines to RPMs for every new feature that a new version of a package offers, not to mention that you'd hardly want the corresponding Requires: lines to have to enumerate all the public interfaces those packages used just in case one of them ever went away. I have a feeling I'm missing part of the picture here, somehow. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: >>> provides = foobar-1.1, foobar-1.2, foobar-1.3, foobar-1.4, foobar-1.5, >>> foobar-1.6, foobar-2.0, foobar-2.1, foobar-2.2, foobar-2.3, >>> foobar-3.0, foobar-3.1 >> >> This is what I have expected to do. In new releases of pgTAP, I’d probably >> just add version lines. I might give certain releases names, but probably >> not. I’m too lazy, and if a given release has more than one new feature, >> it’d be a bit silly. >> >> I’ve never been very keen on this approach, but then I don’t understand >> packaging systems very well, so it might rock, and I just don’t know how >> to use it properly. But I cannot tell. > > So the idea is that you're actually supposed to separately catalog > each feature you added (e.g. each new function), so that people can > depend specifically on those features. Then if you remove the > function again in some distant future, you stop advertising that > feature (but you can still advertise any other features you added in > the same release). If you're not going to do that, then this feature > as proposed is strictly worse than figuring out a way to compare > version numbers, because it's more work, some people will not bother > to update the provides line, and other people will sometimes forget > it. > > I don't really have the foggiest idea how people using other packaging > systems handle this. It seems like it would be a huge pain in the > rear end to be continually adding Provides: lines to RPMs for every > new feature that a new version of a package offers, not to mention > that you'd hardly want the corresponding Requires: lines to have to > enumerate all the public interfaces those packages used just in case > one of them ever went away. I have a feeling I'm missing part of the > picture here, somehow. Basically those examples are far too fine grained. Let's get back to the example of ip4r that I know better. I would imagine those provides lines in there: provides = ip4, ip4r provides = ip4, ip4r, ip4r_gist, ip4r_gin Then when adding ipv6 support and datatypes: provides = ip4, ip4r, ip6, ip6r provides = ip4, ip4r, ip6, ip6r, ip4r_gist, ip4r_gin, ip6r_gist, ip6r_gin Pick any one as what I would consider a realistic example. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Robert Haas <robertmhaas@gmail.com> writes: > So the idea is that you're actually supposed to separately catalog > each feature you added (e.g. each new function), so that people can > depend specifically on those features. > I don't really have the foggiest idea how people using other packaging > systems handle this. It seems like it would be a huge pain in the > rear end to be continually adding Provides: lines to RPMs for every > new feature that a new version of a package offers, not to mention > that you'd hardly want the corresponding Requires: lines to have to > enumerate all the public interfaces those packages used just in case > one of them ever went away. I have a feeling I'm missing part of the > picture here, somehow. Yeah. AFAIK, nobody actually does that. In my experience with Red Hat packages, so-called "virtual Provides" (which are exactly equivalent to this proposed feature) are used only for cases where there is or is planned to be more than one package that can supply a given API, and the Provides is really more of a logical package name than an identifier of a feature as such. When people want to depend on a feature that was added after initial release of a package, they invariably use versioned dependencies like "Requires: foobar >= nnn". And it's also pretty common to use such a dependency because you need to have a bug fixed that was fixed in version nnn; so assuming that you only need feature names for, er, features may be a mistake too. So if you look at common practice, this whole idea is wrong and we ought to define a way to compare version numbers instead. I'm not entirely sure that I want to go there yet, but it certainly bears considering as a time-tested alternative design. regards, tom lane
On ons, 2012-03-28 at 23:00 -0700, Hitoshi Harada wrote: > I totally agree with Robert's point that one feature is not > standardized and nobody can tell how you can depend on the feature in > the end. Mind you, I've never heard about building dependency by its > name as a string in other packaging system. If you want to introduce > the concept of version dependency not feature name dependency, do > *it*; I don't think feature dependency solves it. The Python setuptools (a.k.a. distutils a.k.a. distribute a.k.a. eggs a.k.a. easy_install a.k.a. dont-get-me-started) system supports feature names that a package can provide, but it uses them for a different purpose. The idea is that a package "foo" can depend on a package "bar[somethingextra]", and then bar itself would declare it's dependencies such that it depends, say, on "ham", but if feature "somethingextra" is required, it also depends on "eggs". This is actually quite useful, but it breaks down when you, say, want to wrap your egg into a Debian package.
On tor, 2012-03-29 at 09:51 +0200, Dimitri Fontaine wrote: > I don't want to introduce version dependency, because I don't think we > need it. If you want to compare what we're doing here with say debian > packaging, then look at how they package libraries. The major version > number is now part of the package name and you depend on that > directly. > > So let's take the shortcut to directly depend on the “feature” name. > > For a PostgreSQL extension example, we could pick ip4r. That will soon > include support for ipv6 (it's already done code wise, missing docs > update). If you want to use ip4r for storing ipv6, you will simply > require “ip6r” or whatever feature name is provided by the extension > including it. Note that in Debian, virtual package names (the ones you give in the Provides line) are effectively centrally managed. Most are specified by the policy, the rest are managed between the affected packages. This rests on the assumption that very little outside packaging that deviates from the official Debian packaging goes on. Since we don't have any central coordinator or authority of that kind, we need to design the system differently. At the very least, I would suggest that feature names are per-extension. So in the above example you could depend on something like "ipv4[ipv6]".
On Thu, Mar 29, 2012 at 2:28 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > On ons, 2012-03-28 at 23:00 -0700, Hitoshi Harada wrote: >> I totally agree with Robert's point that one feature is not >> standardized and nobody can tell how you can depend on the feature in >> the end. Mind you, I've never heard about building dependency by its >> name as a string in other packaging system. If you want to introduce >> the concept of version dependency not feature name dependency, do >> *it*; I don't think feature dependency solves it. > > The Python setuptools (a.k.a. distutils a.k.a. distribute a.k.a. eggs > a.k.a. easy_install a.k.a. dont-get-me-started) system supports feature > names that a package can provide, but it uses them for a different > purpose. The idea is that a package "foo" can depend on a package > "bar[somethingextra]", and then bar itself would declare it's > dependencies such that it depends, say, on "ham", but if feature > "somethingextra" is required, it also depends on "eggs". > > This is actually quite useful, Wow. That is complex, but I agree that it's useful. > but it breaks down when you, say, want to > wrap your egg into a Debian package. *blink* Huh? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Peter Eisentraut <peter_e@gmx.net> writes: > At the very least, I would suggest that feature names are per-extension. Yeah, I had about come to that conclusion too. A global namespace for them would be a mistake given lack of central coordination. regards, tom lane
On Thu, Mar 29, 2012 at 2:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Yeah. AFAIK, nobody actually does that. In my experience with Red Hat > packages, so-called "virtual Provides" (which are exactly equivalent to > this proposed feature) are used only for cases where there is or is > planned to be more than one package that can supply a given API, and the > Provides is really more of a logical package name than an identifier of > a feature as such. Note that this feature as designed will NOT service that use-case, as discussed upthread, and in fact it would probably need a full rewrite to do so, because the principle of operation is based on the dependency mechanism, which doesn't support this. In fact, ALTER EXTENSION .. UPDATE as implemented in this patch is already doing some clever tomfoolery to make things work as the user will expect - when you update an extension, it removes the dependencies between the extension and its extension features, then drops any extension features that are no longer needed, then puts back the dependencies. This produces pretty logical error messages, but I don't think it generalizes to any of the other things we might want to do (e.g. version number dependencies) so while we could commit this patch the way it is now I think doing anything more complex will require a different approach. > When people want to depend on a feature that was > added after initial release of a package, they invariably use > versioned dependencies like "Requires: foobar >= nnn". And it's also > pretty common to use such a dependency because you need to have a bug > fixed that was fixed in version nnn; so assuming that you only need > feature names for, er, features may be a mistake too. Hmm, interesting. > So if you look at common practice, this whole idea is wrong and we ought > to define a way to compare version numbers instead. I'm not entirely > sure that I want to go there yet, but it certainly bears considering as > a time-tested alternative design. I think that's definitely worth considering. One idea would be to mandate that you can only use the version-dependencies feature if both versions are of some specific form (e.g. two or three integers separated by periods). If you try to depend on foo >= 1.b or if you depend on foo >= 1.3 but the installed version is 1.b, it errors out. Frankly, I'm not sure we bet on the right horse in not mandating a version numbering scheme from the beginning. But given that we didn't, we probably don't want to get too forceful about it too quickly. However, we could ease into it by documenting a recommended numbering scheme and making features like version-dependencies work only when that scheme is used. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On tor, 2012-03-29 at 14:39 -0400, Robert Haas wrote: > > but it breaks down when you, say, want to > > wrap your egg into a Debian package. > > *blink* Huh? Well, you can't represent that mechanism in a Debian (or RPM) package dependency. So the alternatives are make it a Recommends and add a free-form explanation somewhere, or make it a hard Depends, thus overriding the fine-grained dependency setup. More to the point, I think any mechanism we dream up here that is smarter than what dpkg or rpm can represent is going to be useful only in niche situation.
On Mar 29, 2012, at 11:48 AM, Robert Haas wrote: > Frankly, I'm not sure we bet on the right horse in not mandating a > version numbering scheme from the beginning. But given that we > didn't, we probably don't want to get too forceful about it too > quickly. However, we could ease into it by documenting a recommended > numbering scheme and making features like version-dependencies work > only when that scheme is used. PGXN mandates semantic versions for this reason (currently v1.0.0): http://semver.org/spec/v1.0.0.html Removes all the ambiguity. David
Tom Lane <tgl@sss.pgh.pa.us> writes: > Peter Eisentraut <peter_e@gmx.net> writes: >> At the very least, I would suggest that feature names are per-extension. > > Yeah, I had about come to that conclusion too. A global namespace for > them would be a mistake given lack of central coordination. That's how I did it first, but Alvaro opposed to that because it allows for more than one extension to provide for the same feature name. http://archives.postgresql.org/pgsql-hackers/2012-03/msg01425.php Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes: > Tom Lane <tgl@sss.pgh.pa.us> writes: >> Peter Eisentraut <peter_e@gmx.net> writes: >>> At the very least, I would suggest that feature names are per-extension. >> Yeah, I had about come to that conclusion too. A global namespace for >> them would be a mistake given lack of central coordination. > That's how I did it first, but Alvaro opposed to that because it allows > for more than one extension to provide for the same feature name. > http://archives.postgresql.org/pgsql-hackers/2012-03/msg01425.php Right, but the question that has to be considered is how often would that be intentional as opposed to an undesirable name collision. I think Hitoshi was right upthread that it will seldom if ever be the case that somebody is independently reimplementing somebody else's API, so the use-case for intentional substitution seems thin. regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: >> That's how I did it first, but Alvaro opposed to that because it allows >> for more than one extension to provide for the same feature name. >> http://archives.postgresql.org/pgsql-hackers/2012-03/msg01425.php > > Right, but the question that has to be considered is how often would > that be intentional as opposed to an undesirable name collision. > I think Hitoshi was right upthread that it will seldom if ever be > the case that somebody is independently reimplementing somebody > else's API, so the use-case for intentional substitution seems thin. I reverted that change and we're now back to: Table "pg_catalog.pg_extension_feature" Column | Type | Modifiers ------------+------+----------- extoid | oid | not null extfeature | name | not null Indexes: "pg_extension_feature_index" UNIQUE, btree (extoid, extfeature) "pg_extension_feature_oid_index" UNIQUE, btree (oid) Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachment
On tor, 2012-03-29 at 14:48 -0400, Robert Haas wrote: > Frankly, I'm not sure we bet on the right horse in not mandating a > version numbering scheme from the beginning. But given that we > didn't, we probably don't want to get too forceful about it too > quickly. However, we could ease into it by documenting a recommended > numbering scheme and making features like version-dependencies work > only when that scheme is used. Or an extension could specify itself which version numbering scheme it uses. This just has to be a reference to a type, which in turn could be semver, debversion, or even just numeric or text (well, maybe name). Then you'd just need to use the comparison operators of that type to figure things out.
On Apr 2, 2012, at 11:24 AM, Peter Eisentraut wrote: > Or an extension could specify itself which version numbering scheme it > uses. This just has to be a reference to a type, which in turn could be > semver, debversion, or even just numeric or text (well, maybe name). > Then you'd just need to use the comparison operators of that type to > figure things out. Sounds like a lot of work for core to maintain various version comparison schemes… David
"David E. Wheeler" <david@justatheory.com> writes: > On Apr 2, 2012, at 11:24 AM, Peter Eisentraut wrote: >> Or an extension could specify itself which version numbering scheme it >> uses. This just has to be a reference to a type, which in turn could be >> semver, debversion, or even just numeric or text (well, maybe name). >> Then you'd just need to use the comparison operators of that type to >> figure things out. > Sounds like a lot of work for core to maintain various version comparison schemes Well, the primary argument for avoiding version comparison semantics to begin with was exactly that we didn't want to mandate a particular version-numbering scheme. However, if we're going to decide that we have to have version comparisons, I think we should just bite the bullet and specify one version numbering scheme. More than one is going to add complexity, sow confusion, and not really buy anything. regards, tom lane
On Apr 2, 2012, at 11:58 AM, Tom Lane wrote: >> Sounds like a lot of work for core to maintain various version comparison schemes > > Well, the primary argument for avoiding version comparison semantics to > begin with was exactly that we didn't want to mandate a particular > version-numbering scheme. However, if we're going to decide that we > have to have version comparisons, I think we should just bite the bullet > and specify one version numbering scheme. More than one is going to add > complexity, sow confusion, and not really buy anything. Precisely my thinking. Best, David
Tom Lane <tgl@sss.pgh.pa.us> writes: >> On Apr 2, 2012, at 11:24 AM, Peter Eisentraut wrote: >>> Or an extension could specify itself which version numbering scheme it >>> uses. This just has to be a reference to a type, which in turn could be >>> semver, debversion, or even just numeric or text (well, maybe name). >>> Then you'd just need to use the comparison operators of that type to >>> figure things out. That's exactly what I'm trying to avoid :) > Well, the primary argument for avoiding version comparison semantics to > begin with was exactly that we didn't want to mandate a particular > version-numbering scheme. However, if we're going to decide that we > have to have version comparisons, I think we should just bite the bullet > and specify one version numbering scheme. More than one is going to add > complexity, sow confusion, and not really buy anything. I still believe we don't *need* any numbering scheme for extension versions. Now, maybe we as a community want one. I'm voting against. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Tue, Apr 3, 2012 at 4:15 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Tom Lane <tgl@sss.pgh.pa.us> writes: >>> On Apr 2, 2012, at 11:24 AM, Peter Eisentraut wrote: >>>> Or an extension could specify itself which version numbering scheme it >>>> uses. This just has to be a reference to a type, which in turn could be >>>> semver, debversion, or even just numeric or text (well, maybe name). >>>> Then you'd just need to use the comparison operators of that type to >>>> figure things out. > > That's exactly what I'm trying to avoid :) > >> Well, the primary argument for avoiding version comparison semantics to >> begin with was exactly that we didn't want to mandate a particular >> version-numbering scheme. However, if we're going to decide that we >> have to have version comparisons, I think we should just bite the bullet >> and specify one version numbering scheme. More than one is going to add >> complexity, sow confusion, and not really buy anything. > > I still believe we don't *need* any numbering scheme for extension > versions. Now, maybe we as a community want one. I'm voting against. Examining this thread, I think there is insufficient consensus to push this patch into 9.2. It's not entirely clear that this patch isn't what we want, but it's not entirely clear that it is what we want either, and I think it's too late in the release cycle to push anything into the release that we're not fairly sure we actually want, because there won't be time to revisit that decision before this ends up out in the wild. It's also not the sort of thing we can just whack around in the next release if we get it wrong, because APIs for coping with versioning are exactly the kind of thing that you can't go change at the drop of a hat. I'm therefore marking this Returned with Feedback. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > Examining this thread, I think there is insufficient consensus to push > this patch into 9.2. It's not entirely clear that this patch isn't > what we want, but it's not entirely clear that it is what we want > either, and I think it's too late in the release cycle to push > anything into the release that we're not fairly sure we actually want, > because there won't be time to revisit that decision before this ends > up out in the wild. It's also not the sort of thing we can just whack > around in the next release if we get it wrong, because APIs for coping > with versioning are exactly the kind of thing that you can't go change > at the drop of a hat. I'm therefore marking this Returned with > Feedback. Fair enough I guess. The first extension patch took me more than 2 years∫of brewing before we get to a consensus on what we wanted in core, I shouldn't have assumed it would ease the path for next improvements too much. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support