Review: Extensions Patch

Extensions Patch v15 Review
===========================

Submission review
-----------------

* Is the patch in context diff format?

Yes.

* Does it apply cleanly to the current git master?

Yes.

* Does it include reasonable tests, necessary doc patches, etc?

`make check` passes.
`make installcheck` fails (regression.diffs attached).
`make -C contrib installcheck` passes

I see tests for pg_execute_sql_string() and replace(), but absolutely nothing else. Such an important core feature really should have a pretty comprehensive suite of tests exercising all the functionality. Relying on the contrib extension tests won't do it, as they exercise only a subset of the functionality (e.g., no custom_variable_classes), and then only as a side-effect of their actual purpose.

Details on docs below.

Usability review
----------------

Read what the patch is supposed to do, and consider:

* Does the patch actually implement that?

Yes.

* Do we want that?

OH YES!

* Do we already have it?

Nope.

* Does it follow SQL spec, or the community-agreed behavior?

It's an implementation of community-agreed behavior, as hashed out on the mail list over the years.

* Does it include pg_dump support (if applicable)?

Yes, it does.

* Are there dangers?

Yes. Much of the execution is superuser-only, so we need to make sure that such is actually the case (unit tests would help with that).

* Have all the bases been covered?

Mostly, yes. The lack of tests is the single biggest drawback to this patch.

Feature test
------------

Apply the patch, compile it and test:

* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

I had some trouble getting a third-party extension to work. See the end of this document for details.

* Are there any assertion failures or crashes?

No.

Performance review
------------------

* Does the patch slow down simple tests?

Not that I've noticed.

* If it claims to improve performance, does it?

N/A.

* Does it slow down other things?

Not that I've noticed.

Coding review
-------------

Read the changes to the code in detail and consider:

