Extension Templates S03E11

On 1 August 2013 18:01, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:

Hi,

Please find attached to this email the latest and greatest version of
in-line SQL only extensions support, known as "Extension Templates" and
which could be renamed "In-Catalog Extension Templates".

I've included a high-level description of the patch in a style that
targets the detailed commit messages for features of that source code
impact level.

The attached patch is known to address all points raised in the previous
reviews and to implement the best design we could come up with, thanks
to immense helping from Tom, Heikki and Markus. Of course, bugs are all
my precious.

I'm going to register that patch to the next commitfest. It's not the
only patch I intend to register for september though, as I want to get
to a usable situation with Event Triggers, so you can expect a series of
patches for that, covering what couldn't make it previously.

As I think this WIP is about as ready-for-committer as it will ever be,
it would be fantastic if we could do a single committer review before
CF2013-09 so that I know that it's going to be accepted… or not. Well at
least it's in the queue already, we'll see what can be done.

Regards,

---
Implement in-catalog Extension Template facility.

Previously, the only way to CREATE EXTENSION involved installing file
system templates in a place generally owned by root: creation scripts,
upgrade scripts, main control file and auxilliary control files. This
patch implements a way to upload all those resources into the catalogs,
so that a PostgreSQL connection is all you need to make an extension
available.

By design and for security concerns the current Extension Template
facility is not able to deal with extensions that need to load a DSO
module into the backend. Using any other PL is supported though.

An extension created from a template depends on it, and the templates
are part of any backup script taken with pg_dump. So that at pg_restore
time, when CREATE EXTENSION is executed the templates are already in
place.

To be able to do that, though, we need a difference in behavior in
between the classic file system level templates and the catalog
templates: there's no dependency tracking happening at all with file
system templates and those can be changed at will even if an extension
has been already instanciated from the templates, or even removed.

Apart from the dependency tracking, the only other difference between
file system templates and catalog templates for extensions is that the
later are managed per-database. The file system level templates being
managed per major version of PostgreSQL is considered a drawback of that
method and not to be immitated by the in-catalog system, more flexible
by design.

At CREATE EXTENSION time, the file system templates are always prefered
to the catalog templates. Also, it's prohibited to make available an
extension in the catalogs if an extension of the same name is already
available from file system templates. That said, some "race conditions"
make it still possible to have the same extension name available as a
file system template and a catalog template. Even if only the former
will ever get installed, it's been deemed prudent to restrict the
in-catalog templates for extensions to superusers only.

Could you please resubmit this without using SnapshotNow as it's no longer
supported?

Thanks

Thom

Thom Brown <thom@linux.com> writes:

Could you please resubmit this without using SnapshotNow as it's no longer
supported?

Sure, sorry that I missed that, please find v12 attached.

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

Hi,

2013-08-04 15:20 keltez�ssel, Dimitri Fontaine �rta:

Thom Brown <thom@linux.com> writes:

Could you please resubmit this without using SnapshotNow as it's no longer
supported?

Sure, sorry that I missed that, please find v12 attached.

Here's a review for this patch.

* Is the patch in a patch format which has context? (eg: context diff format)

Yes.

* Does it apply cleanly to the current git master?

No, it has one reject in src/bin/pg_dump/pg_backup_archiver.c.
It was obvious to fix, version 12a is attached.

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

It has extended the SQL reference nicely but the reference to
<xref linkend="extend-extensions"> was not obvious enough
regarding the list of control parameters.

The SQL syntax has them in allcaps, while the referenced section
has them in lower case. It's easy to miss them while just browsing
for e.g. RELOCATABLE. I had to go back twice to find the proper
part of the text.

I would like to see the control parameters documented in allcaps
in CREATE EXTENSION TEMPLATE. Then ALTER EXTENSION
TEMPLATE should reference the CREATE instead of the longer
text in <xref linkend="extend-extensions">. This xref can still
be there for reference in all three of CREATE/ALTER/DROP
EXTENSION TEMPLATE.

* Does the patch actually implement what it's supposed to do?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

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

There's no such provision in the spec.
As far as I can tell, it follows the community-agreed behavior.

* Does it include pg_dump support (if applicable)?

