Thread: creating extension including dependencies

Hi,

I am getting tired installing manually required extensions manually. I
was wondering if we might want to add option to CREATE SEQUENCE that
would allow automatic creation of the extensions required by the
extension that is being installed by the user.

I also wrote some prototype patch that implements this.

I call it prototype mainly because the syntax (CREATE EXTENSION ...
RECURSIVE) could be improved, I originally wanted to do something like
INCLUDING DEPENDENCIES but that need news (unreserved) keyword and I
don't think it's worth it, plus it's wordy.

--
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

<p dir="ltr">+1<p dir="ltr">Is it working in runtime too? <br /><div class="gmail_quote">Dne 15.6.2015 0:51 napsal
uživatel"Petr Jelinek" <<a href="mailto:petr@2ndquadrant.com">petr@2ndquadrant.com</a>>:<br type="attribution"
/><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br /><br />
Iam getting tired installing manually required extensions manually. I was wondering if we might want to add option to
CREATESEQUENCE that would allow automatic creation of the extensions required by the extension that is being installed
bythe user.<br /><br /> I also wrote some prototype patch that implements this.<br /><br /> I call it prototype mainly
becausethe syntax (CREATE EXTENSION ... RECURSIVE) could be improved, I originally wanted to do something like
INCLUDINGDEPENDENCIES but that need news (unreserved) keyword and I don't think it's worth it, plus it's wordy.<br
/><br/> -- <br />  Petr Jelinek                  <a href="http://www.2ndQuadrant.com/" rel="noreferrer"
target="_blank">http://www.2ndQuadrant.com/</a><br/>  PostgreSQL Development, 24x7 Support, Training & Services<br
/><br/><br /> --<br /> Sent via pgsql-hackers mailing list (<a
href="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<br/> To make changes to your
subscription:<br/><a href="http://www.postgresql.org/mailpref/pgsql-hackers" rel="noreferrer"
target="_blank">http://www.postgresql.org/mailpref/pgsql-hackers</a><br/><br /></blockquote></div> 

On 06/15/2015 12:50 AM, Petr Jelinek wrote:
> Hi,
> 
> I am getting tired installing manually required extensions manually. I
> was wondering if we might want to add option to CREATE SEQUENCE that
> would allow automatic creation of the extensions required by the
> extension that is being installed by the user.

I would like this, too.

> I call it prototype mainly because the syntax (CREATE EXTENSION ...
> RECURSIVE) could be improved, I originally wanted to do something like
> INCLUDING DEPENDENCIES but that need news (unreserved) keyword and I
> don't think it's worth it, plus it's wordy.

If we're bikeshedding already, I prefer CREATE EXTENSION ... CASCADE;
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

On Mon, Jun 15, 2015 at 7:50 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:
> Hi,
>
> I am getting tired installing manually required extensions manually. I was
> wondering if we might want to add option to CREATE SEQUENCE that would allow
> automatic creation of the extensions required by the extension that is being
> installed by the user.

I'm wondering how much helpful this feature is. Because, even if we can save
some steps for CREATE EXTENSION by using the feature, we still need to
manually find out, download and install all the extensions that the target
extension depends on. So isn't it better to implement the tool like yum, i.e.,
which performs all those steps almost automatically, rather than the proposed
feature? Maybe it's outside PostgreSQL core.

Regards,

-- 
Fujii Masao

On 2015-07-07 22:36:29 +0900, Fujii Masao wrote:
> On Mon, Jun 15, 2015 at 7:50 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:
> > Hi,
> >
> > I am getting tired installing manually required extensions manually. I was
> > wondering if we might want to add option to CREATE SEQUENCE that would allow
> > automatic creation of the extensions required by the extension that is being
> > installed by the user.
> 
> I'm wondering how much helpful this feature is. Because, even if we can save
> some steps for CREATE EXTENSION by using the feature, we still need to
> manually find out, download and install all the extensions that the target
> extension depends on. So isn't it better to implement the tool like yum, i.e.,
> which performs all those steps almost automatically, rather than the proposed
> feature? Maybe it's outside PostgreSQL core.

That doesn't seem to make much sense to me. Something like yum can't
install everything in all relevant databases. Sure, yum will be used to
install dependencies between extensions on the filesystem level.

At the minimum I'd like to see that CREATE EXTENSION foo; would install
install extension 'bar' if foo dependended on 'bar' if CASCADE is
specified. Right now we always error out saying that the dependency on
'bar' is not fullfilled - not particularly helpful.

On Jul 7, 2015, at 6:41 AM, Andres Freund <andres@anarazel.de> wrote:

> At the minimum I'd like to see that CREATE EXTENSION foo; would install
> install extension 'bar' if foo dependended on 'bar' if CASCADE is
> specified. Right now we always error out saying that the dependency on
> 'bar' is not fullfilled - not particularly helpful.

+1

If `yum install foo` also installs bar, and `pgxn install foo` downloads, builds, and installs bar, it makes sense to
methat `CREATE EXTENSION foo` would install bar if it was available, and complain if it wasn’t. 

Best,

David

On 2015-07-07 15:41, Andres Freund wrote:
> On 2015-07-07 22:36:29 +0900, Fujii Masao wrote:
>> On Mon, Jun 15, 2015 at 7:50 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:
>>> Hi,
>>>
>>> I am getting tired installing manually required extensions manually. I was
>>> wondering if we might want to add option to CREATE SEQUENCE that would allow
>>> automatic creation of the extensions required by the extension that is being
>>> installed by the user.
>>
>> I'm wondering how much helpful this feature is. Because, even if we can save
>> some steps for CREATE EXTENSION by using the feature, we still need to
>> manually find out, download and install all the extensions that the target
>> extension depends on. So isn't it better to implement the tool like yum, i.e.,
>> which performs all those steps almost automatically, rather than the proposed
>> feature? Maybe it's outside PostgreSQL core.
>
> That doesn't seem to make much sense to me. Something like yum can't
> install everything in all relevant databases. Sure, yum will be used to
> install dependencies between extensions on the filesystem level.
>
> At the minimum I'd like to see that CREATE EXTENSION foo; would install
> install extension 'bar' if foo dependended on 'bar' if CASCADE is
> specified. Right now we always error out saying that the dependency on
> 'bar' is not fullfilled - not particularly helpful.
>

That's what the proposed patch does (with slightly different syntax but 
syntax is something that can be changed easily).

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On 07/09/2015 07:05 PM, Petr Jelinek wrote:
> On 2015-07-07 15:41, Andres Freund wrote:
>> On 2015-07-07 22:36:29 +0900, Fujii Masao wrote:
>>> On Mon, Jun 15, 2015 at 7:50 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:
>>>> Hi,
>>>>
>>>> I am getting tired installing manually required extensions manually. I was
>>>> wondering if we might want to add option to CREATE SEQUENCE that would allow
>>>> automatic creation of the extensions required by the extension that is being
>>>> installed by the user.
>>>
>>> I'm wondering how much helpful this feature is. Because, even if we can save
>>> some steps for CREATE EXTENSION by using the feature, we still need to
>>> manually find out, download and install all the extensions that the target
>>> extension depends on. So isn't it better to implement the tool like yum, i.e.,
>>> which performs all those steps almost automatically, rather than the proposed
>>> feature? Maybe it's outside PostgreSQL core.
>>
>> That doesn't seem to make much sense to me. Something like yum can't
>> install everything in all relevant databases. Sure, yum will be used to
>> install dependencies between extensions on the filesystem level.
>>
>> At the minimum I'd like to see that CREATE EXTENSION foo; would install
>> install extension 'bar' if foo dependended on 'bar' if CASCADE is
>> specified. Right now we always error out saying that the dependency on
>> 'bar' is not fullfilled - not particularly helpful.
>
> That's what the proposed patch does (with slightly different syntax but
> syntax is something that can be changed easily).

This seems quite reasonable, but I have to ask: How many extensions are 
there out there that depend on another extension? Off the top of my 
head, I can't think of any..

- Heikki

