Re: Extensions, patch 22 (cleanup, review, cleanup)

Hi,

From last round of review from Robert and Álvaro, here's the patch
version 22. Changes:

- cleanup contrib/ and 'Adjust search_path' comments
- remove contrib/*/uninstall* scripts
- add some documentation to the NO USER DATA option
- remove objects locking in the pg_extension_objects() SRF, per Robert
- propose split patches

It so happens that git allows to prepare diffs for subdirs (or files) of
a tree, so I've been using that to separate away contrib changes from
the others. As a result, 3 patches are attached: full, contrib only,
doc and src only.

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

On Mon, December 20, 2010 22:35, Dimitri Fontaine wrote:
> Hi,
>
> From last round of review from Robert and �lvaro, here's the patch
> version 22. Changes:
>
> - cleanup contrib/ and 'Adjust search_path' comments
> - remove contrib/*/uninstall* scripts
> - add some documentation to the NO USER DATA option
> - remove objects locking in the pg_extension_objects() SRF, per Robert
> - propose split patches
>

I used the 'full' patch.

During configure I spotted this:

[...]
checking for bison... /usr/bin/bison
configure: using bison (GNU Bison) 2.3
gawk: { if ($4 < 1.875-extension) exit 0; else exit 1;}
gawk: ^ syntax error
gawk: { if ($4 < 1.875-extension) exit 0; else exit 1;}
gawk: ^ syntax error
checking for flex... /usr/local/bin/flex
configure: using flex 2.5.3
[...]

Otherwise the patch applies, and compiles, checks, installs and runs OK. (obviously I haven't
tested anything yet)

Linux Centos 5.4

Erik Rijkers

On Mon, December 20, 2010 22:55, Erik Rijkers wrote:
> On Mon, December 20, 2010 22:35, Dimitri Fontaine wrote:

>
> During configure I spotted this:
>
> [...]
> checking for bison... /usr/bin/bison
> configure: using bison (GNU Bison) 2.3
> gawk: { if ($4 < 1.875-extension) exit 0; else exit 1;}
> gawk: ^ syntax error
> gawk: { if ($4 < 1.875-extension) exit 0; else exit 1;}
> gawk: ^ syntax error
> checking for flex... /usr/local/bin/flex
> configure: using flex 2.5.3
> [...]
>

Apologies - please ignore the above ...

It turns out this is an artifact of my build script hacking of the configure file.

> Otherwise the patch applies, and compiles, checks, installs and runs OK. (obviously I haven't
> tested anything yet)
>
> Linux Centos 5.4
>
> Erik Rijkers
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Excerpts from Dimitri Fontaine's message of lun dic 20 18:35:44 -0300 2010:
> Hi,
>
> From last round of review from Robert and Álvaro, here's the patch
> version 22. Changes:

I noticed this bit in the docs:

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.

However, the code to that function contains this bit:

/*
* CREATE EXTENSION is superuser only and we check we're in that code
* path, so we don't add another explicit check for superuser here.
*/
if (!create_extension)
ereport(ERROR,
(errmsg("this function can only be used from CREATE EXTENSION")));

So presumably this shouldn't be documented because it cannot be called
anyway?

To be honest I don't understand the purpose of this part of the patch.

I attach some minor fixes while reading it over. I compiled but didn't
run it :-)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

>> On Mon, December 20, 2010 22:35, Dimitri Fontaine wrote:
>
>

I might be mistaken but it looks like a doc/src/sgml/ref/alter_extension.sgml is missing?

"Erik Rijkers" <er(at)xs4all(dot)nl> writes:
> I might be mistaken but it looks like a doc/src/sgml/ref/alter_extension.sgml is missing?

Mmm, it seems that git was agreeing with you, so here's it:

git ls-files doc/src/sgml/ref/alter_extension.sgml
http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=commitdiff;h=9371a9763651df2636cb6c20dced7cd67398c477

It was already online for readers of the HTML version of the docs:

http://pgsql.tapoueh.org/extensions/doc/html/sql-alterextension.html

And it will appear in next revision of the patch. Thanks!
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> function <link linkend="functions-extension">pg_extension_flag_dump</link>
[...]
> So presumably this shouldn't be documented because it cannot be called
> anyway?

It can be called but only from an extension's script.

> To be honest I don't understand the purpose of this part of the patch.

So the problem we're offering a solution for, here, is the extension
with user data problem: the extension infrastructure is only there so
that pg_dump knows to filter OUT sql objects from its dump, prefering a
single create extension command. Some extension allows users to control
the data in some of they're objects: now you want to have those in the
backup again.

From the docs:

http://pgsql.tapoueh.org/extensions/doc/html/functions-admin.html#FUNCTIONS-EXTENSION

pg_extension_with_user_data allows extension's author to prepare
installation scripts that will work well for initial installation and
when restoring from a pg_dump backup, which issues CREATE EXTENSION
foo WITH NO USER DATA;. See CREATE EXTENSION for details. One way to
use it is as following:

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;
$$;

I don't really know how to improve the docs, you seem to have been
surprised by reading the CREATE EXTENSION docs but you didn't follow the
link to the function's doc with the above details, did you?

I'm open to improving things here, but I'm not seeing how yet.

> I attach some minor fixes while reading it over. I compiled but didn't
> run it :-)

Thanks a lot, that's applied in my git repo, and I did run it
successfully! It will be part of the next patch revision.

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

On Tue, December 21, 2010 09:57, Dimitri Fontaine wrote:
> "Erik Rijkers" <er(at)xs4all(dot)nl> writes:
>> I might be mistaken but it looks like a doc/src/sgml/ref/alter_extension.sgml is missing?
>
> Mmm, it seems that git was agreeing with you, so here's it:
>
> git ls-files doc/src/sgml/ref/alter_extension.sgml
> http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=commitdiff;h=9371a9763651df2636cb6c20dced7cd67398c477
>
> It was already online for readers of the HTML version of the docs:
>
> http://pgsql.tapoueh.org/extensions/doc/html/sql-alterextension.html
>
> And it will appear in next revision of the patch. Thanks!
> --
> Dimitri Fontaine
> http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
>

Two changes to sql-alterextension.sgml:

ALTER EXTENSION name SET EXTENSION new_schema

should be:

ALTER EXTENSION name SET SCHEMA new_schema

And in the 'Description' there are (I think) old copy/paste remnants:

ALTER EXTENSION changes the definition of an existing type. There are only one subforms:
SET SCHEMA

it should be (something like):

ALTER EXTENSION changes an existing extension. There is only one form:
ALTER EXTENSION set schema new_schema

Erik Rijkers

"Erik Rijkers" <er(at)xs4all(dot)nl> writes:
>> http://pgsql.tapoueh.org/extensions/doc/html/sql-alterextension.html
[...]
> Two changes to sql-alterextension.sgml:

Fixed and uploaded on the URL above, will be in the next patch revision,
thanks!

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