Re: creating extension including dependencies

On Mon, Jul 20, 2015 at 10:20 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
> On 2015-07-19 17:16, Michael Paquier wrote:
>>
>> On Sat, Jul 18, 2015 at 8:00 AM, Petr Jelinek <petr(at)2ndquadrant(dot)com>
>> wrote:
>>>
>>> On 2015-07-15 06:07, Michael Paquier wrote:
>>>>
>>>>
>>>> On Fri, Jul 10, 2015 at 11:28 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>>>
>>>>>
>>>>> Andres Freund <andres(at)anarazel(dot)de> writes:
>>>>>>
>>>>>>
>>>>>> On July 10, 2015 4:16:59 PM GMT+02:00, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 2
test_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 2
test_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 1
test_ext2 | 1.0 | public | Test extension 2
test_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