* Does it follow the project [http://developer.postgresql.org/pgdocs/postgres/source.html coding guidelines]?

I'm not a C expert, but it looks like it matches quite closely with all the other code. It's very neat and well-commented.

* Are there portability issues?
* Will it work on Windows/BSD etc?

I can't comment on these.

* Are the comments sufficient and accurate?

Yes.

* Does it do what it says, correctly?

I can't comment on this by looking at the code; see detailed review from a user's perspective below.

* Does it produce compiler warnings?

No.

* Can you make it crash?

No.

Architecture review
-------------------

* Is everything done in a way that fits together coherently with other features/modules?
* Are there interdependencies that can cause problems?

Can't really comment on this.

Notes on the documentation
--------------------------

The pg_extension catalog docs are a little thin, but probably sufficient. They appear to be duplicated, though, with the entries for catalog-pg-extension and view-pg-extensions looking identical. One is apparently a list of installed extensions, and the other an SRF that lists installed and available extensions. But I find the duplication a bit confusing. (Might be easer if I wasn't reading SGML though.)

The extend-extension docs

I don't understand this paragraph at all:

<para>
The way to put together a coherent set of custom <acronym>SQL</> objects
is to create an <literal>extension</literal>. There are two sides of the
extension, the Operating System side contains several files and
the <acronym>SQL</> one uses them.
</para>

This paragraph isn't very clear, either:

<para>
The control file can also contain
a <literal>custom_variable_classes</literal> key followed by any
number of custom settings,
named <literal><replaceable class="parameter">class</replaceable>.varname</literal>.
The <literal>custom_variable_classes</literal> value takes effect
at <command>CREATE EXTENSION</command> time, and is persistent. The
custom settings are set automatically too, but are not persistent.

Examples might help.

Control file keys:

* comment -- Should mention that it's used for CREATE COMMENT ON EXTENSION, no?
* version -- If it's free-form, how will you do dependency-checking? Or will you?
* script
* encoding -- Seems unnecessary; one can explicitly set it in the .sql file, no?
* custom_variable_classes

I don't understand this paragraph:

<para>
The admin
function <link linkend="functions-extension">pg_extension_flag_dump</link>
can be used to revert the default <literal>pg_dump</literal> policy
about objects that belong to an extension and force the flagged objects
to be part of the backups.
</para>

What's a pg_dump policy?

* There appears to be an irrelevant change to the docs for replace().
* For the pg_read_binary_file() docs, should there not be some sort of note about permissions to the file to be read in?

With pg_execute_sql_string() and friends, what if there aren't the same number of parameters as there are placeholders? Should be a note about that.

Also, why are the placholders different than everywhere else? I see

+ SELECT pg_execute_sql_string('CREATE SCHEMA @schema@;', '@schema@', 'utils');

and

+ SELECT pg_execute_sql_string('CREATE SCHEMA s;', 's', 'bar');

Why not $1 like is used in PREPARE and user-defined functions? I think the second example is particularly weird, as there's nothing to indicate that "s" is a placeholder. I don't mind "@schema@", but would like to see uses of such things be consistent throughout PostgreSQL (and I expect that one cannot add such as placeholders to PREPARE).

In the CREATE EXTENSION docs, I don't understand this section:

<varlistentry>
<term>NO USER DATA</term>
<listitem>
<para>
This option is used by <xref linkend="APP-PGDUMP">. As it's possible
to flag extension's objects so that they get dumped
(see <xref linkend="functions-extension">), this option allow
extension authors to prepare installation scripts that will avoid
creating user data when restoring.
</para>
</listitem>
</varlistentry>

Does it mean that no user data will be dumped when the extension is dumped?

In the DROP EXTENSION docs, I think there's a pasto:

<refname>DROP EXTENSION</refname>
<refpurpose>remove a tablespace</refpurpose>

Another one here:

<varlistentry>
<term><literal>CASCADE</literal></term>
<listitem>
<para>
Automatically drop objects that depend on the index.
</para>
</listitem>
</varlistentry>

s/index/extension/. Same in the RESTRICT section.

In xfunc.sgml, I see:

<term><varname>EXTENSION</varname></term>
<listitem>
<para>
extension control files to install
into <literal><replaceable>prefix</replaceable>/share/$MODULEDIR</literal>,
derived from
the <literal><replaceable>extension</replaceable>.control.in</literal>
file to add in your sources. <literal>EXTENSION</literal> can also
be a list, you then have to provide a <literal>.control.in</literal>
file per extension in this list.

Why would multiple control files be necessary? And how would it map to EXTVERSION? Would all the extensions get the one version in EXTVERSION?

In func.sgml:

<literal><function>pg_extension_flag_dump(<parameter>objid</> <type>oid</>)</function></literal>
</entry>
<entry><type>bool</type></entry>
<entry>Flag an extension's <literal>objectid</literal> as worthy
of <literal>pg_dump</literal>.</entry>

Why is this necessary? Aren't all database objects "worthy" of being dumped? And this:

<entry>
<literal><function>pg_extension_with_user_data()</function></literal>
</entry>
<entry><type>bool</type></entry>
<entry>Return <literal>true</literal> only when the extension's
script should care for user data.</entry>

What constitutes "user data" when it comes to extensions? In contrast, the explanation below of "pg_extension_flag_dump" is very clear, making it obvious what it's for and why it might be useful. I don't see how it relates to the summary above, however. OTOH, it also says:

<para>
This function can only be called when loading a <acronym>SQL</> script
using the command <command>CREATE
EXTENSION</command>. See <xref linkend="sql-createextension">
and <xref linkend="extend-extension">.
</para>

And I have no idea how I'd call that function within another function call. Does this mean that only the extension author can call it? Seems like a weird scoping without precedent.

Hrm. Looking at extension.c, I see:

+ if (!create_extension)
+ ereport(ERROR,
+ (errmsg("This function can only be used from CREATE EXTENSION.")));

I think that's a confusing error message, frankly. Perhaps better would be something like "This function can only be called from SQL files executed by CREATE EXTENSION". The docs should make that clearer, too. It was only when I saw this error message that I realized how it's scoped.

Now, some of the information in the [wiki page](http://wiki.postgresql.org/wiki/Extensions) actually seems more informative. For example, the description and examples of \dx, \dx+, and pg_extension_objects() are *much* better. Any way to work those into the docs? Maybe they don't go into the function reference pages, but perhaps into the "Putting it all together" doc? In fact, I think the wiki doc should be copied into the "putting it all together" doc almost verbatim (modulo the patch excerpts). That will be a big help to prospective extension authors.

Speaking of pg_extension_objects(), it'd be nice if its output identified the extension to which each object belongs. Oh, I see, you can ask for only the objects in a single extension. That's okay then. But I don't think you need to document all the OUT paramters, just the required single argument specifying the extension name.

Reviewing the wiki page some more, you have this example (also in the docs, IIRC):

+ DO $$
+ BEGIN
+ IF pg_extension_with_user_data() THEN
+ create schema foo;
+ create table foo.bar(id serial primary key);
+ perform pg_extension_flag_dump('foo.bar_id_seq'::regclass);
+ perform pg_extension_flag_dump('foo.bar::regclass);
+ END IF;
+ END;
+ $$;

That's good, but what if someone has restored a database that has those objects already? Will the extension be re-created with this code before the tables are loaded? Or vice versa? Either way, you're going to have a conflict when you restore from a dump.

Code Review
-----------

I find the use of variables in the control files confusing. It looks like this:

version = 'EXTVERSION'

Why not use make syntax?

version = $(EXTVERSION)

That makes it much clearer that it's a variable and provides the same syntax a the extension developer will already be using in the Makefile.

I've already mentioned that the @extschema@ placeholder is different from any other SQL placeholder:

SET search_path = @extschema@;

I'm not sure of a better option, but I do think it should be made consistent, if at all possible, since this is executed in the database, not at `make` time.

Speaking of which, what happens if an extension has no search_path? Or an old explicit one?

SET search_path = public;

Hrm. I'm wondering if it might make more sense to ignore such statements when sometning is executed by `pg_execute_sql_file()`? IOW, if I do

CREATE EXTENSION foo WITH SCHEMA bar;

That the schema would be installed in bar no matter what search_path is set in the file loaded from the file system. I think that this would make CREATE EXTENSION behave more as expected, *and* allow existing extension distributions to just work (modulo the control file). No need for placeholders in the .sql file anymore, either. Maybe there's some optoin to CREATE EXTENSION that would *not* disable `SET search_path`.

Speaking of the contol file, I think that its inclusion in extension distributions should be completely optional. Of the keys supported:

* comment
* version
* script
* encoding
* custom_variable_classes

All except the last one could be written into the Makefile, and then `make` would generate the control file for installation. Only if the extension author wants to support custom_variable_classes would you need to create an explicit control file. And even that could probably be finessed with some sort of Make variable.

I'd like to see the control file be something that the extension author doesn't have to deal with, as much as possible. It just feels superfluous.

BTW, you might want to put the changes to the existing contrib modules in a separate patch, just to simplify things a bit. Not a big deal at this point, but it might help the committer to have further separation. But reviewing that bit, I was wondering, how are the uninstall scripts handled? Are they necessary anymore?

### Questions ###

* I assume that if an extension is installed that includes a DSO that the server will need to be HUPed before you can CREATE EXTENSION, yes?

* I trust that CREATE EXTENSION and DROP EXTENSION and ALTER EXTENSION are transactional, yes?

* Is there a regtype for extensions?

* Why can only super users get a list of extensions? It might be useful for me to be able to check from within a udf to see if a given extension is installed, for example. I can see why you wouldn't want to allow listing of extensions that are installed on the system but not in the current database.

* Is there no way to keep a list of available extensions somewhere in the cluster? Seems kind of silly to parse all the control files from within pg_extensions(). That can make a call to pg_extensions() pretty expensive.

* What's with `replace_text_variadic(PG_FUNCTION_ARGS)`? Related?

* \dx+ is not documented in \? in psql. I think you just need to add "[+]".

* `doc/src/sgml/ref/psql-ref.sgml` needs updating too.

* I like that I can see a list of installed and available extensions, but I'm not sure that \dx+ is the right command for the latter. Usually + means "more columns to provide more information for the same objects as you saw without the +". Is there any precedent for showing different objects with + than without?

Exercising the Feature
----------------------

* I built my cluster with all the contrib extensions. \dx+ does a nice job of listing what's available. So I ran `CREATE EXTENSION hstore`. It seemed to work great, but was *very* verbose. Every SQL statement was emitted to the terminal. I thought \echo was turned off by CREATE EXTENSION. I was able to use hstore after that; worked great. Yay!

* Wanted to try the schema support, so next did

CREATE SCHEMA ex;
CREATE EXTENSION citext WITH SCHEMA ex;

This was not verbose the way hstore was. And now it's nicely installed:

postgres=# \dx
List of extensions
Schema │ Name │ Version │ Description
────────┼────────┼──────────┼────────────────────────────────────────
ex │ citext │ 9.1devel │ case-insensitive character string type
public │ hstore │ 9.1devel │ storing sets of key/value pairs
(2 rows)

Very nice. And I was able to use it with `create table users (nickname ex.citext primary key);` Awesome.

* Next I tried dropping it. `DROP EXTENSION ex.citext;` did not work. What if I have citext installed in two schemas and just want to drop one of them? Ah, I see, one cannot have an extension with the same name in different schemas. Currently the schema-qualification is a syntax error, but maybe it should be a little more informative?

* Without the schema-qualification I get an error because it's in use. But `CASCADE` works. Great.

* The `pg_extension` catalog and `pg_extensions` views are both useful, but too close in name. I found it confusing how similar they are. It seems to me like the `pg_extension` class is well-named and like the other catalog table names, but `pg_extenions` should perhaps be `pg_available_extensions` or something.

Creating a Third Party Extension
--------------------------------

I have a very simple extension I wrote more or less as an example extension for PGXN. It's a key/value pair data type called [pair](https://github.com/theory/kv-pair). As an experiment, to see how it would feel to use extensions as an extension developer, I created [a branch](https://github.com/theory/kv-pair/tree/9.1-extension) to port it. With that change, I gave it a try:

export PATH=/usr/local/pgsql-devel/bin:$PATH
make
sudo make install
make installcheck PGUSER=postgres

The test failed with this error:

! CREATE EXTENSION pair;
! NOTICE: Installing extension 'pair' from '/usr/local/pgsql-devel/share/contrib/pair.sql
! ERROR: could not execute sql file: '/usr/local/pgsql-devel/share/contrib/pair.sql'

The file is there and looks fine. I also just tried to install it myself:

> /usr/local/pgsql-devel/bin/psql -U postgres
psql (9.1devel)
Type "help" for help.

postgres=# create extension pair;
NOTICE: Installing extension 'pair' from '/usr/local/pgsql-devel/share/contrib/pair.sql', in schema public, with user data
ERROR: could not execute sql file: '/usr/local/pgsql-devel/share/contrib/pair.sql'

It would be nice if it gave me more information. What failed, exactly? It also fails when I use `WITH SCHEMA public`.

I assume this will be a simple issue. Overall I'm happy with how easy it will be to update existing extensions to use the new functionality, but as I've mentioned above, I do think that it would be better if:

* One could specify stuff *only* in the `Makefile` and let `make` create a control file.

* I could omit the @extschema@ business altogether and just let CREATE EXTENSION set the path for the duration of its execution.

With these two changes, it would be *even easier* for existing third-party extensions to be adapted.

Conclusion
----------

Overall I'm very happy with this patch. There are some nits I've brought up here that need to be addressed, and I do think there are some design changes that would be worth considering. So I'm marking it as returned. But I also think that someone more familiar with the core code should do a proper code review during this commitfest (I'm mostly about functionality and interface).

But I think it's close, and I can't wait to have this stuff solidly in core.

Best,

David

Hi,

Thanks for the review, that's quite one! :)

I'm not sure to follow you all along, it seems like the reading is "try
it first then understand and comment again", so sometimes I'm not sure
if you're saying that docs are missing the point or that the behaviour
ain't right.

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> `make installcheck` fails (regression.diffs attached).

You forgot the attachment. I'd add mine if I didn't get "All 124 tests
passed." result for the command :)

(Note that I've been having problems too, but can't reproduce them
easily, and they might have been related to forgotten maintainer-clean
steps).

> I see tests for pg_execute_sql_string() and replace(), but absolutely
> nothing else. Such an important core feature really should have a
> pretty comprehensive suite of tests exercising all the
> functionality. Relying on the contrib extension tests won't do it, as
> they exercise only a subset of the functionality (e.g., no
> custom_variable_classes), and then only as a side-effect of their
> actual purpose.

Well, the exposed functionality aren't much (2 SQL commands and some
support functions) and you need to have a contrib at hand to exercise
them. So I'm not sure about how to add in regression tests for contrib
infrastructure work and have them depend only on -core. I'll take ideas.

The other thing is that the very important feature here is dump and
restore of a database containing extensions. You're not reporting about
it, though. And I'm not sure about how to check for pg_dump content from
pg_regress oriented tests.

> Yes. Much of the execution is superuser-only, so we need to make sure
> that such is actually the case (unit tests would help with that).

How do you mean? Trying to subvert the features by being able to run
them as a non-superuser? How do you install tests around that?

> Notes on the documentation
> --------------------------
>
> The pg_extension catalog docs are a little thin, but probably
> sufficient. They appear to be duplicated, though, with the entries for
> catalog-pg-extension and view-pg-extensions looking identical. One is
> apparently a list of installed extensions, and the other an SRF that
> lists installed and available extensions. But I find the duplication a
> bit confusing. (Might be easer if I wasn't reading SGML though.)

Well yes the pg_extension catalog is filled at CREATE EXTENSION time so
that we have an OID to register dependencies on. Then I added a SRF to
also list available extensions so that it's easy to integrate the
feature into pgadmin and the like.

Now I'm open to suggestions as far as the names are involved, or maybe
we should keep the SRF but remove the view. psql could easily enough use
the function directly I think.

> I don't understand this paragraph at all:
> This paragraph isn't very clear, either:
> Examples might help.

Well, I think native English skills are required here. And I'm not too
sure about the example.

> Control file keys:
>
> * comment -- Should mention that it's used for CREATE COMMENT ON EXTENSION, no?
> * version -- If it's free-form, how will you do dependency-checking? Or will you?
> * script
> * encoding -- Seems unnecessary; one can explicitly set it in the .sql file, no?
> * custom_variable_classes

Doc says:
http://pgsql.tapoueh.org/extensions/doc/html/extend-extension.html

This control file allows for registering the extension and should
provide values for the keys described below.

Then there's the detail about fields. I think the context is enough to
understand, but I'd be happy to integrate some revisions of the current
content.

For the version, it has been agreed before (Developer Meeting) that v1
would not care about inter-extension dependencies nor be smart about it,
or I would have proposed a patch to have the following into core:

http://packages.debian.org/sid/postgresql-9.0-debversion

> I don't understand this paragraph:
>
> <para>
> The admin
> function <link linkend="functions-extension">pg_extension_flag_dump</link>
> can be used to revert the default <literal>pg_dump</literal> policy
> about objects that belong to an extension and force the flagged objects
> to be part of the backups.
> </para>
>
> What's a pg_dump policy?

Here again not being a native speaker shows up it seems. What I mean is
that the only goal of all this patch is for pg_dump to stop including
extensions install scripts, but just mark the dependency.

So obviously, if your extension provides for some user editable content,
you want to reverse the default behavior and have those content be parts
of the dump, right?

Now, to phrase that in a manual fashion...

> * There appears to be an irrelevant change to the docs for replace().

As detailed in another thread, the patch you're reviewing depends on
another one that has been reviewed separately, marked as ready, but is
not yet in. So the extension patch is including the pg_execute_sql_file
patch, to ease your reviewer work (you don't have to deal with patch
dependencies). So you're seeing unrelated-at-first-glance changes.

> Also, why are the placholders different than everywhere else? I see
>
> + SELECT pg_execute_sql_string('CREATE SCHEMA @schema@;', '@schema@', 'utils');
>
> and
>
> + SELECT pg_execute_sql_string('CREATE SCHEMA s;', 's', 'bar');
>
> Why not $1 like is used in PREPARE and user-defined functions? I think
> the second example is particularly weird, as there's nothing to
> indicate that "s" is a placeholder. I don't mind "@schema@", but would
> like to see uses of such things be consistent throughout PostgreSQL
> (and I expect that one cannot add such as placeholders to PREPARE).

The @extschema@ syntax has been proposed by Heikki, and I just adopted
it. If another is to emerge, I'll adapt to it. While preparing this, I
didn't want to mix mechanism and policy, so there's nothing imposed on
the extension authors or the function users wrt to what a placeholder
name should look like.

> In the CREATE EXTENSION docs, I don't understand this section:
> <varlistentry>
> <term>NO USER DATA</term>
> Does it mean that no user data will be dumped when the extension is
> dumped?

Consider you have an extension with user data. At create extension time,
you're creating a table to hold the data, and maybe you insert default
values into it. At pg_restore time, CREATE EXTENSION will be called
again. Do you want the extension's provided default user data to be
restored, or the one from the backup?

So pg_dump will issue CREATE EXTENSION foo WITH NO USER DATA variant,
and will also issue dump commands for objects marked as being user data,
so that at pg_restore time you get the values from the backup when it
makes sense.

Updates on the manual style paragraph to explain that are welcome. It
could be that what's needed is a better overview of the whole thing
somewhere, but I though what's written would be enough. It seems to be
only enough when you already know what it's all about --- and that's
typical of Unix style man pages. Do we want to include an extension
author tutorial?

> In the DROP EXTENSION docs, I think there's a pasto:
> s/index/extension/. Same in the RESTRICT section.

Thanks, fixed here.

> Why would multiple control files be necessary? And how would it map to
> EXTVERSION? Would all the extensions get the one version in
> EXTVERSION?

That part has been extensively re-hashed by Alvaro (and Heikki I think)
already. The basis for multiple control files per extension is to be
able to cope with contrib/spi, that contains 5 extensions:

MODULES = autoinc insert_username moddatetime refint timetravel
EXTENSION = autoinc auto_username moddatetime refint timetravel
EXTVERSION = $(VERSION)

Now, would you make the same layout choice and maintain different
version numbering schemes, you still can do that, but you won't benefit
from the EXTVERSION facility.

> <literal><function>pg_extension_flag_dump(<parameter>objid</> <type>oid</>)</function></literal>
> Why is this necessary? Aren't all database objects "worthy" of being
> dumped? And this:

Remember that the only goal of all that patch is *NOT* to dump the
extension's install script?

> What constitutes "user data" when it comes to extensions? In contrast,
> the explanation below of "pg_extension_flag_dump" is very clear,
> making it obvious what it's for and why it might be useful. I don't
> see how it relates to the summary above, however. OTOH, it also says:

User data is anything that the extension's install script flagged so.

> <para>
> This function can only be called when loading a <acronym>SQL</> script
> using the command <command>CREATE
> EXTENSION</command>. See <xref linkend="sql-createextension">
> and <xref linkend="extend-extension">.
> </para>
>
> And I have no idea how I'd call that function within another function
> call. Does this mean that only the extension author can call it? Seems
> like a weird scoping without precedent.

Indeed. Those two functions only make sense when used while running the
CREATE EXTENSION sql command, so are only meaningful in the extension's
install script. And yes that looks like a precedent.

> + (errmsg("This function can only be used from CREATE EXTENSION.")));
>
> I think that's a confusing error message, frankly. Perhaps better
> would be something like "This function can only be called from SQL
> files executed by CREATE EXTENSION". The docs should make that
> clearer, too. It was only when I saw this error message that I
> realized how it's scoped.

Any proposal here?

> Now, some of the information in the [wiki
> page](http://wiki.postgresql.org/wiki/Extensions) actually seems more
> informative. For example, the description and examples of \dx, \dx+,
> and pg_extension_objects() are *much* better. Any way to work those
> into the docs? Maybe they don't go into the function reference pages,
> but perhaps into the "Putting it all together" doc? In fact, I think
> the wiki doc should be copied into the "putting it all together" doc
> almost verbatim (modulo the patch excerpts). That will be a big help
> to prospective extension authors.

Well maybe it's just me but I feel like wiki content is "free style"
while the manual tone has to be sharp and concise, and the examples in
there must be carefully chosen so as not to deprecate early. But if the
consensus is to add the wiki content into the documentation chapter,
that's what I'll do for next patch version.

http://pgsql.tapoueh.org/extensions/doc/html/extend-extension.html

> Reviewing the wiki page some more, you have this example (also in the docs, IIRC):
>
> + DO $$
> + BEGIN
> + IF pg_extension_with_user_data() THEN
> + create schema foo;
> + create table foo.bar(id serial primary key);
> + perform pg_extension_flag_dump('foo.bar_id_seq'::regclass);
> + perform pg_extension_flag_dump('foo.bar::regclass);
> + END IF;
> + END;
> + $$;
>
> That's good, but what if someone has restored a database that has
> those objects already? Will the extension be re-created with this code
> before the tables are loaded? Or vice versa? Either way, you're going
> to have a conflict when you restore from a dump.

The example is in the docs, yes. The whole goal of the two functions
pg_extension_flag_dump() and pg_extension_with_user_data() is for
pg_restore to instruct the extension's install script if it should
create the user data objects or bypass the step because they will be in
the dump already.

I think we have to find a way to state that clearly in the manual.

> Code Review
> -----------
>
> I find the use of variables in the control files confusing. It looks like this:
>
> version = 'EXTVERSION'
>
> Why not use make syntax?
>
> version = $(EXTVERSION)
>
> That makes it much clearer that it's a variable and provides the same
> syntax a the extension developer will already be using in the
> Makefile.

For starters, you should be comparing this facility with the .sql.in one
where you can use the following notation:

CREATE OR REPLACE FUNCTION citext_cmp(citext, citext)
RETURNS int4
AS 'MODULE_PATHNAME'
LANGUAGE C STRICT IMMUTABLE;

Then, if you insist on some strange cleverer input syntax, I'll welcome
the C-coded patch to make that happen. You might want to know that the
current extension patch is re-using the same parsing code that is used
for postgresql.conf and recovery.conf, though.

> I've already mentioned that the @extschema@ placeholder is different from any other SQL placeholder:
>
> SET search_path = @extschema@;
>
> I'm not sure of a better option, but I do think it should be made
> consistent, if at all possible, since this is executed in the
> database, not at `make` time.

Again, there's no technical restriction in there.

=# select replace('Are placeholders $2 and $1 better than @name(at)?', '$1', 'foo', '$2', 'bar', '@name@', '@');
replace
---------------------------------------------
Are placeholders bar and foo better than @?
(1 row)

It looks like bikeshedding time. We managed to reduce it up until now,
but well, it has to kick-in after a while :)

> Speaking of which, what happens if an extension has no search_path? Or
> an old explicit one?

No. Please do not try to open this can of worms. It took one entire year
for us to agree that extension should not concern themselves with schema
and search_path issues, and 2 developer meetings.

So what's in the patch is a very simple way for users to instruct the
extension about a schema preference choice, which as been asked by
Heikki and Robert this time. I'll try to come up with the archives link
later, now ain't a good time for me to crawl the lists.

The only other acceptable option would be to do nothing with schema at
install time and to ask users to use the next patch in the series.

CREATE EXTENSION foo;
ALTER EXTENSION foo SET SCHEMA bar;

Now, this would have to fail if more than exactly one schema is
depending on the extension's foo, and that's it. Considering that most
(if not all) extensions are using only one schema, it was though nice to
include the WITH SCHEMA bar; facility at CREATE EXTENSION time, and the
simple implementation is what you're commenting on.

> All except the last one could be written into the Makefile, and then
> `make` would generate the control file for installation. Only if the
> extension author wants to support custom_variable_classes would you
> need to create an explicit control file. And even that could probably
> be finessed with some sort of Make variable.

We've been trying to get there, and Tom and Alvaro both said that it's
not worth the Makefile effort and dance, then Itagaki proposed the
current mechanism, which is based on extension.control.in files and a
simple implicit Make rule in pgxs.mk.

> BTW, you might want to put the changes to the existing contrib modules
> in a separate patch, just to simplify things a bit. Not a big deal at
> this point, but it might help the committer to have further
> separation. But reviewing that bit, I was wondering, how are the
> uninstall scripts handled? Are they necessary anymore?

Separating the only use case of the patch away does not sound like
simplifying anything for me, but maybe that's only because I'm already
hard at work maintaining 7 git branches (+ master) to be able to issue
reviewable patches.

As far as the uninstall scripts goes, yes, I think they are not
necessary anymore and I now realize I didn't remove them. Will wait for
some more input before to add their removal in the v16 patch.

> ### Questions ###
>
> * I assume that if an extension is installed that includes a DSO that
> the server will need to be HUPed before you can CREATE EXTENSION, yes?

If by DSO you mean dynamic shared object (.so, .dylib or .dll) then no.

> * I trust that CREATE EXTENSION and DROP EXTENSION and ALTER EXTENSION
> are transactional, yes?

Sure. But the patch you're reviewing is not featuring ALTER EXTENSION,
that's for the next one :)

> * Is there a regtype for extensions?

No. Do we need one?

> * Why can only super users get a list of extensions? It might be
> useful for me to be able to check from within a udf to see if a given
> extension is installed, for example. I can see why you wouldn't want
> to allow listing of extensions that are installed on the system but
> not in the current database.

Anyone can SELECT FROM pg_extension, the catalog, and so list the
installed extensions. Same for listing an extension's object.

The superuser only function is the one that has to parse the control
files. I'm not good about security concerns, but it well seems to me
that parsing files in $SHAREDIR should be superuser only.

So I think what you want to happen is what's in the code. Right?

> * Is there no way to keep a list of available extensions somewhere in
> the cluster? Seems kind of silly to parse all the control files from
> within pg_extensions(). That can make a call to pg_extensions() pretty
> expensive.

Where would you put that cache, and when do you invalidate its entries
or add new ones? I don't think that listing available extensions is such
a frequent operation that it warrant for its own cache, even if you're
able to come up with a good enough management rule set.

Oh, and I just did select * from pg_extensions(); 3 times here, to be
sure we're on the same page. Time: 166,868 ms then 7,823 then 3,584.

> * What's with `replace_text_variadic(PG_FUNCTION_ARGS)`? Related?

Again, it's all to do with giving you a single all-included patch. This
function is a byproduct of the pg_execute_from_file() patch review.

> * \dx+ is not documented in \? in psql. I think you just need to add
> "[+]".

Well spotted! Fixed here.

> * `doc/src/sgml/ref/psql-ref.sgml` needs updating too.

Done, thanks.

> * I like that I can see a list of installed and available extensions,
> but I'm not sure that \dx+ is the right command for the
> latter. Usually + means "more columns to provide more information for
> the same objects as you saw without the +". Is there any precedent for
> showing different objects with + than without?

Well, I'm not sure about what the best user interface for it is in psql,
but the feature is there already:

=# select name, comment, installed from pg_extensions limit 2;
name | comment | installed
---------------+--------------------------------------------+-----------
adminpack | Administrative functions for PostgreSQL | t
auto_username | functions for tracking who changed a table | f
(2 rows)

> Exercising the Feature
> ----------------------
>
> * I built my cluster with all the contrib extensions. \dx+ does a nice
> job of listing what's available. So I ran `CREATE EXTENSION
> hstore`. It seemed to work great, but was *very* verbose. Every SQL
> statement was emitted to the terminal. I thought \echo was turned off
> by CREATE EXTENSION. I was able to use hstore after that; worked
> great. Yay!

The chatter has been discussed extensively with Alvaro and Tom in the
pg_execute_from_file() branch already --- sorry about that, but really,
I though that handing you a single patch would make it easier.

So this chatter is a result of 2 things:

- we're using SPI in CREATE EXTENSION

- hstore issues a WARNING on the => operator creation, and SPI reaction
to WARNING is to dump the query string, which is the install script
here.

> * Next I tried dropping it. `DROP EXTENSION ex.citext;` did not
> work. What if I have citext installed in two schemas and just want to
> drop one of them? Ah, I see, one cannot have an extension with the
> same name in different schemas. Currently the schema-qualification is
> a syntax error, but maybe it should be a little more informative?

Well this point is not decided upon yet. I'm arguing that an extension
is a database global object, and there's a point to be made for them to
live in a schema.

Currently, they depend on the schema you've been using at CREATE
EXTENSION time, and the schema created by the script are depending on
the extension, and as a result all is working fine and following the
usual conventions (try drop schema cascade when its owner ain't a
superuser but an extension has been installed there, e.g.).

It could be that the easiest way to fix the current mess is to remove
WITH SCHEMA support at CREATE EXTENSION time and have the facility back
in core with the ALTER EXTENSION SET SCHEMA patch.

> * The `pg_extension` catalog and `pg_extensions` views are both
> useful, but too close in name. I found it confusing how similar they
> are. It seems to me like the `pg_extension` class is well-named and
> like the other catalog table names, but `pg_extenions` should perhaps
> be `pg_available_extensions` or something.

Give me the name and I'll update the patch. Well, or this could be
considered a commiter decision.

> Creating a Third Party Extension
> --------------------------------
>
> ! CREATE EXTENSION pair;
> ! NOTICE: Installing extension 'pair' from '/usr/local/pgsql-devel/share/contrib/pair.sql
> ! ERROR: could not execute sql file: '/usr/local/pgsql-devel/share/contrib/pair.sql'
>
> It would be nice if it gave me more information. What failed, exactly?
> It also fails when I use `WITH SCHEMA public`.

Yeah, that's a byproduct of using SPI_execute() in pg_execute_sql_file.
Your script contains BEGIN; and COMMIT; commands. It's not supposed to.
You won't find a single contrib module containing those either, with or
without my patch.

> * One could specify stuff *only* in the `Makefile` and let `make`
> create a control file.

Been trying, failed. Sorry.

> * I could omit the @extschema@ business altogether and just let CREATE
> EXTENSION set the path for the duration of its execution.

That won't fly. The only workable solution is a 2-steps recipe:
CREATE EXTENSION pair;
ALTER EXTENSION pair SET SCHEMA utils;

And the second command will raise an error if 'pair' has registered
dependencies for more than exactly one schema. Some work needs to be
done to see that happen, though.

> Conclusion
> ----------
>
> Overall I'm very happy with this patch. There are some nits I've
> brought up here that need to be addressed, and I do think there are
> some design changes that would be worth considering. So I'm marking it
> as returned. But I also think that someone more familiar with the core
> code should do a proper code review during this commitfest (I'm mostly
> about functionality and interface).

I'm pleased to see that you mean 'Waiting on Author', because I intend
to answer timely enough to still hope for the patch to make it in this
commitfest, baring showstoppers.

If you follow the git repository you will notice that the doc changes
asked for in this review are pushed. I'll wait some more for us to
decide on remaining points before to produce yet another patch version,
though.

http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=shortlog;h=refs/heads/extension

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

On Dec 4, 2010, at 6:14 AM, Dimitri Fontaine wrote:

> Hi,
>
> Thanks for the review, that's quite one! :)
>
> I'm not sure to follow you all along, it seems like the reading is "try
> it first then understand and comment again", so sometimes I'm not sure
> if you're saying that docs are missing the point or that the behaviour
> ain't right.

Overall I think the docs could use a lot more fleshing out. Some of the stuff in the wiki would help a lot. At some point, though, I'll work over the docs myself and either send a patch to you or to the list (if it has been committed to core).

> "David E. Wheeler" <david(at)kineticode(dot)com> writes:
>> `make installcheck` fails (regression.diffs attached).
>
> You forgot the attachment. I'd add mine if I didn't get "All 124 tests
> passed." result for the command :)

Sorry, attached now.

> (Note that I've been having problems too, but can't reproduce them
> easily, and they might have been related to forgotten maintainer-clean
> steps).

Yeah, I did that a bunch of times. This was the best run.

> Well, the exposed functionality aren't much (2 SQL commands and some
> support functions) and you need to have a contrib at hand to exercise
> them. So I'm not sure about how to add in regression tests for contrib
> infrastructure work and have them depend only on -core. I'll take ideas.

You write a very simple contrib module exclusively for testing. It doesn't have to actually do anything other than create a couple of objects. A domain perhaps.

> The other thing is that the very important feature here is dump and
> restore of a database containing extensions. You're not reporting about
> it, though. And I'm not sure about how to check for pg_dump content from
> pg_regress oriented tests.

Yeah, I haven't tried the dump and restore stuff. Are there no dump/restore tests in core? If there are, I expect all you'd have to do is add an extension or two to be dumped and restored.

>> Yes. Much of the execution is superuser-only, so we need to make sure
>> that such is actually the case (unit tests would help with that).
>
> How do you mean? Trying to subvert the features by being able to run
> them as a non-superuser? How do you install tests around that?

If the tests run as a super user (I think they usually do), then all you have to do is create a new unprivileged user, switch to that user via SET ROLE, and call the function in question.

>> The pg_extension catalog docs are a little thin, but probably
>> sufficient. They appear to be duplicated, though, with the entries for
>> catalog-pg-extension and view-pg-extensions looking identical. One is
>> apparently a list of installed extensions, and the other an SRF that
>> lists installed and available extensions. But I find the duplication a
>> bit confusing. (Might be easer if I wasn't reading SGML though.)
>
> Well yes the pg_extension catalog is filled at CREATE EXTENSION time so
> that we have an OID to register dependencies on. Then I added a SRF to
> also list available extensions so that it's easy to integrate the
> feature into pgadmin and the like.

Right.

> Now I'm open to suggestions as far as the names are involved, or maybe
> we should keep the SRF but remove the view. psql could easily enough use
> the function directly I think.

I think the catalog should be pg_created_extensions (or pg_installed_extensions) and the view should be pg_available_extensions.

Is there no way to have a catalog table visible to *all* databases that contains the list of available extensions?

>> I don't understand this paragraph at all:
>> This paragraph isn't very clear, either:
>> Examples might help.
>
> Well, I think native English skills are required here. And I'm not too
> sure about the example.

I recognize that some documentation pages have no (or very few examples). But oftentimes nothing makes the utility of a feature more apparent than an example. I tend to use them liberally in my documentation.

As I said, I'll probably contribute a refactoring of the docs at some point.

>> Control file keys:
>>
>> * comment -- Should mention that it's used for CREATE COMMENT ON EXTENSION, no?
>> * version -- If it's free-form, how will you do dependency-checking? Or will you?
>> * script
>> * encoding -- Seems unnecessary; one can explicitly set it in the .sql file, no?
>> * custom_variable_classes
>
> Doc says:
> http://pgsql.tapoueh.org/extensions/doc/html/extend-extension.html
>
> This control file allows for registering the extension and should
> provide values for the keys described below.
>
> Then there's the detail about fields. I think the context is enough to
> understand, but I'd be happy to integrate some revisions of the current
> content.

Yes, I think if you just mentioned that the comment is used for an implicit CREATE COMMENT that it would be useful. That's true, yes? The others are okay as-is.

> For the version, it has been agreed before (Developer Meeting) that v1
> would not care about inter-extension dependencies nor be smart about it,
> or I would have proposed a patch to have the following into core:
>
> http://packages.debian.org/sid/postgresql-9.0-debversion

OKay.

>
>> I don't understand this paragraph:
>>
>> <para>
>> The admin
>> function <link linkend="functions-extension">pg_extension_flag_dump</link>
>> can be used to revert the default <literal>pg_dump</literal> policy
>> about objects that belong to an extension and force the flagged objects
>> to be part of the backups.
>> </para>
>>
>> What's a pg_dump policy?
>
> Here again not being a native speaker shows up it seems. What I mean is
> that the only goal of all this patch is for pg_dump to stop including
> extensions install scripts, but just mark the dependency.

Yeah. Say that then. Makes much more sense.

> So obviously, if your extension provides for some user editable content,
> you want to reverse the default behavior and have those content be parts
> of the dump, right?

Right.

> Now, to phrase that in a manual fashion...
>
>> * There appears to be an irrelevant change to the docs for replace().
>
> As detailed in another thread, the patch you're reviewing depends on
> another one that has been reviewed separately, marked as ready, but is
> not yet in. So the extension patch is including the pg_execute_sql_file
> patch, to ease your reviewer work (you don't have to deal with patch
> dependencies). So you're seeing unrelated-at-first-glance changes.

Got it.

>> Also, why are the placholders different than everywhere else? I see
>>
>> + SELECT pg_execute_sql_string('CREATE SCHEMA @schema@;', '@schema@', 'utils');
>>
>> and
>>
>> + SELECT pg_execute_sql_string('CREATE SCHEMA s;', 's', 'bar');
>>
>> Why not $1 like is used in PREPARE and user-defined functions? I think
>> the second example is particularly weird, as there's nothing to
>> indicate that "s" is a placeholder. I don't mind "@schema@", but would
>> like to see uses of such things be consistent throughout PostgreSQL
>> (and I expect that one cannot add such as placeholders to PREPARE).
>
> The @extschema@ syntax has been proposed by Heikki, and I just adopted
> it. If another is to emerge, I'll adapt to it. While preparing this, I
> didn't want to mix mechanism and policy, so there's nothing imposed on
> the extension authors or the function users wrt to what a placeholder
> name should look like.

Okay. I think I see the need to differentiate between a variable in .control.in replaced by `make` and one replaced by `CREATE EXTENSION`. Something explaining that might be good.

>> In the CREATE EXTENSION docs, I don't understand this section:
>> <varlistentry>
>> <term>NO USER DATA</term>
>> Does it mean that no user data will be dumped when the extension is
>> dumped?
>
> Consider you have an extension with user data. At create extension time,
> you're creating a table to hold the data, and maybe you insert default
> values into it. At pg_restore time, CREATE EXTENSION will be called
> again. Do you want the extension's provided default user data to be
> restored, or the one from the backup?

Backup.

> So pg_dump will issue CREATE EXTENSION foo WITH NO USER DATA variant,
> and will also issue dump commands for objects marked as being user data,
> so that at pg_restore time you get the values from the backup when it
> makes sense.

Ah, I see.

> Updates on the manual style paragraph to explain that are welcome. It
> could be that what's needed is a better overview of the whole thing
> somewhere, but I though what's written would be enough. It seems to be
> only enough when you already know what it's all about --- and that's
> typical of Unix style man pages. Do we want to include an extension
> author tutorial?

Yes!

>> Why would multiple control files be necessary? And how would it map to
>> EXTVERSION? Would all the extensions get the one version in
>> EXTVERSION?
>
> That part has been extensively re-hashed by Alvaro (and Heikki I think)
> already. The basis for multiple control files per extension is to be
> able to cope with contrib/spi, that contains 5 extensions:
>
> MODULES = autoinc insert_username moddatetime refint timetravel
> EXTENSION = autoinc auto_username moddatetime refint timetravel
> EXTVERSION = $(VERSION)
>
> Now, would you make the same layout choice and maintain different
> version numbering schemes, you still can do that, but you won't benefit
> from the EXTVERSION facility.

Okay. So a control file corresponds to a single extension with a single version, but that extension might have multiple files, right? No, wait, you have five *extensions* above (but only one version number). Why not just make it *on* extension in five scripts? That would be less confusing to me, given that you can have only one version number, it appears.

>> <literal><function>pg_extension_flag_dump(<parameter>objid</> <type>oid</>)</function></literal>
>> Why is this necessary? Aren't all database objects "worthy" of being
>> dumped? And this:
>
> Remember that the only goal of all that patch is *NOT* to dump the
> extension's install script?

I do now, yes.

>> What constitutes "user data" when it comes to extensions? In contrast,
>> the explanation below of "pg_extension_flag_dump" is very clear,
>> making it obvious what it's for and why it might be useful. I don't
>> see how it relates to the summary above, however. OTOH, it also says:
>
> User data is anything that the extension's install script flagged so.

Okay. The scoping feels a little odd, given the example:

DO $$
BEGIN
IF pg_extension_with_user_data() THEN
create schema foo;
create table foo.bar(id serial primary key);
perform pg_extension_flag_dump('foo.bar_id_seq'::regclass);
perform pg_extension_flag_dump('foo.bar::regclass);
END IF;
END;
$$;

But not too bad I guess.

>> <para>
>> This function can only be called when loading a <acronym>SQL</> script
>> using the command <command>CREATE
>> EXTENSION</command>. See <xref linkend="sql-createextension">
>> and <xref linkend="extend-extension">.
>> </para>
>>
>> And I have no idea how I'd call that function within another function
>> call. Does this mean that only the extension author can call it? Seems
>> like a weird scoping without precedent.
>
> Indeed. Those two functions only make sense when used while running the
> CREATE EXTENSION sql command, so are only meaningful in the extension's
> install script. And yes that looks like a precedent.

Yeah.

>> + (errmsg("This function can only be used from CREATE EXTENSION.")));
>>
>> I think that's a confusing error message, frankly. Perhaps better
>> would be something like "This function can only be called from SQL
>> files executed by CREATE EXTENSION". The docs should make that
>> clearer, too. It was only when I saw this error message that I
>> realized how it's scoped.
>
> Any proposal here?

Yes. Change the error message to "This function can only be called from SQL
files executed by CREATE EXTENSION".

> Well maybe it's just me but I feel like wiki content is "free style"
> while the manual tone has to be sharp and concise, and the examples in
> there must be carefully chosen so as not to deprecate early. But if the
> consensus is to add the wiki content into the documentation chapter,
> that's what I'll do for next patch version.
>
> http://pgsql.tapoueh.org/extensions/doc/html/extend-extension.html

Yes, I definitely think that there should be a lot more information in that page, including an example or six. Some of the stuff from the wiki page would go a long way towards that. And that seems like the right place for it, too, as long as the CREATE EXTENSION page points to it.

> The example is in the docs, yes. The whole goal of the two functions
> pg_extension_flag_dump() and pg_extension_with_user_data() is for
> pg_restore to instruct the extension's install script if it should
> create the user data objects or bypass the step because they will be in
> the dump already.
>
> I think we have to find a way to state that clearly in the manual.

Yes. I see now how it works, but it was a bit confusing for me to get there. I think I had some trouble with the term "user data," though I don't have a better term to offer, alas.

>
>> Code Review
>> -----------
>>
>> I find the use of variables in the control files confusing. It looks like this:
>>
>> version = 'EXTVERSION'
>>
>> Why not use make syntax?
>>
>> version = $(EXTVERSION)
>>
>> That makes it much clearer that it's a variable and provides the same
>> syntax a the extension developer will already be using in the
>> Makefile.
>
> For starters, you should be comparing this facility with the .sql.in one
> where you can use the following notation:
>
> CREATE OR REPLACE FUNCTION citext_cmp(citext, citext)
> RETURNS int4
> AS 'MODULE_PATHNAME'
> LANGUAGE C STRICT IMMUTABLE;

Right, I forgot about MODULE_PATHNAME in the .sql.in file.

> Then, if you insist on some strange cleverer input syntax, I'll welcome
> the C-coded patch to make that happen. You might want to know that the
> current extension patch is re-using the same parsing code that is used
> for postgresql.conf and recovery.conf, though.

It's probably fine, given the precedent of MODULE_PATHNAME. I'd rather see the .control file killed from the distribution altogether (that is, generated by `make`).

>> I've already mentioned that the @extschema@ placeholder is different from any other SQL placeholder:
>>
>> SET search_path = @extschema@;
>>
>> I'm not sure of a better option, but I do think it should be made
>> consistent, if at all possible, since this is executed in the
>> database, not at `make` time.
>
> Again, there's no technical restriction in there.
>
> =# select replace('Are placeholders $2 and $1 better than @name(at)?', '$1', 'foo', '$2', 'bar', '@name@', '@');
> replace
> ---------------------------------------------
> Are placeholders bar and foo better than @?
> (1 row)
>
> It looks like bikeshedding time. We managed to reduce it up until now,
> but well, it has to kick-in after a while :)

Oh, so they're not defined placeholders. I can do anything I want supported by variadic replace(). Seems kind of weird, though. Erm, except that @extschema@ is explicitly supported by CREATE EXTENSION, right?

>> Speaking of which, what happens if an extension has no search_path? Or
>> an old explicit one?
>
> No. Please do not try to open this can of worms. It took one entire year
> for us to agree that extension should not concern themselves with schema
> and search_path issues, and 2 developer meetings.

Yeah, except that you *are* making us be concerned with them. I am required to put this line into my extension's .sql file:

SET search_path = @extschema@;

I'd rather not have to.

> So what's in the patch is a very simple way for users to instruct the
> extension about a schema preference choice, which as been asked by
> Heikki and Robert this time. I'll try to come up with the archives link
> later, now ain't a good time for me to crawl the lists.

No, I agree that I should be able to do `CREATE EXTENSION foo WITH SCHEMA bar` and have it work. But it appears to *only* work if the extension author included the above magical line. It feels superfluous to me.

IOW, if I install extension "foo" and it does *not* have the above magic line, then this command will *not* do what I expect:

CREATE EXTENSION foo WITH SCHEMA bar;

Extension "foo" will be in the public schema (usually) rather than "bar".

> The only other acceptable option would be to do nothing with schema at
> install time and to ask users to use the next patch in the series.
>
> CREATE EXTENSION foo;
> ALTER EXTENSION foo SET SCHEMA bar;

Why is that the only other option?

> Now, this would have to fail if more than exactly one schema is
> depending on the extension's foo, and that's it. Considering that most
> (if not all) extensions are using only one schema, it was though nice to
> include the WITH SCHEMA bar; facility at CREATE EXTENSION time, and the
> simple implementation is what you're commenting on.

What I'm suggesting is that extension authors should not be concerned with schemas.

>> All except the last one could be written into the Makefile, and then
>> `make` would generate the control file for installation. Only if the
>> extension author wants to support custom_variable_classes would you
>> need to create an explicit control file. And even that could probably
>> be finessed with some sort of Make variable.
>
> We've been trying to get there, and Tom and Alvaro both said that it's
> not worth the Makefile effort and dance, then Itagaki proposed the
> current mechanism, which is based on extension.control.in files and a
> simple implicit Make rule in pgxs.mk.

You already have key/value replacement of EXTVERSION. What's so different about EXTNAME, EXTSCRIPT, EXTENCODING, and EXTCOMMENT? If I don't need a custom variable class, why bother me with the added complexity of another file with another syntax?

>> BTW, you might want to put the changes to the existing contrib modules
>> in a separate patch, just to simplify things a bit. Not a big deal at
>> this point, but it might help the committer to have further
>> separation. But reviewing that bit, I was wondering, how are the
>> uninstall scripts handled? Are they necessary anymore?
>
> Separating the only use case of the patch away does not sound like
> simplifying anything for me, but maybe that's only because I'm already
> hard at work maintaining 7 git branches (+ master) to be able to issue
> reviewable patches.

Yeah, was just a thought. I expect that everything in contrib can work even if it's not an extension, and could be converted in a later commit. But it's not a big deal to do it up-front.

> As far as the uninstall scripts goes, yes, I think they are not
> necessary anymore and I now realize I didn't remove them. Will wait for
> some more input before to add their removal in the v16 patch.

Cool.

>> ### Questions ###
>>
>> * I assume that if an extension is installed that includes a DSO that
>> the server will need to be HUPed before you can CREATE EXTENSION, yes?
>
> If by DSO you mean dynamic shared object (.so, .dylib or .dll) then no.

Yes, shared object. Don't you need to restart the server to have it pick up a new dso now?

>> * I trust that CREATE EXTENSION and DROP EXTENSION and ALTER EXTENSION
>> are transactional, yes?
>
> Sure. But the patch you're reviewing is not featuring ALTER EXTENSION,
> that's for the next one :)

Right. I kept thinking it should be there, and then remembering it was separate. :-)

>> * Is there a regtype for extensions?
>
> No. Do we need one?

Probably not.

>> * Why can only super users get a list of extensions? It might be
>> useful for me to be able to check from within a udf to see if a given
>> extension is installed, for example. I can see why you wouldn't want
>> to allow listing of extensions that are installed on the system but
>> not in the current database.
>
> Anyone can SELECT FROM pg_extension, the catalog, and so list the
> installed extensions. Same for listing an extension's object.

Well then the limit on the function is arbitrary.

> The superuser only function is the one that has to parse the control
> files. I'm not good about security concerns, but it well seems to me
> that parsing files in $SHAREDIR should be superuser only.

Oh, agreed. Better still would be some way to have a catalog (heh) of available extensions readable from all databases in a cluster. Then, amusingly enough, one wouldn't need control files, either. But maybe that's not possible in PostgreSQL? I'm not sure.

> So I think what you want to happen is what's in the code. Right?

I think so. That function is used by the view, right?

>> * Is there no way to keep a list of available extensions somewhere in
>> the cluster? Seems kind of silly to parse all the control files from
>> within pg_extensions(). That can make a call to pg_extensions() pretty
>> expensive.
>
> Where would you put that cache, and when do you invalidate its entries
> or add new ones? I don't think that listing available extensions is such
> a frequent operation that it warrant for its own cache, even if you're
> able to come up with a good enough management rule set.

No idea. I know zilch about that stuff.

> Oh, and I just did select * from pg_extensions(); 3 times here, to be
> sure we're on the same page. Time: 166,868 ms then 7,823 then 3,584.

That's pretty damned slow.

>> * I like that I can see a list of installed and available extensions,
>> but I'm not sure that \dx+ is the right command for the
>> latter. Usually + means "more columns to provide more information for
>> the same objects as you saw without the +". Is there any precedent for
>> showing different objects with + than without?
>
> Well, I'm not sure about what the best user interface for it is in psql,
> but the feature is there already:
>
> =# select name, comment, installed from pg_extensions limit 2;
> name | comment | installed
> ---------------+--------------------------------------------+-----------
> adminpack | Administrative functions for PostgreSQL | t
> auto_username | functions for tracking who changed a table | f

Yeah, I'm not sure, either. I like the *functionality of \dx+, I'm just not sure that's the right name for it.

>> * I built my cluster with all the contrib extensions. \dx+ does a nice
>> job of listing what's available. So I ran `CREATE EXTENSION
>> hstore`. It seemed to work great, but was *very* verbose. Every SQL
>> statement was emitted to the terminal. I thought \echo was turned off
>> by CREATE EXTENSION. I was able to use hstore after that; worked
>> great. Yay!
>
> The chatter has been discussed extensively with Alvaro and Tom in the
> pg_execute_from_file() branch already --- sorry about that, but really,
> I though that handing you a single patch would make it easier.
>
> So this chatter is a result of 2 things:
>
> - we're using SPI in CREATE EXTENSION
>
> - hstore issues a WARNING on the => operator creation, and SPI reaction
> to WARNING is to dump the query string, which is the install script
> here.

Ah. SPI isn't able to be more precise about it, eh? Pity. Maybe for hstore we could silence the warning in the install script, eh?

>> * Next I tried dropping it. `DROP EXTENSION ex.citext;` did not
>> work. What if I have citext installed in two schemas and just want to
>> drop one of them? Ah, I see, one cannot have an extension with the
>> same name in different schemas. Currently the schema-qualification is
>> a syntax error, but maybe it should be a little more informative?
>
> Well this point is not decided upon yet. I'm arguing that an extension
> is a database global object, and there's a point to be made for them to
> live in a schema.

Those sound like mutually-contradicting statements to me. But maybe it's because I tend to think of schemas as namespaces, and perhaps they're not?

> Currently, they depend on the schema you've been using at CREATE
> EXTENSION time, and the schema created by the script are depending on
> the extension, and as a result all is working fine and following the
> usual conventions (try drop schema cascade when its owner ain't a
> superuser but an extension has been installed there, e.g.).
>
> It could be that the easiest way to fix the current mess is to remove
> WITH SCHEMA support at CREATE EXTENSION time and have the facility back
> in core with the ALTER EXTENSION SET SCHEMA patch.

It seems to me that if you can put an extension in a schema, then you can put another extension with the same name in another schema. That's how it works for all other schema-residing objects, AFAIK.

>> * The `pg_extension` catalog and `pg_extensions` views are both
>> useful, but too close in name. I found it confusing how similar they
>> are. It seems to me like the `pg_extension` class is well-named and
>> like the other catalog table names, but `pg_extenions` should perhaps
>> be `pg_available_extensions` or something.
>
> Give me the name and I'll update the patch. Well, or this could be
> considered a commiter decision.

Covered above: pg_created_extensions and pg_available_extensions.

>> Creating a Third Party Extension
>> --------------------------------
>>
>> ! CREATE EXTENSION pair;
>> ! NOTICE: Installing extension 'pair' from '/usr/local/pgsql-devel/share/contrib/pair.sql
>> ! ERROR: could not execute sql file: '/usr/local/pgsql-devel/share/contrib/pair.sql'
>>
>> It would be nice if it gave me more information. What failed, exactly?
>> It also fails when I use `WITH SCHEMA public`.
>
> Yeah, that's a byproduct of using SPI_execute() in pg_execute_sql_file.
> Your script contains BEGIN; and COMMIT; commands. It's not supposed to.
> You won't find a single contrib module containing those either, with or
> without my patch.

Ah! Yep, that was the problem. Still, it would be great if I could get better feedback as to what the error actually is. I'm not sure I ever would have figured it out myself, given that it works just fine if I install it the old-fashioned way (psql -f).

>> * One could specify stuff *only* in the `Makefile` and let `make`
>> create a control file.
>
> Been trying, failed. Sorry.

Keep trying man! I got your back!

>> * I could omit the @extschema@ business altogether and just let CREATE
>> EXTENSION set the path for the duration of its execution.
>
> That won't fly.

Why not?

> The only workable solution is a 2-steps recipe:
> CREATE EXTENSION pair;
> ALTER EXTENSION pair SET SCHEMA utils;
>
> And the second command will raise an error if 'pair' has registered
> dependencies for more than exactly one schema. Some work needs to be
> done to see that happen, though.

This seems orthogonal to me.

> I'm pleased to see that you mean 'Waiting on Author', because I intend
> to answer timely enough to still hope for the patch to make it in this
> commitfest, baring showstoppers.

D'oh! Sorry, yes, that's what I meant. I think I made the same mistake the last time I did a commitfest review. Duh. Thanks for changing it in the cf app.

> If you follow the git repository you will notice that the doc changes
> asked for in this review are pushed. I'll wait some more for us to
> decide on remaining points before to produce yet another patch version,
> though.

Sounds like a plan.

Best,

David

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> Overall I think the docs could use a lot more fleshing out. Some of
> the stuff in the wiki would help a lot. At some point, though, I'll
> work over the docs myself and either send a patch to you or to the
> list (if it has been committed to core).

Ok, I'll leave it to you then.

> You write a very simple contrib module exclusively for testing. It
> doesn't have to actually do anything other than create a couple of
> objects. A domain perhaps.

What about unaccent? Or lo (1 domain, 2 functions)?

> Yeah, I haven't tried the dump and restore stuff. Are there no
> dump/restore tests in core? If there are, I expect all you'd have to
> do is add an extension or two to be dumped and restored.

Not that I know of. Grep finished with no matches found.
grep -nH -re pg_dump src/test

> I think the catalog should be pg_created_extensions (or
> pg_installed_extensions) and the view should be
> pg_available_extensions.

Well I don't see the point into changing both of them, so I'll change
only the view name.

> Is there no way to have a catalog table visible to *all* databases
> that contains the list of available extensions?

That's called a shared catalog. I don't see any benefit of having to
maintain that when we do already have a directory containing the files
and the restriction that extensions names are the file names.

Again, if you really want to have that, you have to first detail how and
you fill in the shared catalog, and update it.

> Yes, I think if you just mentioned that the comment is used for an
> implicit CREATE COMMENT that it would be useful. That's true, yes? The
> others are okay as-is.

Will do that too, ok. As soon as I'm able to git pull :)

> Okay. I think I see the need to differentiate between a variable in
> .control.in replaced by `make` and one replaced by `CREATE
> EXTENSION`. Something explaining that might be good.

That's documenting the building process and I guess should go into the
PGXS section of the manual, right?

>> Updates on the manual style paragraph to explain that are welcome. It
>> could be that what's needed is a better overview of the whole thing
>> somewhere, but I though what's written would be enough. It seems to be
>> only enough when you already know what it's all about --- and that's
>> typical of Unix style man pages. Do we want to include an extension
>> author tutorial?
>
> Yes!

Would you include that in the documentation rework and patch you're
talking about or expect me to care about it? :)

>> MODULES = autoinc insert_username moddatetime refint timetravel
>> EXTENSION = autoinc auto_username moddatetime refint timetravel
>> EXTVERSION = $(VERSION)
>>
> No, wait, you have five *extensions* above (but only one version
> number). Why not just make it *on* extension in five scripts? That
> would be less confusing to me, given that you can have only one
> version number, it appears.

One script, one extension. If you want to manage things otherwise
internally, it's easy enough, either with a Make rule using cat, or
using pg_execute_from_file() from your main script.

Now, the contrib/spi directory really does contain 5 different
extensions. I don't see a good reason not to support that.

> Yes. Change the error message to "This function can only be called from SQL
> files executed by CREATE EXTENSION".

Thanks for the wording, I'd only change files to script (singular).

> It's probably fine, given the precedent of MODULE_PATHNAME. I'd rather
> see the .control file killed from the distribution altogether (that
> is, generated by `make`).

It only works fine when you have a single Makefile for a single
extension. As soon as you want more than one extension managed by a
single Makefile, it's an horror story.

So we need a way to indicate in the Makefile that we want the building
process to create the control file from the Make variables. I'm not sure
if the non-existence of a .control.in or a .control file is enough a
clue, and I remember about contorted Make rules. Will try again.

> Oh, so they're not defined placeholders. I can do anything I want
> supported by variadic replace(). Seems kind of weird, though. Erm,
> except that @extschema@ is explicitly supported by CREATE EXTENSION,
> right?

Yes, this very name is hard-coded.

> SET search_path = @extschema@;
>
> I'd rather not have to.
[...]
> IOW, if I install extension "foo" and it does *not* have the above
> magic line, then this command will *not* do what I expect:
>
> CREATE EXTENSION foo WITH SCHEMA bar;
>
> Extension "foo" will be in the public schema (usually) rather than "bar".

Well, before that you had to explicitly write public in there, which IMO
is so much worse. Then again, I now think that the right way to approach
that is to remove this feature. The user would have a 2-steps operation
instead, but that works right always.

In this idea, CREATE EXTENSION will always create objects in the schema
that is hard-coded in the script file, but once this is done, it's quite
easy to check about them all being in the same place.

BEGIN;
CREATE EXTENSION citext;
ALTER EXTENSION citext SET SCHEMA utils;
COMMIT;

In the future we might want to extend this command to support for moving
things in more than one schema at a time. I'm just thinking the need is
not there though.

ALTER EXTENSION name SET SCHEMA foo TO bar, baz TO quux;

> Yeah, was just a thought. I expect that everything in contrib can work
> even if it's not an extension, and could be converted in a later
> commit. But it's not a big deal to do it up-front.

Things that are not extensions in contrib are things that don't have any
SQL script, so that they're not creating objects in a database. That's it.

> Yes, shared object. Don't you need to restart the server to have it
> pick up a new dso now?

No, that's only when you want to preload the module (that's how a .so
file is called) in shared_preload_libraries. Other than that the module
is loaded on-demand, and per-session.

> Oh, agreed. Better still would be some way to have a catalog (heh) of
> available extensions readable from all databases in a cluster. Then,
> amusingly enough, one wouldn't need control files, either. But maybe
> that's not possible in PostgreSQL? I'm not sure.

You have to think about the whole install process of extensions,
here. CREATE EXTENSION is only the last step, it requires you have
already installed the files in the system. The control file allows us to
get meta-data about the extension for registering it into the system (to
get an OID to depend upon), then you run the SQL script that has to be
there in $PGDATA/contrib, then when you use the objects installed, you
might need to load a module (.so) that has to be found in $libdir.

Now you're saying that we could bypass the control file. That would mean
that to do the extension registering, the extension author would have to
provide for something else. Maybe another SQL command. That was the idea
I had a long time ago, until Heikki proposed the current scheme over a
bear at the Royal Oak, and I don't want to make a step backward :)

> I think so. That function is used by the view, right?

From psql \d

View definition:
SELECT e.name, e.version, e.custom_variable_classes, e.comment, e.installed
FROM pg_extensions() e(name, version, custom_variable_classes, comment, installed);

> Ah. SPI isn't able to be more precise about it, eh? Pity. Maybe for
> hstore we could silence the warning in the install script, eh?

Mmm, set client_encoding to ERROR around creating the operator would
work, but that would still fill the logs. Do we want to suppress this
warning here?

> It seems to me that if you can put an extension in a schema, then you
> can put another extension with the same name in another schema. That's
> how it works for all other schema-residing objects, AFAIK.

Extensions create objects that are schema-qualified, but are not
themselves. You don't put an extension in a schema, you change the
schema where the extension's objects live.

> Ah! Yep, that was the problem. Still, it would be great if I could get
> better feedback as to what the error actually is. I'm not sure I ever
> would have figured it out myself, given that it works just fine if I
> install it the old-fashioned way (psql -f).

I'd need some input on how to fix SPI to support that.

>>> * I could omit the @extschema@ business altogether and just let CREATE
>>> EXTENSION set the path for the duration of its execution.
>>
>> That won't fly.
>
> Why not?

What you want is to know that the script did create its objects into the
schema you're giving, but there's no reason the script is not
hard-coding the schema, or using more than one. Back to the 2-steps idea.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr

On Dec 7, 2010, at 8:00 AM, Dimitri Fontaine wrote:

>> You write a very simple contrib module exclusively for testing. It
>> doesn't have to actually do anything other than create a couple of
>> objects. A domain perhaps.
>
> What about unaccent? Or lo (1 domain, 2 functions)?

Sure. Doesn't have to actually do anything.

>> Yeah, I haven't tried the dump and restore stuff. Are there no
>> dump/restore tests in core? If there are, I expect all you'd have to
>> do is add an extension or two to be dumped and restored.
>
> Not that I know of. Grep finished with no matches found.
> grep -nH -re pg_dump src/test

Pity.

>> I think the catalog should be pg_created_extensions (or
>> pg_installed_extensions) and the view should be
>> pg_available_extensions.
>
> Well I don't see the point into changing both of them, so I'll change
> only the view name.

Just to make it less ambiguous

>> Is there no way to have a catalog table visible to *all* databases
>> that contains the list of available extensions?
>
> That's called a shared catalog. I don't see any benefit of having to
> maintain that when we do already have a directory containing the files
> and the restriction that extensions names are the file names.

Because then you don't need control files at all.

> Again, if you really want to have that, you have to first detail how and
> you fill in the shared catalog, and update it.

`make install` should do it. From variables in the Makefile.

>> Okay. I think I see the need to differentiate between a variable in
>> .control.in replaced by `make` and one replaced by `CREATE
>> EXTENSION`. Something explaining that might be good.
>
> That's documenting the building process and I guess should go into the
> PGXS section of the manual, right?

Or in the "putting it all together" section.

>>> Updates on the manual style paragraph to explain that are welcome. It
>>> could be that what's needed is a better overview of the whole thing
>>> somewhere, but I though what's written would be enough. It seems to be
>>> only enough when you already know what it's all about --- and that's
>>> typical of Unix style man pages. Do we want to include an extension
>>> author tutorial?
>>
>> Yes!
>
> Would you include that in the documentation rework and patch you're
> talking about or expect me to care about it? :)

Possibly. I'm not going to do it this week; seems like there are some issues that still need shaking out in the implementation, to judge from the "pg_execute_from_file review" thread.

>>> MODULES = autoinc insert_username moddatetime refint timetravel
>>> EXTENSION = autoinc auto_username moddatetime refint timetravel
>>> EXTVERSION = $(VERSION)
>>>
>> No, wait, you have five *extensions* above (but only one version
>> number). Why not just make it *on* extension in five scripts? That
>> would be less confusing to me, given that you can have only one
>> version number, it appears.
>
> One script, one extension. If you want to manage things otherwise
> internally, it's easy enough, either with a Make rule using cat, or
> using pg_execute_from_file() from your main script.
>
> Now, the contrib/spi directory really does contain 5 different
> extensions. I don't see a good reason not to support that.

Each would get a separate control file. The mapping of one version number to multiple extensions is potentially confusing.

>> Yes. Change the error message to "This function can only be called from SQL
>> files executed by CREATE EXTENSION".
>
> Thanks for the wording, I'd only change files to script (singular).

Sure, that works.

>> It's probably fine, given the precedent of MODULE_PATHNAME. I'd rather
>> see the .control file killed from the distribution altogether (that
>> is, generated by `make`).
>
> It only works fine when you have a single Makefile for a single
> extension. As soon as you want more than one extension managed by a
> single Makefile, it's an horror story.

Why is that? We currently manage multiple script files, test files, etc. in a single Makefile. Wildcard operators are very useful for this sort of thing.

> So we need a way to indicate in the Makefile that we want the building
> process to create the control file from the Make variables. I'm not sure
> if the non-existence of a .control.in or a .control file is enough a
> clue, and I remember about contorted Make rules. Will try again.

Why wouldn't that be enough of a clue?

>> Oh, so they're not defined placeholders. I can do anything I want
>> supported by variadic replace(). Seems kind of weird, though. Erm,
>> except that @extschema@ is explicitly supported by CREATE EXTENSION,
>> right?
>
> Yes, this very name is hard-coded.
>
>> SET search_path = @extschema@;
>>
>> I'd rather not have to.
> [...]
>> IOW, if I install extension "foo" and it does *not* have the above
>> magic line, then this command will *not* do what I expect:
>>
>> CREATE EXTENSION foo WITH SCHEMA bar;
>>
>> Extension "foo" will be in the public schema (usually) rather than "bar".
>
> Well, before that you had to explicitly write public in there, which IMO
> is so much worse. Then again, I now think that the right way to approach
> that is to remove this feature. The user would have a 2-steps operation
> instead, but that works right always.

Yes, that would be preferable, but a one-step operation would of course be ideal.

> In this idea, CREATE EXTENSION will always create objects in the schema
> that is hard-coded in the script file, but once this is done, it's quite
> easy to check about them all being in the same place.

If there is no hard-coded schema in the extension script, what would it do? First schema in search_path, I presume.

> BEGIN;
> CREATE EXTENSION citext;
> ALTER EXTENSION citext SET SCHEMA utils;
> COMMIT;
>
> In the future we might want to extend this command to support for moving
> things in more than one schema at a time. I'm just thinking the need is
> not there though.
>
> ALTER EXTENSION name SET SCHEMA foo TO bar, baz TO quux;

Perhaps. v2, eh? ;-P

>> Yeah, was just a thought. I expect that everything in contrib can work
>> even if it's not an extension, and could be converted in a later
>> commit. But it's not a big deal to do it up-front.
>
> Things that are not extensions in contrib are things that don't have any
> SQL script, so that they're not creating objects in a database. That's it.

Sure, not the point I was making, but it's not important.

>> Yes, shared object. Don't you need to restart the server to have it
>> pick up a new dso now?
>
> No, that's only when you want to preload the module (that's how a .so
> file is called) in shared_preload_libraries. Other than that the module
> is loaded on-demand, and per-session.

Some do require shared_preload_libraries, no?

>> Oh, agreed. Better still would be some way to have a catalog (heh) of
>> available extensions readable from all databases in a cluster. Then,
>> amusingly enough, one wouldn't need control files, either. But maybe
>> that's not possible in PostgreSQL? I'm not sure.
>
> You have to think about the whole install process of extensions,
> here. CREATE EXTENSION is only the last step, it requires you have
> already installed the files in the system. The control file allows us to
> get meta-data about the extension for registering it into the system (to
> get an OID to depend upon), then you run the SQL script that has to be
> there in $PGDATA/contrib, then when you use the objects installed, you
> might need to load a module (.so) that has to be found in $libdir.

Sure. Just s/control file/shared catalog/.

> Now you're saying that we could bypass the control file. That would mean
> that to do the extension registering, the extension author would have to
> provide for something else. Maybe another SQL command. That was the idea
> I had a long time ago, until Heikki proposed the current scheme over a
> bear at the Royal Oak, and I don't want to make a step backward :)

Why? If the metadata is in the Makefile (or META.json), then `make install` could use it to populate the shared catalog.

>> I think so. That function is used by the view, right?
>
> From psql \d
>
> View definition:
> SELECT e.name, e.version, e.custom_variable_classes, e.comment, e.installed
> FROM pg_extensions() e(name, version, custom_variable_classes, comment, installed);
>
>> Ah. SPI isn't able to be more precise about it, eh? Pity. Maybe for
>> hstore we could silence the warning in the install script, eh?
>
> Mmm, set client_encoding to ERROR around creating the operator would
> work, but that would still fill the logs. Do we want to suppress this
> warning here?

SET client_min_messages TO warning;
SET log_min_messages TO warning;

Though I think I'd rather that the warning still went to the log.

>> It seems to me that if you can put an extension in a schema, then you
>> can put another extension with the same name in another schema. That's
>> how it works for all other schema-residing objects, AFAIK.
>
> Extensions create objects that are schema-qualified, but are not
> themselves. You don't put an extension in a schema, you change the
> schema where the extension's objects live.

Yes. That's rather confusing IMHO. Not sure of a better way, though.

>> Ah! Yep, that was the problem. Still, it would be great if I could get
>> better feedback as to what the error actually is. I'm not sure I ever
>> would have figured it out myself, given that it works just fine if I
>> install it the old-fashioned way (psql -f).
>
> I'd need some input on how to fix SPI to support that.

Yeah, that would be really helpful.

>>>> * I could omit the @extschema@ business altogether and just let CREATE
>>>> EXTENSION set the path for the duration of its execution.
>>>
>>> That won't fly.
>>
>> Why not?
>
> What you want is to know that the script did create its objects into the
> schema you're giving, but there's no reason the script is not
> hard-coding the schema, or using more than one. Back to the 2-steps idea.

And it may be that the extension needs to be in more than one schema, as Tom pointed out. Better for the common case, though, would be to limit the extension objects to a single schema. Then extension scripts wouldn't need the set search_path statement, or they would be ignored by CREATE EXTENSION.

Best,

David

On Tue, Dec 7, 2010 at 4:29 PM, David E. Wheeler <david(at)kineticode(dot)com> wrote:
>>> IOW, if I install extension "foo" and it does *not* have the above
>>> magic line, then this command will *not* do what I expect:
>>>
>>>    CREATE EXTENSION foo WITH SCHEMA bar;
>>>
>>> Extension "foo" will be in the public schema (usually) rather than "bar".
>>
>> Well, before that you had to explicitly write public in there, which IMO
>> is so much worse. Then again, I now think that the right way to approach
>> that is to remove this feature. The user would have a 2-steps operation
>> instead, but that works right always.
>
> Yes, that would be preferable, but a one-step operation would of course be ideal.

I think this so-called two-step approach is pretty ugly.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I think this so-called two-step approach is pretty ugly.

Well it does not need to be exposed to the user, thinking about it, as
proposed in the other thread. Other than that, you're argument here is
exactly the same as the ones saying that VACUUM or Hint Bints are
bad. It's just that if you want correctness, you don't have anything
better.

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

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
>> What about unaccent? Or lo (1 domain, 2 functions)?
>
> Sure. Doesn't have to actually do anything.

Ok, so that's already in the patch :)

>> That's called a shared catalog. I don't see any benefit of having to
>> maintain that when we do already have a directory containing the files
>> and the restriction that extensions names are the file names.
>
> Because then you don't need control files at all.
>
>> Again, if you really want to have that, you have to first detail how and
>> you fill in the shared catalog, and update it.
>
> `make install` should do it. From variables in the Makefile.

I see that you're not too much into packaging, but here, we don't ever
use `make install` on a production machine. This step happens on the
packaging server, then we install and QA the stuff, then the package
gets installed on the servers where we need it.

Also, I don't see how make install is going to know which cluster it
should talk to — it's quite easy and typicall to run this command on a
server where you have several major versions installed, and several
clusters per major version.

So, again, the data that you so want to remove from the control files I
have no idea at all where to put it.

> Possibly. I'm not going to do it this week; seems like there are some
> issues that still need shaking out in the implementation, to judge
> from the "pg_execute_from_file review" thread.

Yeah, dust ain't settled completely yet… working on that.

> Each would get a separate control file. The mapping of one version
> number to multiple extensions is potentially confusing.

Funny, each already get a separate control file now.

$ ls contrib/spi/*control.in
autoinc.control.in auto_username.control.in moddatetime.control.in
refint.control.in timetravel.control.in

Then the idea behind the version number in the Makefile is that you
generally are maintaining it there and don't want to have to maintain it
in more than one place.

> Why is that? We currently manage multiple script files, test files,
> etc. in a single Makefile. Wildcard operators are very useful for this
> sort of thing.

Well, that was you saying just above that using the same EXTVERSION Make
variable for more than one control file is "potentially confusing". What
about using all the other variables in the same way?

>> Well, before that you had to explicitly write public in there, which IMO
>> is so much worse. Then again, I now think that the right way to approach
>> that is to remove this feature. The user would have a 2-steps operation
>> instead, but that works right always.
>
> Yes, that would be preferable, but a one-step operation would of
> course be ideal.

Thinking about it, as proposed in the other thread, I now think that the
2-steps operation could be internal and not user exposed.

>> ALTER EXTENSION name SET SCHEMA foo TO bar, baz TO quux;
>
> Perhaps. v2, eh? ;-P

Yes please :)

> Some do require shared_preload_libraries, no?

One of them only, pg_stat_statements.

> SET client_min_messages TO warning;
> SET log_min_messages TO warning;
>
> Though I think I'd rather that the warning still went to the log.

(that's about hstore verbosity) ok will see about changing
client_min_messages around the CREATE OPERATOR =>.

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

On Wed, Dec 8, 2010 at 4:19 AM, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I think this so-called two-step approach is pretty ugly.
>
> Well it does not need to be exposed to the user, thinking about it, as
> proposed in the other thread. Other than that, you're argument here is
> exactly the same as the ones saying that VACUUM or Hint Bints are
> bad. It's just that if you want correctness, you don't have anything
> better.

Exposing it to the user is what I think is ugly.

It's also worth noting that ALTER EXTENSION .. SET SCHEMA does NOT
guarantee a correct relocation, because someone might have done ALTER
FUNCTION .. SET search_path = @extschema@, and that's not going to get
properly fixed up. I'm coming to the conclusion more and more that
ALTER EXTENSION .. SET SCHEMA just can't work reliably.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> It's also worth noting that ALTER EXTENSION .. SET SCHEMA does NOT
> guarantee a correct relocation, because someone might have done ALTER
> FUNCTION .. SET search_path = @extschema@, and that's not going to get
> properly fixed up. I'm coming to the conclusion more and more that
> ALTER EXTENSION .. SET SCHEMA just can't work reliably.

Dimitri's last reply to me
http://archives.postgresql.org/message-id/87r5ds1v4q.fsf@hi-media-techno.com
suggests that what he has in mind is to define a relocatable extension
as one that can be relocated ;-), ie it does not contain any such
gotchas. Maybe this is too ugly in itself, or not useful enough to be
worth doing. But it doesn't seem technically unworkable to me, so long
as relocatability is made an explicitly declared property of extensions.
It's certainly true that a large fraction of contrib modules should be
relocatable in that sense, because they just contain C functions that
aren't going to care.

Or are you complaining that somebody could break relocatability after
the fact by altering the contained objects? Sure, but he could break
the extension in any number of other ways as well by making such
alterations. The answer to that is privilege checks, and superusers
being presumed to know what they're doing.

regards, tom lane

On Wed, Dec 8, 2010 at 12:25 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> It's also worth noting that ALTER EXTENSION .. SET SCHEMA does NOT
>> guarantee a correct relocation, because someone might have done ALTER
>> FUNCTION .. SET search_path = @extschema@, and that's not going to get
>> properly fixed up.  I'm coming to the conclusion more and more that
>> ALTER EXTENSION .. SET SCHEMA just can't work reliably.
>
> Dimitri's last reply to me
> http://archives.postgresql.org/message-id/87r5ds1v4q.fsf@hi-media-techno.com
> suggests that what he has in mind is to define a relocatable extension
> as one that can be relocated ;-), ie it does not contain any such
> gotchas.  Maybe this is too ugly in itself, or not useful enough to be
> worth doing.  But it doesn't seem technically unworkable to me, so long
> as relocatability is made an explicitly declared property of extensions.
> It's certainly true that a large fraction of contrib modules should be
> relocatable in that sense, because they just contain C functions that
> aren't going to care.

I don't find that a very satisfying solution, but I guess we could do
it that way.

> Or are you complaining that somebody could break relocatability after
> the fact by altering the contained objects?  Sure, but he could break
> the extension in any number of other ways as well by making such
> alterations.  The answer to that is privilege checks, and superusers
> being presumed to know what they're doing.

I wasn't complaining about this.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Exposing it to the user is what I think is ugly.

Ok, and the current idea fixes that! :)

> It's also worth noting that ALTER EXTENSION .. SET SCHEMA does NOT
> guarantee a correct relocation, because someone might have done ALTER
> FUNCTION .. SET search_path = @extschema@, and that's not going to get
> properly fixed up. I'm coming to the conclusion more and more that
> ALTER EXTENSION .. SET SCHEMA just can't work reliably.

For starters, an extension's script that requires an @extschema@
property is to be marked non-relocatable, so the command here would just
error out. Then again, should the extension's author forgot to mark it
relocatable, the @extschema@ placeholder is not replaced in the current
proposal, so that means that the objects schema are all hard-coded in
the script.

Then, if the superuser thinks it's a good idea to break the extension
after it's been installed, I'm not sure we can do anything about it.

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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Dimitri's last reply to me
> http://archives.postgresql.org/message-id/87r5ds1v4q.fsf@hi-media-techno.com
> suggests that what he has in mind is to define a relocatable extension
> as one that can be relocated ;-), ie it does not contain any such
> gotchas. Maybe this is too ugly in itself, or not useful enough to be
> worth doing. But it doesn't seem technically unworkable to me, so long
> as relocatability is made an explicitly declared property of extensions.

Well it does not seem to be complex to code. It's about having a new
property in the control file, relocatable, boolean. This property is
required and controls the behavior of the CREATE EXTENSION ... WITH
SCHEMA command. When true we use the ALTER EXTENSION SET SCHEMA code
path and when false, the placeholder replacement code path. The ALTER
command has already been developed so I need to merge it into the main
patch.

The ALTER EXTENSION SET SCHEMA command needs to be adapted so that it
checks that all the extension's objects are currently in the same schema
and error out if that's not the case.

I'm not going be able to deliver a new patch including that and the
other changes required by David Wheeler's review by tonight, but by
Friday's evening seems like a reasonable target.

> It's certainly true that a large fraction of contrib modules should be
> relocatable in that sense, because they just contain C functions that
> aren't going to care.

As they all currently are using the SET search_path TO public; trick, I
think they are all relocatable as is and all I need is to remove that
line (and add the property to the control file).

> Or are you complaining that somebody could break relocatability after
> the fact by altering the contained objects? Sure, but he could break
> the extension in any number of other ways as well by making such
> alterations. The answer to that is privilege checks, and superusers
> being presumed to know what they're doing.

+1

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

On Dec 8, 2010, at 1:39 AM, Dimitri Fontaine wrote:

> "David E. Wheeler" <david(at)kineticode(dot)com> writes:
>>> What about unaccent? Or lo (1 domain, 2 functions)?
>>
>> Sure. Doesn't have to actually do anything.
>
> Ok, so that's already in the patch :)

No, it's not. There are no unit tests at all. You can call the contrib modules and their tests acceptance tests, but that's not the same thing.

> I see that you're not too much into packaging, but here, we don't ever
> use `make install` on a production machine. This step happens on the
> packaging server, then we install and QA the stuff, then the package
> gets installed on the servers where we need it.
>
> Also, I don't see how make install is going to know which cluster it
> should talk to — it's quite easy and typicall to run this command on a
> server where you have several major versions installed, and several
> clusters per major version.

Yeah, one installs extensions into a PostgreSQL instal, not a cluster. I get that.

> So, again, the data that you so want to remove from the control files I
> have no idea at all where to put it.

Okay, keep the installed control files. But don't make me distribute them unless absolutely necessary.

>> Possibly. I'm not going to do it this week; seems like there are some
>> issues that still need shaking out in the implementation, to judge
>> from the "pg_execute_from_file review" thread.
>
> Yeah, dust ain't settled completely yet… working on that.

Right.

>> Each would get a separate control file. The mapping of one version
>> number to multiple extensions is potentially confusing.
>
> Funny, each already get a separate control file now.
>
> $ ls contrib/spi/*control.in
> autoinc.control.in auto_username.control.in moddatetime.control.in
> refint.control.in timetravel.control.in
>
> Then the idea behind the version number in the Makefile is that you
> generally are maintaining it there and don't want to have to maintain it
> in more than one place.

Sure. But you're mandating one version even if you have multiple extensions. That's the potentially confusing part.

>> Why is that? We currently manage multiple script files, test files,
>> etc. in a single Makefile. Wildcard operators are very useful for this
>> sort of thing.
>
> Well, that was you saying just above that using the same EXTVERSION Make
> variable for more than one control file is "potentially confusing". What
> about using all the other variables in the same way?

What? I don't follow what you're saying.

>> Yes, that would be preferable, but a one-step operation would of
>> course be ideal.
>
> Thinking about it, as proposed in the other thread, I now think that the
> 2-steps operation could be internal and not user exposed.

Maybe. I'm still not convinced that you need the replace() stuff at all, though I can see the utility of it.

>> Some do require shared_preload_libraries, no?
>
> One of them only, pg_stat_statements.

In contrib. You seem to forget that there are a lot of third-party extensions out there already.

>> SET client_min_messages TO warning;
>> SET log_min_messages TO warning;
>>
>> Though I think I'd rather that the warning still went to the log.
>
> (that's about hstore verbosity) ok will see about changing
> client_min_messages around the CREATE OPERATOR =>.

I would much rather retain that warning -- everyone should know about it -- and somehow convince SPI to be much less verbose in reporting issues. It should specify where the error came from (which query) and what the error actually is.

Best,

David

On Dec 8, 2010, at 12:18 PM, Dimitri Fontaine wrote:

>> It's certainly true that a large fraction of contrib modules should be
>> relocatable in that sense, because they just contain C functions that
>> aren't going to care.
>
> As they all currently are using the SET search_path TO public; trick, I
> think they are all relocatable as is and all I need is to remove that
> line (and add the property to the control file).

+1 This alone would be a huge improvement, solving my complaint about @extschema@ for the vast majority of cases. It's reasonable to make the author of a multi-schema extension or an extension that cannot be moved do more work.

Best,

David

Kineticode Billing <david(at)kineticode(dot)com> writes:
> No, it's not. There are no unit tests at all. You can call the contrib
> modules and their tests acceptance tests, but that's not the same
> thing.

Ok, I need some more guidance here. All contrib extension (there are 38
of them) are using the CREATE EXTENSION command and checking the result
with the pg_regress framework. What are we missing?

I can see about adding DROP EXTENSION for all the tests, but that's
about it.

> Okay, keep the installed control files. But don't make me distribute
> them unless absolutely necessary.

Yes you have to distribute them, that's necessary. Sorry about that.

>> Then the idea behind the version number in the Makefile is that you
>> generally are maintaining it there and don't want to have to maintain it
>> in more than one place.
>
> Sure. But you're mandating one version even if you have multiple
> extensions. That's the potentially confusing part.

I see how confusing it is, because what you say ain't true. You can
always put different version numbers in the control file and even skip
the rule to produce the .control from the .control.in by providing the
.control directly. That's just a facility here.

>>> Why is that? We currently manage multiple script files, test files,
>>> etc. in a single Makefile. Wildcard operators are very useful for this
>>> sort of thing.
>>
>> Well, that was you saying just above that using the same EXTVERSION Make
>> variable for more than one control file is "potentially confusing". What
>> about using all the other variables in the same way?
>
> What? I don't follow what you're saying.

You're complaining that a single EXTVERSION applied to more than one
extension's control file is confusing. What if we had EXTCOMMENT and
EXTRELOCATABLE in there too? What exactly are you expecting the Makefile
to look like?

> In contrib. You seem to forget that there are a lot of third-party
> extensions out there already.

True. That's still not the common case, and it's still covered the same
way as before, you need to restart to attach to shared memory.

> I would much rather retain that warning -- everyone should know about
> it -- and somehow convince SPI to be much less verbose in reporting
> issues. It should specify where the error came from (which query) and
> what the error actually is.

The problem is much more complex than that and could well kill the patch
if we insist on fixing it as part of the extension's work, v1. The
problem is exposing more internals of the SQL parser into SPI so that
you can send a bunch of queries in an explicit way. Mind you, the
firsts version of the patch had something like that in there, but that
wouldn't have supported this use case. I've been told to simply use SPI
there.

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

On Dec 8, 2010, at 12:42 PM, Dimitri Fontaine wrote:

> Kineticode Billing <david(at)kineticode(dot)com> writes:
>> No, it's not. There are no unit tests at all. You can call the contrib
>> modules and their tests acceptance tests, but that's not the same
>> thing.
>
> Ok, I need some more guidance here. All contrib extension (there are 38
> of them) are using the CREATE EXTENSION command and checking the result
> with the pg_regress framework. What are we missing?

unit tests. You add a bunch of functions. You need to test those functions.

> I can see about adding DROP EXTENSION for all the tests, but that's
> about it.

If you add that, you'll also need something to CREATE EXTENSION with, eh? And also, tests to make sure that WITH SCHEMA works properly (however that shakes out).

>> Okay, keep the installed control files. But don't make me distribute
>> them unless absolutely necessary.
>
> Yes you have to distribute them, that's necessary. Sorry about that.

I don't see why. Most of them are dead simple and could easily be Makefile variables.

>> Sure. But you're mandating one version even if you have multiple
>> extensions. That's the potentially confusing part.
>
> I see how confusing it is, because what you say ain't true. You can
> always put different version numbers in the control file and even skip
> the rule to produce the .control from the .control.in by providing the
> .control directly. That's just a facility here.

I see, okay.

>> What? I don't follow what you're saying.
>
> You're complaining that a single EXTVERSION applied to more than one
> extension's control file is confusing. What if we had EXTCOMMENT and
> EXTRELOCATABLE in there too? What exactly are you expecting the Makefile
> to look like?

Mostly these will all have only one setting. In more complex cases perhaps one *would* be required to distribute a control file.

>> In contrib. You seem to forget that there are a lot of third-party
>> extensions out there already.
>
> True. That's still not the common case, and it's still covered the same
> way as before, you need to restart to attach to shared memory.

Okay.

>> I would much rather retain that warning -- everyone should know about
>> it -- and somehow convince SPI to be much less verbose in reporting
>> issues. It should specify where the error came from (which query) and
>> what the error actually is.
>
> The problem is much more complex than that and could well kill the patch
> if we insist on fixing it as part of the extension's work, v1. The
> problem is exposing more internals of the SQL parser into SPI so that
> you can send a bunch of queries in an explicit way. Mind you, the
> firsts version of the patch had something like that in there, but that
> wouldn't have supported this use case. I've been told to simply use SPI
> there.

I agree that SPI should be fixed in a different project/patch. Go with what you've got, it will just highlight the problem with SPI more.

Best,

David

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> I don't see why. Most of them are dead simple and could easily be
> Makefile variables.

And how does the information flows from the Makefile to the production
server, already?

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

On Dec 8, 2010, at 1:53 PM, Dimitri Fontaine wrote:

>> I don't see why. Most of them are dead simple and could easily be
>> Makefile variables.
>
> And how does the information flows from the Makefile to the production
> server, already?

`make` generates the file if it doesn't already exist.

David

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
>> And how does the information flows from the Makefile to the production
>> server, already?
>
> `make` generates the file if it doesn't already exist.

Again, will retry when possible, but it has been a time sink once already.

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

Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr> writes:
> Well it does not seem to be complex to code. It's about having a new
> property in the control file, relocatable, boolean. This property is
> required and controls the behavior of the CREATE EXTENSION ... WITH
> SCHEMA command. When true we use the ALTER EXTENSION SET SCHEMA code
> path and when false, the placeholder replacement code path. The ALTER
> command has already been developed so I need to merge it into the main
> patch.

Ok I've done that on the git branch, for people interested into having a
look or playing with it before the week-end, when I think I'll post the
new patch revision. Well I've left alone the behavior change at CREATE
EXTENSION time, and also, well, the necessary documentation.

http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=summary
http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=commitdiff;h=c31d8a7728706d539f50d764fe8f45db92869664

> The ALTER EXTENSION SET SCHEMA command needs to be adapted so that it
> checks that all the extension's objects are currently in the same schema
> and error out if that's not the case.

Done in the commit above. WIP of course, but just so that commit fest
manager notice things are moving here.

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

On Dec 8, 2010, at 2:07 PM, Dimitri Fontaine wrote:

> "David E. Wheeler" <david(at)kineticode(dot)com> writes:
>>> And how does the information flows from the Makefile to the production
>>> server, already?
>>
>> `make` generates the file if it doesn't already exist.
>
> Again, will retry when possible, but it has been a time sink once already.

Fair enough.

David