Finer Extension dependencies

On Sun, Dec 18, 2011 at 6:36 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Hi,

The extensions work we began in 9.1 is not yet finished entirely
(*cough*), so I'm opening a new patch series here by attacking the
dependency problems.

Some people want us to manage extension version numbers with sorting
semantics so that we are able to depend on foo >= 1.2 and crazy things
like this, and I think the need is reasonable and easier than that to
address.

The patch applies with one reject, which I could fix easily. The make
check passed.

extension-provides.v1.patch extensions.docx
haradh1-mac:postgresql-head haradh1$ patch -p1 <
~/Downloads/extension-provides.v1.patch
patching file contrib/pg_upgrade_support/pg_upgrade_support.c
patching file doc/src/sgml/catalogs.sgml
Hunk #2 succeeded at 3063 (offset 11 lines).
Hunk #3 succeeded at 6865 (offset 11 lines).
patching file doc/src/sgml/extend.sgml
patching file src/backend/catalog/Makefile
patching file src/backend/catalog/dependency.c
patching file src/backend/catalog/system_views.sql
patching file src/backend/commands/extension.c
patching file src/include/catalog/dependency.h
patching file src/include/catalog/indexing.h
patching file src/include/catalog/pg_extension_feature.h
patching file src/include/catalog/pg_proc.h
Hunk #1 succeeded at 4341 (offset 25 lines).
patching file src/include/commands/extension.h
patching file src/test/regress/expected/rules.out
patching file src/test/regress/expected/sanity_check.out
Hunk #1 succeeded at 103 (offset 1 line).
Hunk #2 FAILED at 163.
1 out of 2 hunks FAILED -- saving rejects to file
src/test/regress/expected/sanity_check.out.rej

What this patch does is basically:
- add pg_extension_feature to store "feature" (name) provided by extensions
- extension control file now has "provide" parameter to indicate
"feature", which is comma separated
- when creating an extension, the backend looks for "feature" required
in control file
- the installed extension has dependency on "feature"

So, the first thing is catalog.

db1=# \d pg_extension_feature;
Table "pg_catalog.pg_extension_feature"
Column | Type | Modifiers
------------+------+-----------
extoid | oid | not 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.tanuki@gmail.com> writes:

The patch applies with one reject, which I could fix easily. The make
check passed.

Bitrot happens fast in this season… will produce another version of the
patch.

Table "pg_catalog.pg_extension_feature"
Column | Type | Modifiers
------------+------+-----------
extoid | oid | not null
extfeature | name | not null
Indexes:
"pg_extension_feature_index" UNIQUE, btree (extoid, extfeature)
"pg_extension_feature_oid_index" UNIQUE, btree (oid)

* I'm not quit sure why pg_extension_feature_index needs extoid column.

That allows managing features per extension: you need to know which
extension is providing which feature to be able to solve dependencies.

* I have a big question to add two-column catalog. I don't mind the
actual number of columns, but if the table has only two columns, it
implies the design may be bad. Only two column catalog other than this
is pg_largeobject_metadata.

We need each feature to be a full PostgreSQL object so that we can use
the dependency tracking. That allows to manage DROP EXTENSION foo and
cascade to extensions that depend on feature(s) provided by foo.

It also allows to track at update time when the feature set changes if
we're removing a feature that some other installed extension is
depending on, allowing our users to know that they should either drop
this other extension or update it too.

Next, some questions:
- Why is the finer dependency needed? Do you have tangible example
that struggles with the dependency granularity? I feel so good about
the existing dependency on extension as an extension developer of
several ones.

The problem is not yet very apparent only because extensions are very
new. The main thing we address with this patch is depending on a feature
that appeared while developing an extension or that gets removed down
the line. It allows to depend on features and avoid needing to compare
version numbers and maintain a list of which version number is providing
which feature.

This feature has been asked by several extension users, beginning even
before 9.1 got released.

- What happens if DROP EXTENSION ... CASCADE? Does it work?

It should, what happens when you try? :)

- How does pg_upgrade interact with this? The dependency changes.