<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">On Fri, Jul 10, 2015 at 10:09 PM, Heikki
Linnakangas<span dir="ltr"><<a href="mailto:hlinnaka@iki.fi" target="_blank">hlinnaka@iki.fi</a>></span>
wrote:<br/><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex"><spanclass="">On 07/09/2015 07:05 PM, Petr Jelinek wrote:<br /><blockquote
class="gmail_quote"style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> On
2015-07-0715:41, Andres Freund wrote:<br /><blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1pxsolid rgb(204,204,204);padding-left:1ex"> On 2015-07-07 22:36:29 +0900, Fujii Masao wrote:<br
/><blockquoteclass="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">On Mon, Jun 15, 2015 at 7:50 AM, Petr Jelinek <<a
href="mailto:petr@2ndquadrant.com"target="_blank">petr@2ndquadrant.com</a>> wrote:<br /><blockquote
class="gmail_quote"style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Hi,<br
/><br/> I am getting tired installing manually required extensions manually. I was<br /> wondering if we might want to
addoption to CREATE SEQUENCE that would allow<br /> automatic creation of the extensions required by the extension that
isbeing<br /> installed by the user.<br /></blockquote><br /> I'm wondering how much helpful this feature is. Because,
evenif we can save<br /> some steps for CREATE EXTENSION by using the feature, we still need to<br /> manually find
out,download and install all the extensions that the target<br /> extension depends on. So isn't it better to implement
thetool like yum, i.e.,<br /> which performs all those steps almost automatically, rather than the proposed<br />
feature?Maybe it's outside PostgreSQL core.<br /></blockquote><br /> That doesn't seem to make much sense to me.
Somethinglike yum can't<br /> install everything in all relevant databases. Sure, yum will be used to<br /> install
dependenciesbetween extensions on the filesystem level.<br /><br /> At the minimum I'd like to see that CREATE
EXTENSIONfoo; would install<br /> install extension 'bar' if foo dependended on 'bar' if CASCADE is<br /> specified.
Rightnow we always error out saying that the dependency on<br /> 'bar' is not fullfilled - not particularly helpful.<br
/></blockquote><br/> That's what the proposed patch does (with slightly different syntax but<br /> syntax is something
thatcan be changed easily).<br /></blockquote><br /></span> This seems quite reasonable, but I have to ask: How many
extensionsare there out there that depend on another extension? Off the top of my head, I can't think of any..<span
class=""></span><br/></blockquote></div><br /></div><div class="gmail_extra">With transforms there are such
dependencies,and there are 3 in contrib/: hstore_plperl, hstore_plpython and ltree_plpython.<br />-- <br /><div
class="gmail_signature">Michael<br/></div></div></div> 

On 2015-07-10 16:09:48 +0300, Heikki Linnakangas wrote:
> This seems quite reasonable, but I have to ask: How many extensions are
> there out there that depend on another extension? Off the top of my head, I
> can't think of any..

BDR/UDR is one (or two depending on your POV).

I think a part of why there are not more is that the dependencies right
now are painful.

Michael Paquier <michael.paquier@gmail.com> writes:
> On Fri, Jul 10, 2015 at 10:09 PM, Heikki Linnakangas <hlinnaka@iki.fi>
>> This seems quite reasonable, but I have to ask: How many extensions are
>> there out there that depend on another extension? Off the top of my head, I
>> can't think of any..

> With transforms there are such dependencies, and there are 3 in contrib/:
> hstore_plperl, hstore_plpython and ltree_plpython.

It's reasonable to expect that such cases will become more common as the
extension community matures.  It wasn't something we especially had to
worry about in the initial implementation of extensions, but it seems
totally worthwhile to me to add it now.

FWIW, I agree with using "CASCADE" to signal a request to create required
extension(s) automatically.
        regards, tom lane

On 2015-06-15 00:50:08 +0200, Petr Jelinek wrote:
> +                /* Create and execute new CREATE EXTENSION statement. */
> +                ces = makeNode(CreateExtensionStmt);
> +                ces->extname = curreq;
> +                ces->if_not_exists = false;
> +                parents = lappend(list_copy(recursive_parents), stmt->extname);
> +                ces->options = list_make1(makeDefElem("recursive",
> +                                                      (Node *) parents));
> +                CreateExtension(ces);

I think we should copy the SCHEMA option here and document that we use
the same schema. But it needs to be done in a way that doesn't error out
if the extension is not relocatable...

Andres Freund <andres@anarazel.de> writes:
> I think we should copy the SCHEMA option here and document that we use
> the same schema. But it needs to be done in a way that doesn't error out
> if the extension is not relocatable...

Would that propagate down through multiple levels of CASCADE?  (Although
I'm not sure it would be sensible for a non-relocatable extension to
depend on a relocatable one, so maybe the need doesn't arise in practice.)
        regards, tom lane

On July 10, 2015 4:16:59 PM GMT+02:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Andres Freund <andres@anarazel.de> writes:
>> I think we should copy the SCHEMA option here and document that we
>use
>> the same schema. But it needs to be done in a way that doesn't error
>out
>> if the extension is not relocatable...
>
>Would that propagate down through multiple levels of CASCADE? 
>(Although
>I'm not sure it would be sensible for a non-relocatable extension to
>depend on a relocatable one, so maybe the need doesn't arise in
>practice.)

I'd day so. I was thinking it'd adding a flag that allows to pass a schema to a non relocatable extension. That'd then
bepassed down. I agree that it's unlikely to be used often.
 

Andres

--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.

Andres Freund <andres@anarazel.de> writes:
> On July 10, 2015 4:16:59 PM GMT+02:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Would that propagate down through multiple levels of CASCADE?  (Although
>> I'm not sure it would be sensible for a non-relocatable extension to
>> depend on a relocatable one, so maybe the need doesn't arise in
>> practice.)

> I'd day so. I was thinking it'd adding a flag that allows to pass a
> schema to a non relocatable extension. That'd then be passed down. I
> agree that it's unlikely to be used often.

Yeah, I was visualizing it slightly differently, as a separate
internal-only option "schema_if_needed", but it works out to the
same thing either way.
        regards, tom lane

On Tue, Jul 07, 2015 at 10:14:49AM -0700, David E. Wheeler wrote:
> On Jul 7, 2015, at 6:41 AM, Andres Freund <andres@anarazel.de> wrote:
> 
> > At the minimum I'd like to see that CREATE EXTENSION foo; would
> > install install extension 'bar' if foo dependended on 'bar' if
> > CASCADE is specified. Right now we always error out saying that
> > the dependency on 'bar' is not fullfilled - not particularly
> > helpful.
> 
> +1
> 
> If `yum install foo` also installs bar, and `pgxn install foo`
> downloads, builds, and installs bar, it makes sense to me that
> `CREATE EXTENSION foo` would install bar if it was available, and
> complain if it wasn’t.

This is this baseline sane behavior.  Getting the full dependency
tree, although it would be very handy, would require more
infrastructure than we have now.

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Fri, Jul 10, 2015 at 11:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On July 10, 2015 4:16:59 PM GMT+02:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Would that propagate down through multiple levels of CASCADE?  (Although
>>> I'm not sure it would be sensible for a non-relocatable extension to
>>> depend on a relocatable one, so maybe the need doesn't arise in
>>> practice.)
>
>> I'd day so. I was thinking it'd adding a flag that allows to pass a
>> schema to a non relocatable extension. That'd then be passed down. I
>> agree that it's unlikely to be used often.
>
> Yeah, I was visualizing it slightly differently, as a separate
> internal-only option "schema_if_needed", but it works out to the
> same thing either way.

I just had a look at this patch, and here are some comments:
+             [ RECURSIVE ]
After reading the thread, CASCADE sounds like a good thing as well to me.

+                               /* Create and execute new CREATE
EXTENSION statement. */
+                               ces = makeNode(CreateExtensionStmt);
+                               ces->extname = curreq;
+                               ces->if_not_exists = false;
+                               parents =
lappend(list_copy(recursive_parents), stmt->extname);
+                               ces->options =
list_make1(makeDefElem("recursive",
+                                  (Node *) parents));
+                               CreateExtension(ces);
+                               list_free(parents);
ces should be free'd after calling CreateExtension perhaps?

The test_ext*--*.sql files should not be completely empty. They should
include a header like this one (hoge is the Japanese foo...):
/* src/test/modules/test_extension/hoge--1.0.sql */
-- complain if script is sourced in psql, rather than via CREATE EXTENSION
\echo Use "CREATE EXTENSION hoge" to load this file. \quit

That is good practice compared to the other modules, and this way
there is no need to have Makefile for example touch'ing those files
before installing them (I have created them manually to test this
patch).

The list of contrib modules excluded from build in the case of MSVC
needs to include test_extensions ($contrib_excludes in
src/tools/msvc/Mkvcbuild.pm) or build on Windows using MS of VC will
fail. commit_ts does that for example.

Regression tests of contrib/ modules doing transforms should be
updated to use this new stuff IMO. That's not part of the core patch
obviously, but it looks worth including them as well.
-- 
Michael

On 2015-07-15 06:07, Michael Paquier wrote:
> On Fri, Jul 10, 2015 at 11:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> On July 10, 2015 4:16:59 PM GMT+02:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> Would that propagate down through multiple levels of CASCADE?  (Although
>>>> I'm not sure it would be sensible for a non-relocatable extension to
>>>> depend on a relocatable one, so maybe the need doesn't arise in
>>>> practice.)
>>
>>> I'd day so. I was thinking it'd adding a flag that allows to pass a
>>> schema to a non relocatable extension. That'd then be passed down. I
>>> agree that it's unlikely to be used often.
>>
>> Yeah, I was visualizing it slightly differently, as a separate
>> internal-only option "schema_if_needed", but it works out to the
>> same thing either way.

I added DEFAULT SCHEMA option into CREATE EXTENSION which behaves like
SCHEMA but only for extension that don't specify their schema and is
mutually exclusive with just SCHEMA. The DEFAULT SCHEMA propagates when
CASCADE is used while the SCHEMA option does not propagate. I'd like to
hear opinions about this behavior. It would be possible to propagate
SCHEMA as DEFAULT SCHEMA but that might have surprising results
(installing dependencies in same schema as the current ext).

>
> I just had a look at this patch, and here are some comments:
> +             [ RECURSIVE ]
> After reading the thread, CASCADE sounds like a good thing as well to me.

Yep, got that much :)

