Re: 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

On Sun, Dec 18, 2011 at 6:36 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)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 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.
* 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(dot)tanuki(at)gmail(dot)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(dot)tanuki(at)gmail(dot)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(at)2ndquadrant(dot)fr> wrote:
> Hi,
>
> Thank you for reviewing this patch!
>
> Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)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(dot)tanuki(at)gmail(dot)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(at)2ndquadrant(dot)fr> wrote:
> Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)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(dot)tanuki(at)gmail(dot)com> wrote:
> On Mon, Jan 23, 2012 at 2:00 AM, Dimitri Fontaine
> <dimitri(at)2ndquadrant(dot)fr> wrote:
>> Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)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(dot)tanuki(at)gmail(dot)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(dot)tanuki(at)gmail(dot)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

On Mon, Feb 13, 2012 at 3:18 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Hi,
>
> Sorry for the delays, I'm back on PostgreSQL related work again.
>
> Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)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(dot)tanuki(at)gmail(dot)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(at)2ndquadrant(dot)fr> wrote:
> Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)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(dot)tanuki(at)gmail(dot)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(at)gmail(dot)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(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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(at)gmail(dot)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(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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(dot)tanuki(at)gmail(dot)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

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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)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

Excerpts from Dimitri Fontaine's message of jue mar 22 15:08:27 -0300 2012:
> Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)2ndQuadrant(dot)fr> writes:
> Alvaro Herrera <alvherre(at)commandprompt(dot)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

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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)2ndquadrant(dot)fr> wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)gmail(dot)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 fedora teams)

> 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(at)2ndquadrant(dot)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(at)gmail(dot)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(at)2ndquadrant(dot)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(at)gmail(dot)com> writes:
> Could you possibly generate a new diff to save me the trouble of
> fixing the one you sent before?

Please find it attached, it looks better now, and I rebased it against
master for good measure (no conflicts).

> 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

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

Comments:

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

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

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

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

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

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

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

#endif /* PG_PROC_H */
-

Useless hunk.

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

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

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

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

+extern char * get_extension_feature_name(Oid featoid);

Extra space.

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

Missing space (after the comma).

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

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

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

On Wed, Mar 28, 2012 at 8:52 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Mar 28, 2012 at 11:28 AM, Dimitri Fontaine
> <dimitri(at)2ndquadrant(dot)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(dot)tanuki(at)gmail(dot)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(at)2ndquadrant(dot)fr> wrote:
> Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)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(dot)tanuki(at)gmail(dot)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(at)gmail(dot)com> writes:
> I think the lack of pg_upgrade support is a must-fix before commit.

I though that would only be a TODO for 9.2 to 9.3 upgrades. When
upgrading from 9.1 to 9.2, pg_upgrade will directly stuff extensions
using InsertExtensionTuple() with an empty features list. That will call
insert_extension_features() which ensures that the extension name is
registered as a feature even when not explicitly listed (backward
compatibility with control files).

So I think we're covered now, but still need to revisit the issue later.

I edited the comment this way:

List *features = NIL; /* 9.3 should get features from catalogs */

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

It's already in the pg_available_extension_versions system's view, which
is not covered by psql (yet). Do we want a new \dx derived command?

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

Done.

> List *requires; /* names of prerequisite extensions */
> + List *provides; /* names of provided features */
>
> Comment in requires line of above hunk should be updated to say
> "prerequisite features".

Done.

> -
>
> Useless hunk.

That must be my editor adding a final line when I visit files… and I
can't seem to be able to prevent this hunk from happening?

> + errmsg("parameter \"%s\" must be a
> list of extension names",
>
> This error, which relates to the parsing of the "provides" list, say
> "extension features", and the existing code for "requires" needs to be
> updated to say that as well.

Done.

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

Done.

> +extern char * get_extension_feature_name(Oid featoid);
>
> Extra space.

Fixed.

> + if (strcmp(curreq,pcontrol->name) != 0)
>
> Missing space (after the comma).

Fixed.

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

Right, done now.

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

On Thu, Mar 29, 2012 at 4:37 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)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

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

Best,

David

On Thu, Mar 29, 2012 at 1:48 PM, David E. Wheeler <david(at)justatheory(dot)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 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.

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

Robert Haas <robertmhaas(at)gmail(dot)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(at)gmail(dot)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(at)gmx(dot)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(at)gmx(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Peter Eisentraut <peter_e(at)gmx(dot)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(at)2ndQuadrant(dot)fr> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> Peter Eisentraut <peter_e(at)gmx(dot)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(at)sss(dot)pgh(dot)pa(dot)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

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(at)justatheory(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)2ndquadrant(dot)fr> wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)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