We're compatible because the extension name itself is always considered
as a feature and added to the catalogs, so that e.g. require = 'cube'
will now target a feature rather than an extension, and this feature
defaults to being provided by the 'create extension cube' statement that
pg_upgrade issues given 9.1 dependency tracking.

A minor memo.
list_extension_features(): I guess the size argument for repalloc is
bogus.

Oh, a quick review of repalloc call points seems to indicate you're
right, I'll fix that in the next version of the patch.

So, that's pretty much I've reviewed quickly. I'll need more time to
look in detail, but I'd like more inputs for the high-level design and
direction.

I hope to be answering to your questions here, please ask for more
details as you need them!

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

On 21 Leden 2012, 18:20, Dimitri Fontaine wrote:

Hi,

Thank you for reviewing this patch!

Hitoshi Harada <umi.tanuki@gmail.com> writes:

Next, some questions:
- Why is the finer dependency needed? Do you have tangible example
that struggles with the dependency granularity? I feel so good about
the existing dependency on extension as an extension developer of
several ones.

The problem is not yet very apparent only because extensions are very
new. The main thing we address with this patch is depending on a feature
that appeared while developing an extension or that gets removed down
the line. It allows to depend on features and avoid needing to compare
version numbers and maintain a list of which version number is providing
which feature.

This feature has been asked by several extension users, beginning even
before 9.1 got released.

It's also about several extension providing the same sort of functionality
implemented in different ways. Think about extensions that allow you to
send e-mails right from the database. One could do that directly, the
other one could implement queuing, another one could be optimized for a
specific SMTP server. With features, you could just say 'require = mail'
and use the extension that's installed. Yes, this needs a common API.

I personally see this as a major step towards fully-fledged package
management, similar to those available in modern Linux distributions (apt,
yum, portage, ...).

Tomas

On Sat, Jan 21, 2012 at 9:20 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Hi,

Thank you for reviewing this patch!

Hitoshi Harada <umi.tanuki@gmail.com> writes:

The patch applies with one reject, which I could fix easily. The make
check passed.

Bitrot happens fast in this season…  will produce another version of the
patch.

Table "pg_catalog.pg_extension_feature"
   Column   | Type | Modifiers
------------+------+-----------
 extoid     | oid  | not null
 extfeature | name | not null
Indexes:
    "pg_extension_feature_index" UNIQUE, btree (extoid, extfeature)
    "pg_extension_feature_oid_index" UNIQUE, btree (oid)

* I'm not quit sure why pg_extension_feature_index needs extoid column.

That allows managing features per extension: you need to know which
extension is providing which feature to be able to solve dependencies.

Do you mean you want UNIQUE constraint by this index? I found the
usage is to search feature by (only) its name, so I wondered if extoid
is not necessary.

* I have a big question to add two-column catalog. I don't mind the
actual number of columns, but if the table has only two columns, it
implies the design may be bad. Only two column catalog other than this
is pg_largeobject_metadata.

We need each feature to be a full PostgreSQL object so that we can use
the dependency tracking.  That allows to manage DROP EXTENSION foo and
cascade to extensions that depend on feature(s) provided by foo.

I guess if we spend more time, we'll figure out what is "feature"
actually, and then will see what kind of columns/attributes are needed
to represent it. Although I agree we can add them later, again, this
may imply the design is premature. (it's ok if i am the only person
who thinks so)

Next, some questions:
- Why is the finer dependency needed? Do you have tangible example
that struggles with the dependency granularity? I feel so good about
the existing dependency on extension as an extension developer of
several ones.

The problem is not yet very apparent only because extensions are very
new. The main thing we address with this patch is depending on a feature
that appeared while developing an extension or that gets removed down
the line. It allows to depend on features and avoid needing to compare
version numbers and maintain a list of which version number is providing
which feature.

This feature has been asked by several extension users, beginning even
before 9.1 got released.

- What happens if DROP EXTENSION ... CASCADE? Does it work?

It should, what happens when you try? :)