>
> +                               /* Create and execute new CREATE
> EXTENSION statement. */
> +                               ces = makeNode(CreateExtensionStmt);
> +                               ces->extname = curreq;
> +                               ces->if_not_exists = false;
> +                               parents =
> lappend(list_copy(recursive_parents), stmt->extname);
> +                               ces->options =
> list_make1(makeDefElem("recursive",
> +
>                                     (Node *) parents));
> +                               CreateExtension(ces);
> +                               list_free(parents);
> ces should be free'd after calling CreateExtension perhaps?
>

Yeah and the exercise with list_copy and list_free on parents is
probably not needed.

> The test_ext*--*.sql files should not be completely empty. They should
> include a header like this one (hoge is the Japanese foo...):
> /* src/test/modules/test_extension/hoge--1.0.sql */
> -- complain if script is sourced in psql, rather than via CREATE EXTENSION
> \echo Use "CREATE EXTENSION hoge" to load this file. \quit
>

Done.


> The list of contrib modules excluded from build in the case of MSVC
> needs to include test_extensions ($contrib_excludes in
> src/tools/msvc/Mkvcbuild.pm) or build on Windows using MS of VC will
> fail. commit_ts does that for example.
>

Done, hopefully correctly, I don't have access to MSVC.

> Regression tests of contrib/ modules doing transforms should be
> updated to use this new stuff IMO. That's not part of the core patch
> obviously, but it looks worth including them as well.
>

Done.


--
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Jul 18, 2015 at 8:00 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:
> On 2015-07-15 06:07, Michael Paquier wrote:
>>
>> On Fri, Jul 10, 2015 at 11:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>
>>> Andres Freund <andres@anarazel.de> writes:
>>>>
>>>> On July 10, 2015 4:16:59 PM GMT+02:00, Tom Lane <tgl@sss.pgh.pa.us>
>>>> wrote:
>>>>>
>>>>> Would that propagate down through multiple levels of CASCADE?
>>>>> (Although
>>>>> I'm not sure it would be sensible for a non-relocatable extension to
>>>>> depend on a relocatable one, so maybe the need doesn't arise in
>>>>> practice.)
>>>
>>>
>>>> I'd day so. I was thinking it'd adding a flag that allows to pass a
>>>> schema to a non relocatable extension. That'd then be passed down. I
>>>> agree that it's unlikely to be used often.
>>>
>>>
>>> Yeah, I was visualizing it slightly differently, as a separate
>>> internal-only option "schema_if_needed", but it works out to the
>>> same thing either way.
>
>
> I added DEFAULT SCHEMA option into CREATE EXTENSION which behaves like
> SCHEMA but only for extension that don't specify their schema and is
> mutually exclusive with just SCHEMA. The DEFAULT SCHEMA propagates when
> CASCADE is used while the SCHEMA option does not propagate. I'd like to hear
> opinions about this behavior. It would be possible to propagate SCHEMA as
> DEFAULT SCHEMA but that might have surprising results (installing
> dependencies in same schema as the current ext).

Hm...

First, the current patch has a surprising behavior because it fails to
create an extension in cascade when creation is attempted on a custom
schema:
=# create schema po;
CREATE SCHEMA
=# create extension hstore_plperl with schema po cascade;
NOTICE:  00000: installing required extension "hstore"
LOCATION:  CreateExtension, extension.c:1483
NOTICE:  00000: installing required extension "plperl"
LOCATION:  CreateExtension, extension.c:1483
ERROR:  42704: type "hstore" does not exist
LOCATION:  typenameType, parse_type.c:258
Time: 30.071 ms
To facilitate the life of users, I think that the parent extensions
should be created on the same schema as its child by default. In this
case hstore should be created in po, not public.

And actually by looking at the code I can guess that when DEFAULT
SCHEMA is used and that a non-relocatable extension with a schema
defined is created in cascade this will error-out as well. That's not
really user-friendly..

Hence, as a schema can only be specified in a control file for a
non-relocatable extension, I think that the schema name propagated to
the root extensions should be the one specified in the control file of
the child if it is defined in it instead of the one specified by user
(imagine for example an extension created in cascade that requires
adminpack, extension that can only be deployed in pg_catalog), and
have the root node use its own schema if it has one in its control
file by default.

For example in this case:
foo1 (WITH SCHEMA hoge) -----> foo2 (schema = pg_catalog) -----> foo2_1                                               |
                             |--> foo2_2                                               ---> foo3 (no schema)
 
With this command:
CREATE EXTENSION foo1 WITH SCHEMA hoge;
foo3 is created on schema po. foo2, as well its required dependencies
foo2_1 and foo2_2 are created on pg_catalog.

Now DEFAULT SCHEMA is trying to achieve: "Hey, I want to define foo2_1
and foo2_2 on schema hoge". This may be worth achieving (though IMO I
think that foo1 should have direct dependencies with foo2_1 and
foo2_2), but I think that we should decide what is the default
behavior we want, and implement the additional behavior in a second
patch, separated from the patch of this thread. Personally I am more a
fan of propagating to parent extensions the schema of the child and
not of its grand-child by default, but I am not alone here :)

>> The list of contrib modules excluded from build in the case of MSVC
>> needs to include test_extensions ($contrib_excludes in
>> src/tools/msvc/Mkvcbuild.pm) or build on Windows using MS of VC will
>> fail. commit_ts does that for example.
>>
>
> Done, hopefully correctly, I don't have access to MSVC.

That's done correctly.

+                               /*
+                                * Propagate the CASCADE option and
add current extenssion
+                                * to the list for checking the cyclic
dependencies.
+                                */
s/extenssion/extension/

+                               /* Check for cyclic dependency between
extension. */
s/extension/extensions/

psql tab completion should be completed with cascade. See the part
with WITH SCHEMA in tab-complete.c.

Regards,
-- 
Michael

On 2015-07-19 17:16, Michael Paquier wrote:
> On Sat, Jul 18, 2015 at 8:00 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:
>> On 2015-07-15 06:07, Michael Paquier wrote:
>>>
>>> On Fri, Jul 10, 2015 at 11:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>>
>>>> Andres Freund <andres@anarazel.de> writes:
>>>>>
>>>>> On July 10, 2015 4:16:59 PM GMT+02:00, Tom Lane <tgl@sss.pgh.pa.us>
>>>>> wrote:
>>>>>>
>>>>>> Would that propagate down through multiple levels of CASCADE?
>>>>>> (Although
>>>>>> I'm not sure it would be sensible for a non-relocatable extension to
>>>>>> depend on a relocatable one, so maybe the need doesn't arise in
>>>>>> practice.)
>>>>
>>>>
>>>>> I'd day so. I was thinking it'd adding a flag that allows to pass a
>>>>> schema to a non relocatable extension. That'd then be passed down. I
>>>>> agree that it's unlikely to be used often.
>>>>
>>>>
>>>> Yeah, I was visualizing it slightly differently, as a separate
>>>> internal-only option "schema_if_needed", but it works out to the
>>>> same thing either way.
>>
>>
>> I added DEFAULT SCHEMA option into CREATE EXTENSION which behaves like
>> SCHEMA but only for extension that don't specify their schema and is
>> mutually exclusive with just SCHEMA. The DEFAULT SCHEMA propagates when
>> CASCADE is used while the SCHEMA option does not propagate. I'd like to hear
>> opinions about this behavior. It would be possible to propagate SCHEMA as
>> DEFAULT SCHEMA but that might have surprising results (installing
>> dependencies in same schema as the current ext).
>
> Hm...
>
> First, the current patch has a surprising behavior because it fails to
> create an extension in cascade when creation is attempted on a custom
> schema:
> =# create schema po;
> CREATE SCHEMA
> =# create extension hstore_plperl with schema po cascade;
> NOTICE:  00000: installing required extension "hstore"
> LOCATION:  CreateExtension, extension.c:1483
> NOTICE:  00000: installing required extension "plperl"
> LOCATION:  CreateExtension, extension.c:1483
> ERROR:  42704: type "hstore" does not exist
> LOCATION:  typenameType, parse_type.c:258
> Time: 30.071 ms
> To facilitate the life of users, I think that the parent extensions
> should be created on the same schema as its child by default. In this
> case hstore should be created in po, not public.
>

That's actually a bug because the previous version of the patch did not
set the reqext correctly after creating the required extension.

>
> Hence, as a schema can only be specified in a control file for a
> non-relocatable extension, I think that the schema name propagated to
> the root extensions should be the one specified in the control file of
> the child if it is defined in it instead of the one specified by user
> (imagine for example an extension created in cascade that requires
> adminpack, extension that can only be deployed in pg_catalog), and
> have the root node use its own schema if it has one in its control
> file by default.
>
> For example in this case:
> foo1 (WITH SCHEMA hoge) -----> foo2 (schema = pg_catalog) -----> foo2_1
>                                                  |
>                                  |--> foo2_2
>                                                  ---> foo3 (no schema)
> With this command:
> CREATE EXTENSION foo1 WITH SCHEMA hoge;
> foo3 is created on schema po. foo2, as well its required dependencies
> foo2_1 and foo2_2 are created on pg_catalog.
>
> Now DEFAULT SCHEMA is trying to achieve: "Hey, I want to define foo2_1
> and foo2_2 on schema hoge". This may be worth achieving (though IMO I
> think that foo1 should have direct dependencies with foo2_1 and
> foo2_2), but I think that we should decide what is the default
> behavior we want, and implement the additional behavior in a second
> patch, separated from the patch of this thread. Personally I am more a
> fan of propagating to parent extensions the schema of the child and
> not of its grand-child by default, but I am not alone here :)
>

This is something that I as a user of the feature specifically don't
want to happen as I install extensions either to common schema (for some
utility ones) or into separate schemas (for the rest), but I never want
the utility extension to go to the same schema as the other ones. This
is especially true when installing non-relocatable extension which
depends on relocatable one. Obviously, it does not mean that nobody else
wants this though :)

--
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Jul 20, 2015 at 10:20 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:
> On 2015-07-19 17:16, Michael Paquier wrote:
>>
>> On Sat, Jul 18, 2015 at 8:00 AM, Petr Jelinek <petr@2ndquadrant.com>
>> wrote:
>>>
>>> On 2015-07-15 06:07, Michael Paquier wrote:
>>>>
>>>>
>>>> On Fri, Jul 10, 2015 at 11:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>>>
>>>>>
>>>>> Andres Freund <andres@anarazel.de> writes:
>>>>>>
>>>>>>
>>>>>> On July 10, 2015 4:16:59 PM GMT+02:00, Tom Lane <tgl@sss.pgh.pa.us>
>>>>>>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> Would that propagate down through multiple levels of CASCADE?
>>>>>>> (Although
>>>>>>> I'm not sure it would be sensible for a non-relocatable extension to
>>>>>>> depend on a relocatable one, so maybe the need doesn't arise in
>>>>>>> practice.)
>>>>>
>>>>>
>>>>>
>>>>>> I'd day so. I was thinking it'd adding a flag that allows to pass a
>>>>>> schema to a non relocatable extension. That'd then be passed down. I
>>>>>> agree that it's unlikely to be used often.
>>>>>
>>>>>
>>>>>
>>>>> Yeah, I was visualizing it slightly differently, as a separate
>>>>> internal-only option "schema_if_needed", but it works out to the
>>>>> same thing either way.
>>>
>>>
>>>
>>> I added DEFAULT SCHEMA option into CREATE EXTENSION which behaves like
>>> SCHEMA but only for extension that don't specify their schema and is
>>> mutually exclusive with just SCHEMA. The DEFAULT SCHEMA propagates when
>>> CASCADE is used while the SCHEMA option does not propagate. I'd like to
>>> hear
>>> opinions about this behavior. It would be possible to propagate SCHEMA as
>>> DEFAULT SCHEMA but that might have surprising results (installing
>>> dependencies in same schema as the current ext).
>>
>>
>> Hm...
>>
>> First, the current patch has a surprising behavior because it fails to
>> create an extension in cascade when creation is attempted on a custom
>> schema:
>> =# create schema po;
>> CREATE SCHEMA
>> =# create extension hstore_plperl with schema po cascade;
>> NOTICE:  00000: installing required extension "hstore"
>> LOCATION:  CreateExtension, extension.c:1483
>> NOTICE:  00000: installing required extension "plperl"
>> LOCATION:  CreateExtension, extension.c:1483
>> ERROR:  42704: type "hstore" does not exist
>> LOCATION:  typenameType, parse_type.c:258
>> Time: 30.071 ms
>> To facilitate the life of users, I think that the parent extensions
>> should be created on the same schema as its child by default. In this
>> case hstore should be created in po, not public.
>>
>
> That's actually a bug because the previous version of the patch did not set
> the reqext correctly after creating the required extension.
>
>>
>> Hence, as a schema can only be specified in a control file for a
>> non-relocatable extension, I think that the schema name propagated to
>> the root extensions should be the one specified in the control file of
>> the child if it is defined in it instead of the one specified by user
>> (imagine for example an extension created in cascade that requires
>> adminpack, extension that can only be deployed in pg_catalog), and
>> have the root node use its own schema if it has one in its control
>> file by default.
>>
>> For example in this case:
>> foo1 (WITH SCHEMA hoge) -----> foo2 (schema = pg_catalog) -----> foo2_1
>>                                                  |
>>                                  |--> foo2_2
>>                                                  ---> foo3 (no schema)
>> With this command:
>> CREATE EXTENSION foo1 WITH SCHEMA hoge;
>> foo3 is created on schema po. foo2, as well its required dependencies
>> foo2_1 and foo2_2 are created on pg_catalog.
>>
>> Now DEFAULT SCHEMA is trying to achieve: "Hey, I want to define foo2_1
>> and foo2_2 on schema hoge". This may be worth achieving (though IMO I
>> think that foo1 should have direct dependencies with foo2_1 and
>> foo2_2), but I think that we should decide what is the default
>> behavior we want, and implement the additional behavior in a second
>> patch, separated from the patch of this thread. Personally I am more a
>> fan of propagating to parent extensions the schema of the child and
>> not of its grand-child by default, but I am not alone here :)
>>
>
> This is something that I as a user of the feature specifically don't want to
> happen as I install extensions either to common schema (for some utility
> ones) or into separate schemas (for the rest), but I never want the utility
> extension to go to the same schema as the other ones. This is especially
> true when installing non-relocatable extension which depends on relocatable
> one. Obviously, it does not mean that nobody else wants this though :)

So, in your patch the default behavior is to create parent extensions
on schema public if the child extension created specifies a schema:
=# create schema po;
CREATE SCHEMA
=# create extension test_ext2 cascade schema po;
CREATE EXTENSION
=# \dx  test_ext*         List of installed extensions  Name    | Version | Schema |   Description
-----------+---------+--------+------------------test_ext2 | 1.0     | po     | Test extension 2test_ext3 | 1.0     |
public| Test extension 3
 
(2 rows)
=# drop extension test_ext3 cascade;
NOTICE:  00000: drop cascades to extension test_ext2
LOCATION:  reportDependentObjects, dependency.c:1009
DROP EXTENSION
=# create extension test_ext2 cascade default schema po;
NOTICE:  00000: installing required extension "test_ext3"
LOCATION:  CreateExtension, extension.c:1484
CREATE EXTENSION
Time: 1.578 ms
=# \dx  test_ext*         List of installed extensions  Name    | Version | Schema |   Description
-----------+---------+--------+------------------test_ext2 | 1.0     | po     | Test extension 2test_ext3 | 1.0     |
po    | Test extension 3
 
(2 rows)

And if a non-relocatable extension with a schema has parent extensions
they get created by default in public as well:
=# create extension test_ext1 cascade;
NOTICE:  00000: installing required extension "test_ext2"
LOCATION:  CreateExtension, extension.c:1484
NOTICE:  00000: installing required extension "test_ext3"
LOCATION:  CreateExtension, extension.c:1484
CREATE EXTENSION
=# \dx  test_ext*           List of installed extensions  Name    | Version |  Schema   |   Description
-----------+---------+-----------+------------------test_ext1 | 1.0     | test_ext1 | Test extension 1test_ext2 | 1.0
 | public    | Test extension 2test_ext3 | 1.0     | public    | Test extension 3
 
(3 rows)

Both positions are debatable, but I'd rather take the opposite
approach and pass the schema of the child extension to its parents as
I imagine that in most cases a child extension would rely on the
objects of its parents to be localized on the same schema it uses.
Imagine for example cases where search_path is *not* set to public for
example. In short I would give up on the DEFAULT SCHEMA business, and
add a new flag in the control file to decide if a given extension
passes down the schema name of its child when created in cascade,
default being true for the potential issues with search_path not
pointing to public. Thoughts about that from other folks are welcome.

In order to make things clear in your patch, could you as well add the
following tests to makes clear the whole process?
create extension test_ext1 cascade; -- no default schema specified
create extension test_ext2 cascade schema hoge;
create extension test_ext2 cascade default schema hoge;
Whatever the final choice choice of default behavior made, either
passing the schema of the child extension to its parents or create the
child extensions in public by default, we want to tests all the
combinations with even relocatable extensions and their parents. I am
also noticing that there is no test with a non-relocatable as a parent
extension, you could for example add test_ext4 which is
non-relocatable with no schema, and test_ext5 which is non-relocatable
with a default schema, both bring required by test_ext3.

+++ b/src/test/modules/test_extensions/test_ext1.control
@@ -0,0 +1,4 @@
+comment = 'Test extension 1'
+default_version = '1.0'
+schema = 'test_ext1'
+requires = 'test_ext2'
Could you also add relocatable = false here? I know that this is the
default value but it would be better to add it clearly IMO, it makes
the test easier to understand.

Something else that I missed previously... Wouldn't CASCADE be better
placed at the end of the query? Other DDL commands using it put it at
the end, with RESTRICT as well.
Regards,
-- 
Michael

On Mon, Jul 20, 2015 at 10:29 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:> In > short I would give up on the
DEFAULT SCHEMA business, and
> add a new flag in the control file to decide if a given extension
> passes down the schema name of its child when created in cascade,
> default being true for the potential issues with search_path not
> pointing to public.

Well, so far, it seems like this decision is something where different
DBAs might have different policies.  If you put the flag in the
control file, you're saying it is the extension developer's decision,
which may not be best.

Maybe I'm confused.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jul 20, 2015 at 10:29 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:>
>> In short I would give up on the DEFAULT SCHEMA business, and
>> add a new flag in the control file to decide if a given extension
>> passes down the schema name of its child when created in cascade,
>> default being true for the potential issues with search_path not
>> pointing to public.

> Well, so far, it seems like this decision is something where different
> DBAs might have different policies.  If you put the flag in the
> control file, you're saying it is the extension developer's decision,
> which may not be best.

I have doubts about that too.  But really, why have a flag at all
anywhere?  If we are doing a CASCADE, and the referenced extension needs a
schema, the alternatives are either (1) let it have one, or (2) fail.
I am not seeing that (2) is a superior alternative in any circumstances.

We will need to document that the behavior of CASCADE is "install all
needed extensions into the schema you specify", but what's wrong with
that?  If the user wants to put them in different schemas, he cannot
use CASCADE in any case.
        regards, tom lane

On 2015-07-21 15:48, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Jul 20, 2015 at 10:29 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:>
>>> In short I would give up on the DEFAULT SCHEMA business, and
>>> add a new flag in the control file to decide if a given extension
>>> passes down the schema name of its child when created in cascade,
>>> default being true for the potential issues with search_path not
>>> pointing to public.
>
>> Well, so far, it seems like this decision is something where different
>> DBAs might have different policies.  If you put the flag in the
>> control file, you're saying it is the extension developer's decision,
>> which may not be best.
>
> I have doubts about that too.  But really, why have a flag at all
> anywhere?  If we are doing a CASCADE, and the referenced extension needs a
> schema, the alternatives are either (1) let it have one, or (2) fail.
> I am not seeing that (2) is a superior alternative in any circumstances.
>
> We will need to document that the behavior of CASCADE is "install all
> needed extensions into the schema you specify", but what's wrong with
> that?  If the user wants to put them in different schemas, he cannot
> use CASCADE in any case.
>

Yes this is the behavior I want as well. My main question is if we are 
ok with SCHEMA having different behavior with CASCADE vs without 
CASCADE. I went originally with "no" and added the DEFAULT flag to 
SCHEMA. If the answer is "yes" then we don't need the flag (in that case 
CASCADE acts as the flag).

Or course "yes" would then mean "CREATE EXTENSION foo SCHEMA bar" will 
fail if "foo" is not relocatable but "CREATE EXTENSION foo SCHEMA bar 
CASCADE" will succeed and install "foo" into schema "foo" instead of 
"bar" and only relocatable dependencies will go to "bar". OTOH 
non-relocatable dependencies will go to their own schema no matter what 
user specifies in the command so I guess it's ok to just document that 
this is the behavior of CASCADE. As you say if somebody wants control 
over each individual extension they can't use CASCADE anyway.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

Petr Jelinek <petr@2ndquadrant.com> writes:
> ... My main question is if we are 
> ok with SCHEMA having different behavior with CASCADE vs without 
> CASCADE. I went originally with "no" and added the DEFAULT flag to 
> SCHEMA. If the answer is "yes" then we don't need the flag (in that case 
> CASCADE acts as the flag).

Yeah, I was coming around to that position as well.  Insisting that
SCHEMA throw an error if the extension isn't relocatable makes sense
as long as only one extension is being considered, but once you say
CASCADE it seems like mostly a usability fail.  I think it's probably
OK if with CASCADE, SCHEMA is just "use if needed else ignore".

Obviously we've gotta document all this properly.
        regards, tom lane

On Tue, Jul 21, 2015 at 11:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Petr Jelinek <petr@2ndquadrant.com> writes:
>> ... My main question is if we are
>> ok with SCHEMA having different behavior with CASCADE vs without
>> CASCADE. I went originally with "no" and added the DEFAULT flag to
>> SCHEMA. If the answer is "yes" then we don't need the flag (in that case
>> CASCADE acts as the flag).
>
> Yeah, I was coming around to that position as well.  Insisting that
> SCHEMA throw an error if the extension isn't relocatable makes sense
> as long as only one extension is being considered, but once you say
> CASCADE it seems like mostly a usability fail.  I think it's probably
> OK if with CASCADE, SCHEMA is just "use if needed else ignore".

OK, I'm fine with that, aka with CASCADE and a SCHEMA specified we use
it if needed or ignore it otherwise (if I am following correctly).

"CREATE EXTENSION foo SCHEMA bar" will fail if the extension is not
relocatable *and* does not have a schema specified in its control
file. A non-relocatable extension can be initially created anywhere.
It just cannot be moved afterwards from its original schema.

> Obviously we've gotta document all this properly.

Sure. That's a sine-qua-non condition for this patch.
Regards,
-- 
Michael

On 2015-07-22 07:12, Michael Paquier wrote:
> On Tue, Jul 21, 2015 at 11:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Petr Jelinek <petr@2ndquadrant.com> writes:
>>> ... My main question is if we are
>>> ok with SCHEMA having different behavior with CASCADE vs without
>>> CASCADE. I went originally with "no" and added the DEFAULT flag to
>>> SCHEMA. If the answer is "yes" then we don't need the flag (in that case
>>> CASCADE acts as the flag).
>>
>> Yeah, I was coming around to that position as well.  Insisting that
>> SCHEMA throw an error if the extension isn't relocatable makes sense
>> as long as only one extension is being considered, but once you say
>> CASCADE it seems like mostly a usability fail.  I think it's probably
>> OK if with CASCADE, SCHEMA is just "use if needed else ignore".
>

Here is a patch implementing that. Note that the checks are now done in
different order for non-relocatable extension when SCHEMA is specified
than previously. Before the patch, the SCHEMA was first checked for
conflict with the extension's schema and then there was check for schema
existence. This patch always checks for schema existence first, mainly
to keep code saner (to my eyes).