Yes.

But the version check is already wrong in src/bin/pg_dump/pg_dump.c
since this patch missed 9.3.

+       /*
+        * Before 9.3, there are no extension templates.
+        */
+       if (fout->remoteVersion < 90300)
+       {
+               *numExtensionTemplates = 0;
+               return NULL;
+       }
+

* Are there dangers?

I don't think so.

* Have all the bases been covered?

It seems so.

* Does the feature work as advertised?

Yes.

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

I don't know.

* Are there any assertion failures or crashes?

No.

* Does the patch slow down simple tests?

No.

* If it claims to improve performance, does it?

n/a

* Does it slow down other things?

No.

* Does it follow the project coding guidelines?

Yes.

Nitpicking. This chunk has an extra unnecessary space:

diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
index c4d3f3c..689dc37 100644
--- a/src/backend/catalog/Makefile
+++ b/src/backend/catalog/Makefile
@@ -38,6 +38,7 @@ POSTGRES_BKI_SRCS = $(addprefix $(top_srcdir)/src/include/catalog/,\
         pg_authid.h pg_auth_members.h pg_shdepend.h pg_shdescription.h \
         pg_ts_config.h pg_ts_config_map.h pg_ts_dict.h \
         pg_ts_parser.h pg_ts_template.h pg_extension.h \
+        pg_extension_control.h pg_extension_template.h pg_extension_uptmpl.h \
         pg_foreign_data_wrapper.h pg_foreign_server.h pg_user_mapping.h \
         pg_foreign_table.h \
         pg_default_acl.h pg_seclabel.h pg_shseclabel.h pg_collation.h pg_range.h \

* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

It should.

* Are the comments sufficient and accurate?

Yes.

* Does it do what it says, correctly?

According to the added regression tests, yes.

* Does it produce compiler warnings?

No.

* Can you make it crash?

No.

* Is everything done in a way that fits together coherently with other features/modules?

I think so.

* Are there interdependencies that can cause problems?

I don't know.

Best regards,
Zolt�n B�sz�rm�nyi

--
----------------------------------
Zolt�n B�sz�rm�nyi
Cybertec Sch�nig & Sch�nig GmbH
Gr�hrm�hlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

Hi,

Boszormenyi Zoltan <zb@cybertec.at> writes:

Here's a review for this patch.

Thanks for that review Zoltan!

No, it has one reject in src/bin/pg_dump/pg_backup_archiver.c.
It was obvious to fix, version 12a is attached.

Included in the new version of the patch (v13), attached.

It has extended the SQL reference nicely but the reference to
<xref linkend="extend-extensions"> was not obvious enough
regarding the list of control parameters.

Fixed in the attached.

I would like to see the control parameters documented in allcaps
in CREATE EXTENSION TEMPLATE. Then ALTER EXTENSION
TEMPLATE should reference the CREATE instead of the longer
text in <xref linkend="extend-extensions">. This xref can still
be there for reference in all three of CREATE/ALTER/DROP
EXTENSION TEMPLATE.

I didn't follow exactly your proposal here because I didn't want to have
to maintain the control parameter description list in two different
places. I've still added a detailed list with references and details and
more importantly with coverage of e.g. "NORELOCATABLE" which is not
covered in the referenced material.

But the version check is already wrong in src/bin/pg_dump/pg_dump.c
since this patch missed 9.3.

+ if (fout->remoteVersion < 90300)

Fixed.

Nitpicking. This chunk has an extra unnecessary space:

Fixed.

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

2013-08-27 18:09 keltez�ssel, Dimitri Fontaine �rta:

Hi,

Boszormenyi Zoltan <zb@cybertec.at> writes:

Here's a review for this patch.

Thanks for that review Zoltan!

You're welcome.

I would like to see the control parameters documented in allcaps
in CREATE EXTENSION TEMPLATE. Then ALTER EXTENSION
TEMPLATE should reference the CREATE instead of the longer
text in <xref linkend="extend-extensions">. This xref can still
be there for reference in all three of CREATE/ALTER/DROP
EXTENSION TEMPLATE.

I didn't follow exactly your proposal here because I didn't want to have
to maintain the control parameter description list in two different
places. I've still added a detailed list with references and details and
more importantly with coverage of e.g. "NORELOCATABLE" which is not
covered in the referenced material.

It looks better. Now that the lowercase keywords are expected
by the eye in the referenced text, they will be much easier to find.
Thanks.

Best regards,
Zolt�n B�sz�rm�nyi

--
----------------------------------
Zolt�n B�sz�rm�nyi
Cybertec Sch�nig & Sch�nig GmbH
Gr�hrm�hlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

This doesn't build:

make -C pg_upgrade_support all
make[2]: Entering directory `/var/lib/jenkins/jobs/postgresql_commitfest_world/workspace/contrib/pg_upgrade_support'
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o pg_upgrade_support.o pg_upgrade_support.c -MMD -MP -MF .deps/pg_upgrade_support.Po
pg_upgrade_support.c: In function ‘create_empty_extension’:
pg_upgrade_support.c:193:8: error: too few arguments to function ‘InsertExtensionTuple’
In file included from pg_upgrade_support.c:16:0:
../../src/include/commands/extension.h:60:12: note: declared here
make[2]: *** [pg_upgrade_support.o] Error 1

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Eisentraut <peter_e@gmx.net> writes:

make -C pg_upgrade_support all

Do we have something automated to easily test pg_upgrade?

My memories of how pg_upgrade works with extensions makes me believe
that I don't have anything special to do when those extensions have been
made available through a template: the scripts are not used. That said,
we want to retain some new dependencies…

The contrib/pg_upgrade/IMPLEMENTATION file is silent about upgrading
extensions…

I fixed the pg_upgrade_support compiling in my branch here, please find
the patch-on-patch attached. I also checked that the whole of contribs
now build fine and didn't find any other problem.

Regards,
--
Dimitri Fontaine 06 63 07 10 78
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Dimitri Fontaine wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

make -C pg_upgrade_support all

Do we have something automated to easily test pg_upgrade?

"make check" in contrib/pg_upgrade should do the trick.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

"make check" in contrib/pg_upgrade should do the trick.

PASSED

Even after I added extension to the serial_schedule. I don't know if I
need to do anything specific on that area, will wait about some feedback
on that before sending a new version of the patch. Meanwhile my branch
is updated, and I sent the patch-on-patch too.

Regards,
--
Dimitri Fontaine 06 63 07 10 78
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

"make check" in contrib/pg_upgrade should do the trick.

PASSED

Even after I added extension to the serial_schedule. I don't know if I
need to do anything specific on that area, will wait about some feedback
on that before sending a new version of the patch. Meanwhile my branch
is updated, and I sent the patch-on-patch too.

Here's v14 of the patch with pg_upgrade support fixed, so that the
automated setup that Peter built is able to have at it!

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

On Thu, 2013-08-29 at 12:16 +0200, Dimitri Fontaine wrote:

Here's v14 of the patch with pg_upgrade support fixed, so that the
automated setup that Peter built is able to have at it!

Fails cpluspluscheck:

In file included from /tmp/cpluspluscheck.5g2uWw/test.cpp:3:0:
./src/include/commands/template.h:47:8: error: ‘ExtensionControl’ does not name a type
./src/include/commands/template.h:51:8: error: ‘ExtensionControl’ does not name a type

I think this actually just means the header does not include all it needs by itself.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Eisentraut <peter_e@gmx.net> writes:

Fails cpluspluscheck:

Turns out I'm discovering that particular check, thanks! I could
reproduce and fix the error locally after being led to the command
./src/tools/pginclude/cpluspluscheck.

So please find v15 of the patch attached to this email, that passes all
previously done checks and this one too now.

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

On Mon, Sep 02, 2013 at 02:32:16AM -0400, Peter Eisentraut wrote:

On Thu, 2013-08-29 at 12:16 +0200, Dimitri Fontaine wrote:

Here's v14 of the patch with pg_upgrade support fixed, so that the
automated setup that Peter built is able to have at it!

Fails cpluspluscheck:

In file included from /tmp/cpluspluscheck.5g2uWw/test.cpp:3:0:
./src/include/commands/template.h:47:8: error: ‘ExtensionControl’ does not name a type
./src/include/commands/template.h:51:8: error: ‘ExtensionControl’ does not name a type

I think this actually just means the header does not include all it needs by itself.

Is there some standard set of checks you run on new patches, and are
the results showing up on, say, the buildfarm or some other CI
dashboard?

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Sep 3, 2013 at 4:20 AM, David Fetter <david@fetter.org> wrote:

On Mon, Sep 02, 2013 at 02:32:16AM -0400, Peter Eisentraut wrote:

On Thu, 2013-08-29 at 12:16 +0200, Dimitri Fontaine wrote:

Here's v14 of the patch with pg_upgrade support fixed, so that the
automated setup that Peter built is able to have at it!

Fails cpluspluscheck:

In file included from /tmp/cpluspluscheck.5g2uWw/test.cpp:3:0:
./src/include/commands/template.h:47:8: error: ‘ExtensionControl’ does not name a type
./src/include/commands/template.h:51:8: error: ‘ExtensionControl’ does not name a type

I think this actually just means the header does not include all it needs by itself.

Is there some standard set of checks you run on new patches, and are
the results showing up on, say, the buildfarm or some other CI
dashboard?

I believe that Peter does all those checks using his own Jenkins environment:
http://pgci.eisentraut.org/jenkins/
For the commit fest patches here you go:
http://pgci.eisentraut.org/jenkins/view/All/job/postgresql_commitfest_world/
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Dimitri,

* Dimitri Fontaine (dimitri@2ndQuadrant.fr) wrote:

So please find v15 of the patch attached to this email, that passes all
previously done checks and this one too now.

Looks like there's been a bit of unfortunate bitrot due to Tom's change
to disable fancy output:

patching file src/test/regress/expected/sanity_check.out
Hunk #1 FAILED at 104.
Hunk #2 FAILED at 166.
2 out of 2 hunks FAILED -- saving rejects to file src/test/regress/expected/sanity_check.out.rej

Are there any other changes you have pending for this..? Would be nice
to see the latest version which you've tested and which patches cleanly
against master... ;)

I'll still go ahead and start looking through this, per our discussion.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

Are there any other changes you have pending for this..? Would be nice
to see the latest version which you've tested and which patches cleanly
against master... ;)

I just rebased now, please see attached. I had to pick new OIDs in some
places too, but that's about it.

I'll still go ahead and start looking through this, per our discussion.

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

Dimitri Fontaine wrote:

Stephen Frost <sfrost@snowman.net> writes:

Are there any other changes you have pending for this..? Would be nice
to see the latest version which you've tested and which patches cleanly
against master... ;)

I just rebased now, please see attached. I had to pick new OIDs in some
places too, but that's about it.

I think you're missing support for getObjectIdentity and
getObjectTypeDescription for the new object classes.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, 2013-11-04 at 08:43 -0500, Stephen Frost wrote:

I'll still go ahead and start looking through this, per our discussion.

In the CF app, this is marked "Ready for Committer". That's a bit vague
here, considering Dimitri, you, Peter, and Alvaro are all committers.
Who is this patch waiting on? Is the discussion concluding, or does it
need another round of review?

Regards,
Jeff Davis

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

Jeff Davis <pgsql@j-davis.com> writes:

In the CF app, this is marked "Ready for Committer". That's a bit vague
here, considering Dimitri, you, Peter, and Alvaro are all committers.
Who is this patch waiting on? Is the discussion concluding, or does it
need another round of review?

Thanks for the confusion I guess, but I'm no committer here ;-)

This patch has received extensive review in July and I think it now
properly implements the design proposed by Tom and Heikki in 9.3/CF4.

As the path didn't make it already, yes it needs another (final) round
of review. The main difficulty in reviewing is understanding the design
and the relation in between our current model of extensions and what
this patch offers.

You might find the discussions we had with Markus Wanner quite useful in
this light. The current situation is that I believe the patch to
implement the same “template” model as the on-disk extensions, down to
dependency tracking.

IIRC I left only one differing behavior, which is that you're not
allowed to DROP an Extension Template when it's needed for a dump and
restore cycle, where you could be doing that at the file system level of
course (and pg_restore on a new system would depend on other files).

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers