Re: Review: Extensions Patch

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