I just tried DROP EXTENSION now, and found it broken :(

db1=# create extension kmeans;
CREATE EXTENSION
db1=# drop extension kmeans;
ERROR: cannot drop extension kmeans because extension feature kmeans
requires it
HINT: You can drop extension feature kmeans instead.
db1=# drop extension kmeans cascade;
ERROR: cannot drop extension kmeans because extension feature kmeans
requires it
HINT: You can drop extension feature kmeans instead.

Am I missing something? I'm confused why this happens.

Thanks,
--
Hitoshi Harada

Hitoshi Harada <umi.tanuki@gmail.com> writes:

    "pg_extension_feature_index" UNIQUE, btree (extoid, extfeature)

Do you mean you want UNIQUE constraint by this index? I found the
usage is to search feature by (only) its name, so I wondered if extoid
is not necessary.

I guess you're right and that's something I've just left when I should
have cleaned it. We need to find which extension is providing which
feature, and we need feature names to be globally unique. I'll remove
extoid from this index in the next revision on the patch.

I'm not in a position to provide that next revision just now, that would
happen before the end of the week though.

I guess if we spend more time, we'll figure out what is "feature"
actually, and then will see what kind of columns/attributes are needed
to represent it. Although I agree we can add them later, again, this
may imply the design is premature. (it's ok if i am the only person
who thinks so)

You might be right that a feature is more than just a unique name but as
things are, that's their only useful property.

- What happens if DROP EXTENSION ... CASCADE? Does it work?

It should, what happens when you try? :)

I just tried DROP EXTENSION now, and found it broken :(

db1=# create extension kmeans;
CREATE EXTENSION
db1=# drop extension kmeans;
ERROR: cannot drop extension kmeans because extension feature kmeans
requires it
HINT: You can drop extension feature kmeans instead.

Can you provide me the test case you've been using? That looks like a
bug I need to fix, indeed (unless the problem lies in the test case,
which would mean I need to tighten things some more).

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

On Mon, Jan 23, 2012 at 2:00 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Hitoshi Harada <umi.tanuki@gmail.com> writes:

- What happens if DROP EXTENSION ... CASCADE? Does it work?

It should, what happens when you try? :)

I just tried DROP EXTENSION now, and found it broken :(

db1=# create extension kmeans;
CREATE EXTENSION
db1=# drop extension kmeans;
ERROR:  cannot drop extension kmeans because extension feature kmeans
requires it
HINT:  You can drop extension feature kmeans instead.

Can you provide me the test case you've been using?  That looks like a
bug I need to fix, indeed (unless the problem lies in the test case,
which would mean I need to tighten things some more).

The test case is just above; createdb db1 and create and drop an
extension. The kmean extension is on pgxn. I tried my small test
extension named ext1 which contains only one plpgsql function, and
created it then dropped it, reproduced.

Thanks,
--
Hitoshi Harada

On Mon, Jan 23, 2012 at 3:06 AM, Hitoshi Harada <umi.tanuki@gmail.com> wrote:

On Mon, Jan 23, 2012 at 2:00 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Hitoshi Harada <umi.tanuki@gmail.com> writes:

- What happens if DROP EXTENSION ... CASCADE? Does it work?

It should, what happens when you try? :)