--
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Jul 25, 2015 at 12:59 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:
> On 2015-07-22 07:12, Michael Paquier wrote:
>>
>> On Tue, Jul 21, 2015 at 11:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>
>>> Petr Jelinek <petr@2ndquadrant.com> writes:
>>>>
>>>> ... My main question is if we are
>>>> ok with SCHEMA having different behavior with CASCADE vs without
>>>> CASCADE. I went originally with "no" and added the DEFAULT flag to
>>>> SCHEMA. If the answer is "yes" then we don't need the flag (in that case
>>>> CASCADE acts as the flag).
>>>
>>>
>>> Yeah, I was coming around to that position as well.  Insisting that
>>> SCHEMA throw an error if the extension isn't relocatable makes sense
>>> as long as only one extension is being considered, but once you say
>>> CASCADE it seems like mostly a usability fail.  I think it's probably
>>> OK if with CASCADE, SCHEMA is just "use if needed else ignore".
>>
>>
>
> Here is a patch implementing that. Note that the checks are now done in
> different order for non-relocatable extension when SCHEMA is specified than
> previously. Before the patch, the SCHEMA was first checked for conflict with
> the extension's schema and then there was check for schema existence. This
> patch always checks for schema existence first, mainly to keep code saner
> (to my eyes).

+        In case the extension specifies schema in its control file, the schema
+        can't be overriden using <literal>SCHEMA</> parameter. The actual
+        behavior of the <literal>SCHEMA</> parameter in this case will depend
+        on circumstances:
SCHEMA is not a parameter, it is a clause. Same for CASCADE. Also
"schema" here should use the markup structfield as it is referenced as
the parameter of a control file.

+          <para>
+           If schema is not same as the one in extension's control file and
+           the <literal>CASCADE</> parameter is not given, error will be
+           thrown.
+          </para>
"If schema is not the same". Here I think that you may directly refer
to schema_name...

-               /* If the user is giving us the schema name, it must
exist already */
+               /* If the user is giving us the schema name, it must
exist already. */
Noise?

+# test_ddl_deparse must be first
+REGRESS = test_extensions
Why is test_ddl_deparse mentioned here?

With the patch:
=# create extension adminpack schema popo2;
ERROR:  3F000: schema "popo2" does not exist
LOCATION:  get_namespace_oid, namespace.c:2873
On HEAD:
=# create extension adminpack schema popo2;
ERROR:  0A000: extension "adminpack" must be installed in schema "pg_catalog"
LOCATION:  CreateExtension, extension.c:1352
Time: 0.978 ms
It looks like a regression to me to check for the existence of the
schema before checking if the extension is compatible with the options
given by users (regression test welcome).
-- 
Michael

On 2015-07-25 14:37, Michael Paquier wrote:
> On Sat, Jul 25, 2015 at 12:59 AM, Petr Jelinek <petr@2ndquadrant.com> wrote:
>> On 2015-07-22 07:12, Michael Paquier wrote:
>>>
>>> On Tue, Jul 21, 2015 at 11:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>>
>>>> Petr Jelinek <petr@2ndquadrant.com> writes:
>>>>>
>>>>> ... My main question is if we are
>>>>> ok with SCHEMA having different behavior with CASCADE vs without
>>>>> CASCADE. I went originally with "no" and added the DEFAULT flag to
>>>>> SCHEMA. If the answer is "yes" then we don't need the flag (in that case
>>>>> CASCADE acts as the flag).
>>>>
>>>>
>>>> Yeah, I was coming around to that position as well.  Insisting that
>>>> SCHEMA throw an error if the extension isn't relocatable makes sense
>>>> as long as only one extension is being considered, but once you say
>>>> CASCADE it seems like mostly a usability fail.  I think it's probably
>>>> OK if with CASCADE, SCHEMA is just "use if needed else ignore".
>>>
>>>
>>
>> Here is a patch implementing that...
>
> +          <para>
> +           If schema is not same as the one in extension's control file and
> +           the <literal>CASCADE</> parameter is not given, error will be
> +           thrown.
> +          </para>
> "If schema is not the same". Here I think that you may directly refer
> to schema_name...
>
> -               /* If the user is giving us the schema name, it must
> exist already */
> +               /* If the user is giving us the schema name, it must
> exist already. */
> Noise?

Intentional. Any back-patching there will be anyway complicated by the
change of the code couple lines bellow and above so I think it's OK to
fix the comment even if just cosmetically.

>
> With the patch:
> =# create extension adminpack schema popo2;
> ERROR:  3F000: schema "popo2" does not exist
> LOCATION:  get_namespace_oid, namespace.c:2873
> On HEAD:
> =# create extension adminpack schema popo2;
> ERROR:  0A000: extension "adminpack" must be installed in schema "pg_catalog"
> LOCATION:  CreateExtension, extension.c:1352
> Time: 0.978 ms
> It looks like a regression to me to check for the existence of the
> schema before checking if the extension is compatible with the options
> given by users (regression test welcome).
>

Yes that's what I meant by the change of checking order in the
explanation above. I did that because I thought code would be more
complicated otherwise, but apparently I was stupid...

--
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Jul 26, 2015 at 1:01 AM, Petr Jelinek wrote:
> Yes that's what I meant by the change of checking order in the explanation
> above. I did that because I thought code would be more complicated
> otherwise, but apparently I was stupid...

+        In case the extension specifies schema in its control file, the schema
s/schema/<literal>schema</>/

+++ b/src/test/modules/test_extensions/.gitignore
@@ -0,0 +1,3 @@
+# Generated subdirectories
+/results/
+/tmp_check/
test_extensions/.gitignore is missing /log/.

Something also has not been discussed yet: what to do with new_version
and old_version (the options of CreateExtensionStmt)? As of now if
those options are defined they are not passed down to the parent
extensions but shouldn't we raise an error if they are used in
combination with CASCADE? In any case, I think that the behavior
chosen should be documented.
-- 
Michael

On 2015-07-27 15:18, Michael Paquier wrote:
> On Sun, Jul 26, 2015 at 1:01 AM, Petr Jelinek wrote:
>> Yes that's what I meant by the change of checking order in the explanation
>> above. I did that because I thought code would be more complicated
>> otherwise, but apparently I was stupid...
>
> +        In case the extension specifies schema in its control file, the schema
> s/schema/<literal>schema</>/
>
> +++ b/src/test/modules/test_extensions/.gitignore
> @@ -0,0 +1,3 @@
> +# Generated subdirectories
> +/results/
> +/tmp_check/
> test_extensions/.gitignore is missing /log/.
>
> Something also has not been discussed yet: what to do with new_version
> and old_version (the options of CreateExtensionStmt)? As of now if
> those options are defined they are not passed down to the parent
> extensions but shouldn't we raise an error if they are used in
> combination with CASCADE? In any case, I think that the behavior
> chosen should be documented.
>

