Re: creating extension including dependencies

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