I just tried DROP EXTENSION now, and found it broken :(

db1=# create extension kmeans;
CREATE EXTENSION
db1=# drop extension kmeans;
ERROR:  cannot drop extension kmeans because extension feature kmeans
requires it
HINT:  You can drop extension feature kmeans instead.

Can you provide me the test case you've been using?  That looks like a
bug I need to fix, indeed (unless the problem lies in the test case,
which would mean I need to tighten things some more).

The test case is just above; createdb db1 and create and drop an
extension. The kmean extension is on pgxn. I tried my small test
extension named ext1 which contains only one plpgsql function, and
created it then dropped it, reproduced.

Ping. In case you don't have updates soon, I'll mark Returned with Feedback.

Thanks,
--
Hitoshi Harada

Hitoshi Harada <umi.tanuki@gmail.com> writes:

Ping. In case you don't have updates soon, I'll mark Returned with Feedback.

Pong. Sorry about my recent silence.

I've not been in a position to work on this recently, and am done with
those other duties now. I intend to be posting an updated patch soon
now. Please wait some more before punting this patch out of the current
commit fest.

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

Hi,

Sorry for the delays, I'm back on PostgreSQL related work again.

Hitoshi Harada <umi.tanuki@gmail.com> writes:

I just tried DROP EXTENSION now, and found it broken :(

Please find v2 of the patch. I did change the dependency management in
between the simple cases and the more challenging ones and forgot that I
had to retest it all in between, which is what happen on a tight
schedule and when working at night, I guess.

So the best option I've found here had me add a new function in
pg_depend.c, it's working as intended now.

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

On Mon, Feb 13, 2012 at 3:18 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Hi,

Sorry for the delays, I'm back on PostgreSQL related work again.

Hitoshi Harada <umi.tanuki@gmail.com> writes:

I just tried DROP EXTENSION now, and found it broken :(

Please find v2 of the patch.  I did change the dependency management in
between the simple cases and the more challenging ones and forgot that I
had to retest it all in between, which is what happen on a tight
schedule and when working at night, I guess.

The patch is partially rejected due to the pg_proc column changes from
leakproof, but I could apply manually.

I confirmed DROP EXTENSION is fixed now. In turn, it seems to me
"requires" doesn't work. My test ext2.control looks like:

comment = 'sample1'
default_version = '1.0'
requires = 'featZ'
relocatable = true

And simply this extension can be installed against cleanly-initialized
database. I double-checked there's no entry for featz in
pg_extension_feature.

Also, I found that if control file has duplicate names in "provides",
the error is not friendly ("duplicate entry for pg_extension_feature",
or something). This is same if "provides" has the extension name
itself.

I'll have a look more but give comments so far so that you can find
solutions to them soon.

Thanks,
--
Hitoshi Harada

Hitoshi Harada <umi.tanuki@gmail.com> writes:

I confirmed DROP EXTENSION is fixed now. In turn, it seems to me
"requires" doesn't work. My test ext2.control looks like:

I'm very sorry about that. It's all about playing with pg_depend and
I've failed to spend enough time on that very topic to send a patch that
just works, it seems.

I'm going to fix that over the week-end. Thanks for your reviewing so
far.

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

On Fri, Feb 24, 2012 at 2:09 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Hitoshi Harada <umi.tanuki@gmail.com> writes:

I confirmed DROP EXTENSION is fixed now.  In turn, it seems to me
"requires" doesn't work.  My test ext2.control looks like:

I'm very sorry about that. It's all about playing with pg_depend and
I've failed to spend enough time on that very topic to send a patch that
just works, it seems.

I'm going to fix that over the week-end.  Thanks for your reviewing so
far.

Quickly reviewed the patch and found some issues.

- There are some mixture of pg_extension_feature and pg_extension_feature"s"
- The doc says pg_extension_feature"s" has four columns but it's not true.
- Line 608 is bad. In the loop, provides_itself is repeatedly changed
to true and false and I guess that's not what you meant.
- Line 854+, you can fold two blocks into one. The two blocks are
similar and by giving provides list with list_make1 when
control->provides == NIL you can do it in one block.
- s/trak/track/
- Line 960, you missed updating classId for dependency.

That's pretty much from me. I just looked at the patch and have no
idea about grand architecture. Marking Waiting on Author.

Thanks,
--
Hitoshi Harada

On Tue, Feb 28, 2012 at 5:34 AM, Hitoshi Harada <umi.tanuki@gmail.com> wrote:

Quickly reviewed the patch and found some issues.

- There are some mixture of pg_extension_feature and pg_extension_feature"s"
- The doc says pg_extension_feature"s" has four columns but it's not true.
- Line 608 is bad. In the loop, provides_itself is repeatedly changed
to true and false and I guess that's not what you meant.
- Line 854+, you can fold two blocks into one.  The two blocks are
similar and by giving provides list with list_make1 when
control->provides  == NIL you can do it in one block.
- s/trak/track/
- Line 960, you missed updating classId for dependency.

That's pretty much from me.  I just looked at the patch and have no
idea about grand architecture.  Marking Waiting on Author.

Dimitri, are you going to post an updated patch for this CF?

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

Robert Haas <robertmhaas@gmail.com> writes:

Dimitri, are you going to post an updated patch for this CF?

Yes, I intend to do that. Not sure about diverting from the command
trigger patch while Thom is full speed on reviewing and helping me write
the full covering test cases, though.

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

On Thu, Mar 8, 2012 at 9:39 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Dimitri, are you going to post an updated patch for this CF?

Yes, I intend to do that.  Not sure about diverting from the command
trigger patch while Thom is full speed on reviewing and helping me write
the full covering test cases, though.

I don't think we can wait any longer for this; we're now more than two
months in to this CommitFest, and command triggers is still in full
swing.

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

Robert Haas <robertmhaas@gmail.com> writes:

I don't think we can wait any longer for this; we're now more than two
months in to this CommitFest, and command triggers is still in full
swing.

Is it possible to have another day to send out a revised patch? The
problem reported is either a show stopper or a less-than-one-hour fix, I
would hate to miss 9.2 for having been swamped so much as to miss the
time to qualify the problem.

Baring objections, I'll send a new revision later tonight or tomorrow,
or a notification that the patch is really dead for 9.2.

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

On Wed, Mar 21, 2012 at 11:11 AM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I don't think we can wait any longer for this; we're now more than two
months in to this CommitFest, and command triggers is still in full
swing.

Is it possible to have another day to send out a revised patch?  The
problem reported is either a show stopper or a less-than-one-hour fix, I
would hate to miss 9.2 for having been swamped so much as to miss the
time to qualify the problem.

Baring objections, I'll send a new revision later tonight or tomorrow,
or a notification that the patch is really dead for 9.2.

Sounds reasonable to me.

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

Hi,

Again, thanks very much for the review. Here's an updated patch (just
merged against master) fixing most of your comments here. I couldn't
reproduce previous problems with the attached:

- DROP EXTENSION was broken, asking to cascade to self
- CREATE EXTENSION was bypassing "requires"

I could reproduce the second problem then fix it with the following one
liner. I missed it because my test case still fails for not finding the
cube type rather than the cube extension without this fix:

-	if (!OidIsValid(featoid) && !missing_ok)
+	if (!OidIsValid(*featoid) && !missing_ok)

Thank you all for your patience while I was busy elsewhere, it's
definitely not a show stopper in my book :)

dim=# create extension earthdistance;
ERROR: feature "cube" is not currently provided
HINT: Please install an extension that provides it first

dim=# create extension cube;
CREATE EXTENSION

dim=# create extension earthdistance;
CREATE EXTENSION

dim=# drop extension cube cascade;
NOTICE: drop cascades to extension earthdistance
DROP EXTENSION

Hitoshi Harada <umi.tanuki@gmail.com> writes:

- There are some mixture of pg_extension_feature and pg_extension_feature"s"

Fixed.

- The doc says pg_extension_feature"s" has four columns but it's not true.

Well the SGML table describing the catalog has 4 cols :)

- Line 608 is bad. In the loop, provides_itself is repeatedly changed
to true and false and I guess that's not what you meant.

Fixed.

- Line 854+, you can fold two blocks into one. The two blocks are
similar and by giving provides list with list_make1 when
control->provides == NIL you can do it in one block.

Fixed.

- s/trak/track/

Fixed, I guess the English would need rephrasing.

- Line 960, you missed updating classId for dependency.

I don't think so.

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

Excerpts from Dimitri Fontaine's message of jue mar 22 14:38:29 -0300 2012:

Hi,

Again, thanks very much for the review. Here's an updated patch (just
merged against master) fixing most of your comments here. I couldn't
reproduce previous problems with the attached:

get_available_versions_for_extension seems to contain a bunch of
commented-out lines ...

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support