I don't see why we should raise error, they are used just for the 
top-level extension and I think it makes sense that way. CASCADE 
documentation says SCHEMA option is cascaded to required extensions, do 
we need to say something more than that (ie explicitly saying that the 
old_version and new_version aren't)?

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On Thu, Jul 30, 2015 at 10:58 PM, Petr Jelinek wrote:
> On 2015-07-27 15:18, Michael Paquier wrote:
>> Something also has not been discussed yet: what to do with new_version
>> and old_version (the options of CreateExtensionStmt)? As of now if
>> those options are defined they are not passed down to the parent
>> extensions but shouldn't we raise an error if they are used in
>> combination with CASCADE? In any case, I think that the behavior
>> chosen should be documented.
>>
>
> I don't see why we should raise error, they are used just for the top-level
> extension and I think it makes sense that way. CASCADE documentation says
> SCHEMA option is cascaded to required extensions, do we need to say
> something more than that (ie explicitly saying that the old_version and
> new_version aren't)?

OK, let's do so then. I think that we should still document the fact
that the old and new version strings and not passed to the parent
extensions when cascade is used for clarity. Something like:
"Other options are not recursively applied when the CASCASE clause is used."

I have been through this patch one last time and changed the following:
- Improved documentation: missing markups with <literal>, SCHEMA is a
clause and not a parameter, added explanation that options other than
SCHEMA are not applied recursively with CASCADE
- Corrected .gitignore in test_extensions, log/ was missing.
Those are minor things though, hence I just switched patch as "Ready
for committer".
--
Michael

On 2015-07-31 03:03, Michael Paquier wrote:
> On Thu, Jul 30, 2015 at 10:58 PM, Petr Jelinek wrote:
>> On 2015-07-27 15:18, Michael Paquier wrote:
>>> Something also has not been discussed yet: what to do with new_version
>>> and old_version (the options of CreateExtensionStmt)? As of now if
>>> those options are defined they are not passed down to the parent
>>> extensions but shouldn't we raise an error if they are used in
>>> combination with CASCADE? In any case, I think that the behavior
>>> chosen should be documented.
>>>
>>
>> I don't see why we should raise error, they are used just for the top-level
>> extension and I think it makes sense that way. CASCADE documentation says
>> SCHEMA option is cascaded to required extensions, do we need to say
>> something more than that (ie explicitly saying that the old_version and
>> new_version aren't)?
>
> OK, let's do so then. I think that we should still document the fact
> that the old and new version strings and not passed to the parent
> extensions when cascade is used for clarity. Something like:
> "Other options are not recursively applied when the CASCASE clause is used."
>
> I have been through this patch one last time and changed the following:
> - Improved documentation: missing markups with <literal>, SCHEMA is a
> clause and not a parameter, added explanation that options other than
> SCHEMA are not applied recursively with CASCADE
> - Corrected .gitignore in test_extensions, log/ was missing.
> Those are minor things though, hence I just switched patch as "Ready
> for committer".
>

Yeah I agree with those changes, thanks for the review.


--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

Hi,

I'm looking at committing this patch. I found some nitpick-level things
that I can easily fixup. But I dislike two things:

1) Passing the list of parents through the cascade DefElem strikes me as
incredibly ugly.

For one the cascade option really should take a true/false type option
on the C level (so you can do defGetBoolean()), for another passing
through the list of parents via DefElem->arg seems wrong. You're
supposed to be able to copy parsenodes and at the very least that's
broken by the approach.

2) I don't like the control flow around the schema selection.

It seems to be getting a bit arcane. How about instead moving the
"extension \"%s\" must be installed in schema \"%s\" check into the if
(control->schema != NULL) block and check for d_schema after it? That
should look cleaner.

Greetings,

Andres Freund

On 2015-09-02 17:27:38 +0200, Andres Freund wrote:
> 1) Passing the list of parents through the cascade DefElem strikes me as
> incredibly ugly.
> 
> For one the cascade option really should take a true/false type option
> on the C level (so you can do defGetBoolean()), for another passing
> through the list of parents via DefElem->arg seems wrong. You're
> supposed to be able to copy parsenodes and at the very least that's
> broken by the approach.

I think the fix here is to split off the bulk of CreateExtension() into
an internal function that takes additional parameters.

On 2015-09-02 17:31, Andres Freund wrote:
> On 2015-09-02 17:27:38 +0200, Andres Freund wrote:
>> 1) Passing the list of parents through the cascade DefElem strikes me as
>> incredibly ugly.
>>
>> For one the cascade option really should take a true/false type option
>> on the C level (so you can do defGetBoolean()), for another passing
>> through the list of parents via DefElem->arg seems wrong. You're
>> supposed to be able to copy parsenodes and at the very least that's
>> broken by the approach.
>
> I think the fix here is to split off the bulk of CreateExtension() into
> an internal function that takes additional parameters.
>

Yes that sounds cleaner. Just as a side note, List is a Node and does 
have copy support (and we pass List as DefElem->arg from gram.y in 
several places).
> 2) I don't like the control flow around the schema selection.>> It seems to be getting a bit arcane. How about
insteadmoving the> "extension \"%s\" must be installed in schema \"%s\" check into the if> (control->schema != NULL)
blockand check for d_schema after it? That> should look cleaner.>
 

I did something like that in one of the revisions, the complaint there 
was that it changes order of errors you get in situation when the schema 
is not the same as the one in control file and it also does not exist.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On 2015-09-07 20:56:50 +0200, Petr Jelinek wrote:
> Yes that sounds cleaner. Just as a side note, List is a Node and does have
> copy support (and we pass List as DefElem->arg from gram.y in several
> places).

I know - but the list element in this case don't have copy support, no?
You seem to have put plain C strings in there, right?

> > 2) I don't like the control flow around the schema selection.
> >
> > It seems to be getting a bit arcane. How about instead moving the
> > "extension \"%s\" must be installed in schema \"%s\" check into the if
> > (control->schema != NULL) block and check for d_schema after it? That
> > should look cleaner.
> >
> 
> I did something like that in one of the revisions, the complaint there was
> that it changes order of errors you get in situation when the schema is not
> the same as the one in control file and it also does not exist.

So what? That seems like a pretty harmless change - it's not like this
is something being hit day in/out right now.

Andres

Andres Freund wrote:
> On 2015-09-07 20:56:50 +0200, Petr Jelinek wrote:
> > Yes that sounds cleaner. Just as a side note, List is a Node and does have
> > copy support (and we pass List as DefElem->arg from gram.y in several
> > places).
> 
> I know - but the list element in this case don't have copy support, no?
> You seem to have put plain C strings in there, right?

Seems slightly easier to use makeString(), no?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2015-09-07 16:09:27 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> > On 2015-09-07 20:56:50 +0200, Petr Jelinek wrote:
> > > Yes that sounds cleaner. Just as a side note, List is a Node and does have
> > > copy support (and we pass List as DefElem->arg from gram.y in several
> > > places).
> > 
> > I know - but the list element in this case don't have copy support, no?
> > You seem to have put plain C strings in there, right?
> 
> Seems slightly easier to use makeString(), no?

It'd still be a god forsakenly ugly API to use DefElems for this. Imo
not being able to specify a boolean argument for a boolean value pretty
much hints at it being the wrong approach. I don't see any reason to go
that way rather than split the 'cycle detection' state into an argument
to CreateExtensionInternal() or something.

Greetings,

Andres Freund

On 2015-09-07 21:09, Alvaro Herrera wrote:
> Andres Freund wrote:
>> On 2015-09-07 20:56:50 +0200, Petr Jelinek wrote:
>>> Yes that sounds cleaner. Just as a side note, List is a Node and does have
>>> copy support (and we pass List as DefElem->arg from gram.y in several
>>> places).
>>
>> I know - but the list element in this case don't have copy support, no?
>> You seem to have put plain C strings in there, right?
>
> Seems slightly easier to use makeString(), no?
>

Yes, but I think Andres is correct when saying DefElem->arg is not 
nicest place to put it to.

Looking at the code again, splitting the function is actually not that 
easy since the cascaded extension creation has to execute all the same 
code/checks as CreateExtension does; It might be better to just add the 
parameter to the CreateExtension and call it with NIL value from utility.c.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On 2015-09-07 21:28, Petr Jelinek wrote:
> On 2015-09-07 21:09, Alvaro Herrera wrote:
>> Andres Freund wrote:
>>> On 2015-09-07 20:56:50 +0200, Petr Jelinek wrote:
>>>> Yes that sounds cleaner. Just as a side note, List is a Node and
>>>> does have
>>>> copy support (and we pass List as DefElem->arg from gram.y in several
>>>> places).
>>>
>>> I know - but the list element in this case don't have copy support, no?
>>> You seem to have put plain C strings in there, right?
>>
>> Seems slightly easier to use makeString(), no?
>>
>
> Yes, but I think Andres is correct when saying DefElem->arg is not
> nicest place to put it to.
>

Attached patch uses just boolean in cascade DefElem and splits the
CreateExtension into two functions, the cascade code now calls the
CreateExtensionInternal. One thing though - I am passing the DefElems
directly to the cascaded CreateExtensionStmt options, I think it's not
problem but want to give it extra visibility.

Also the schema check was moved.

--
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Sep 8, 2015 at 6:14 AM, Petr Jelinek wrote:
> Attached patch uses just boolean in cascade DefElem and splits the
> CreateExtension into two functions, the cascade code now calls the
> CreateExtensionInternal. One thing though - I am passing the DefElems
> directly to the cascaded CreateExtensionStmt options, I think it's not
> problem but want to give it extra visibility.
>
> Also the schema check was moved.

OK, passing the list of extensions through the new routine is indeed a
cleaner approach. One point of detail is that instead of doing this
part:
+       /* Handle the CASCADE option. */
+       if (d_cascade)
+               cascade = defGetBoolean(d_cascade);
+       else
+               cascade = false;
You may as well just initialize cascade to false at the beginning of
the routine and update it only if d_cascade is defined.

Attached are as well changes for the documentation that I spotted in
earlier reviews but were not included in the last version sent by Petr
yesterday. Feel free to discard them if you think they are not
adapted, the patch attached applies on top of Petr's patch.
Regards,
--
Michael

On Tue, Sep 8, 2015 at 10:44 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Sep 8, 2015 at 6:14 AM, Petr Jelinek wrote:
>> Attached patch uses just boolean in cascade DefElem and splits the
>> CreateExtension into two functions, the cascade code now calls the
>> CreateExtensionInternal. One thing though - I am passing the DefElems
>> directly to the cascaded CreateExtensionStmt options, I think it's not
>> problem but want to give it extra visibility.
>>
>> Also the schema check was moved.
>
> OK, passing the list of extensions through the new routine is indeed a
> cleaner approach. One point of detail is that instead of doing this
> part:
> +       /* Handle the CASCADE option. */
> +       if (d_cascade)
> +               cascade = defGetBoolean(d_cascade);
> +       else
> +               cascade = false;
> You may as well just initialize cascade to false at the beginning of
> the routine and update it only if d_cascade is defined.
>
> Attached are as well changes for the documentation that I spotted in
> earlier reviews but were not included in the last version sent by Petr
> yesterday. Feel free to discard them if you think they are not
> adapted, the patch attached applies on top of Petr's patch.

And /log/ is missing in src/test/modules/extensions/.gitignore.
-- 
Michael

On 2015-09-08 04:06, Michael Paquier wrote:
> On Tue, Sep 8, 2015 at 10:44 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>>
>> Attached are as well changes for the documentation that I spotted in
>> earlier reviews but were not included in the last version sent by Petr
>> yesterday. Feel free to discard them if you think they are not
>> adapted, the patch attached applies on top of Petr's patch.
>
> And /log/ is missing in src/test/modules/extensions/.gitignore.
>

Ah sorry, I based it on my branch which didn't contain your changes. Merged.

--
   Petr Jelinek                  http://www.2ndQuadrant.com/
   PostgreSQL Development, 24x7 Support, Training & Services

On 2015-09-16 05:44:22 +0200, Petr Jelinek wrote:
> On 2015-09-08 04:06, Michael Paquier wrote:
> >On Tue, Sep 8, 2015 at 10:44 AM, Michael Paquier
> ><michael.paquier@gmail.com> wrote:
> >>
> >>Attached are as well changes for the documentation that I spotted in
> >>earlier reviews but were not included in the last version sent by Petr
> >>yesterday. Feel free to discard them if you think they are not
> >>adapted, the patch attached applies on top of Petr's patch.
> >
> >And /log/ is missing in src/test/modules/extensions/.gitignore.
> >
> 
> Ah sorry, I based it on my branch which didn't contain your changes. Merged.

Please remember to update the commitfest entry...

> @@ -91,8 +92,38 @@ CREATE EXTENSION [ IF NOT EXISTS ] <replaceable class="parameter">extension_name
>          The name of the schema in which to install the extension's
>          objects, given that the extension allows its contents to be
>          relocated.  The named schema must already exist.
> -        If not specified, and the extension's control file does not specify a
> -        schema either, the current default object creation schema is used.
> +        If not specified, and the extension control file does not define
> +        <literal>schema</> either, the current default object creation
> +        schema is used.
> +       </para>

Imo still a spurious change.

Imo this is ready for committer. Will work on committing in the next few
days.

Greetings,

Andres Freund

Andres Freund wrote:

> > @@ -91,8 +92,38 @@ CREATE EXTENSION [ IF NOT EXISTS ] <replaceable class="parameter">extension_name
> >          The name of the schema in which to install the extension's
> >          objects, given that the extension allows its contents to be
> >          relocated.  The named schema must already exist.
> > -        If not specified, and the extension's control file does not specify a
> > -        schema either, the current default object creation schema is used.
> > +        If not specified, and the extension control file does not define
> > +        <literal>schema</> either, the current default object creation
> > +        schema is used.
> > +       </para>
> 
> Imo still a spurious change.

I think some more work is needed in this file -- ISTM the rules used to
determine the creation schema under CASCADE should not be placed within
the SCHEMA clause explanation.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2015-09-16 19:46:10 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> 
> > > @@ -91,8 +92,38 @@ CREATE EXTENSION [ IF NOT EXISTS ] <replaceable class="parameter">extension_name
> > >          The name of the schema in which to install the extension's
> > >          objects, given that the extension allows its contents to be
> > >          relocated.  The named schema must already exist.
> > > -        If not specified, and the extension's control file does not specify a
> > > -        schema either, the current default object creation schema is used.
> > > +        If not specified, and the extension control file does not define
> > > +        <literal>schema</> either, the current default object creation
> > > +        schema is used.
> > > +       </para>
> > 
> > Imo still a spurious change.
> 
> I think some more work is needed in this file -- ISTM the rules used to
> determine the creation schema under CASCADE should not be placed within
> the SCHEMA clause explanation.

Hm. Why not? Seems to make sense in the context of that page?

Greetings,

Andres Freund

On 2015-09-17 17:31, Jeff Janes wrote:
>
> If I fail to specify CASCADE and get an ERROR, I think there should be a
> HINT which suggests the use of CASCADE.
>
>
> create extension earthdistance ;
> ERROR:  required extension "cube" is not installed
>
> (no hint)
>
> There is a HINT on the reverse operation:
> drop extension cube;
> ERROR:  cannot drop extension cube because other objects depend on it
> DETAIL:  extension earthdistance depends on extension cube
> HINT:  Use DROP ... CASCADE to drop the dependent objects too.

Makes sense.

>
> Also, It would be nice to have psql tab complete the word CASCADE.
>

Hmm, it already does?


--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

<div dir="ltr"><p dir="ltr"> On Sep 17, 2015 7:52 PM, "Petr Jelinek" <<a href="mailto:petr@2ndquadrant.com"
target="_blank">petr@2ndquadrant.com</a>>wrote:<br /> ><br /> > On 2015-09-17 17:31, Jeff Janes wrote:<br />
>><br/> >><br /> >> Also, It would be nice to have psql tab complete the word CASCADE.<br />
>><br/> ><br /> > Hmm, it already does?<p>Indeed it does.  Oops.  I need to run the program I just
compiled,and not some other version that happens to be in my $PATH.  I've learned that for pg_ctl mostly, but still
forgetfor psql.<br /><p>Cheers,<p><br /><p>Jeff<br /></div> 

On 2015-09-18 04:52, Petr Jelinek wrote:
> On 2015-09-17 17:31, Jeff Janes wrote:
>>
>> If I fail to specify CASCADE and get an ERROR, I think there should be a
>> HINT which suggests the use of CASCADE.
>>
>>
>> create extension earthdistance ;
>> ERROR:  required extension "cube" is not installed
>>
>> (no hint)
>>
>> There is a HINT on the reverse operation:
>> drop extension cube;
>> ERROR:  cannot drop extension cube because other objects depend on it
>> DETAIL:  extension earthdistance depends on extension cube
>> HINT:  Use DROP ... CASCADE to drop the dependent objects too.
>
> Makes sense.
>

Here it is.


--
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

On 2015-09-20 16:38:58 +0200, Petr Jelinek wrote:
> Here it is.

I went over the patch, trying to commit it. Changed a bunch of stylistic
issues (comments, NOTICE location, ...) . But also found a bug: Namely
cascade_parent was set wrongly in a bunch of situations: When an
extension has multiple dependencies the current name would end up
multiple times on the list, and more importantly a parent's
cascade_parent would be corrupted because the list was being modified
in-place in the child.

Here's an updated patch. Petr, could you please expand the test to
handle a bit more complex cascading setups?

Michael: Why did you exclude test_extensions in Mkvcbuild.pm?

Greetings,

Andres Freund

On 2015-10-03 17:15:54 +0200, Andres Freund wrote:
> Here's an updated patch. Petr, could you please expand the test to
> handle a bit more complex cascading setups?

...

On 2015-10-03 17:16, Andres Freund wrote:
> On 2015-10-03 17:15:54 +0200, Andres Freund wrote:
>> Here's an updated patch. Petr, could you please expand the test to
>> handle a bit more complex cascading setups?
>

Okay, I changed the test to make the dependencies bit more complex -
more than one dependency per extension + also non-cyclic
interdependency. It still works as expected.

--
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

On 2015-10-03 17:56:07 +0200, Petr Jelinek wrote:
> On 2015-10-03 17:16, Andres Freund wrote:
> >On 2015-10-03 17:15:54 +0200, Andres Freund wrote:
> >>Here's an updated patch. Petr, could you please expand the test to
> >>handle a bit more complex cascading setups?
> >
> 
> Okay, I changed the test to make the dependencies bit more complex - more
> than one dependency per extension + also non-cyclic interdependency. It
> still works as expected.

Ok, pushed this way. We can update the windows testing bit later if
necessary.

Thanks!

Andres

On Sun, Oct 4, 2015 at 12:15 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-09-20 16:38:58 +0200, Petr Jelinek wrote:
> Michael: Why did you exclude test_extensions in Mkvcbuild.pm?

test_extensions contains nothing that should be compiled, only things
that should be installed.
-- 
Michael