Thread: Review: extension template

Review: extension template

From
Markus Wanner
Date:
Hi,

I reviewed Dimitri's work on extension templates [2]. There's some
discussion still ongoing and the patch has gone through several
revisions since its addition to the current CF. The patch has already
been marked as 'returned with feedback', and I can support that
resolution (for this CF). I still see value in the following review.

Initially, I was confused about what the patch is supposed to achieve.
The 'template' naming certainly contributed to that confusion. My mental
model of a template is something that I can throw away after its use. (I
note the text search "templates" don't follow that model, either). Where
as up to now, extensions were something that I install (system wide) and
then enable per database (by CREATE, which can be thought of as a
misnomer as well).

Of course you can think of such a system wide installation as a template
for the creation (an instantiation per database). However, we didn't
ever call the system wide installations templates. Nor does the patch
start to adapt that naming scheme.

The major distinguishing factor is not the 'template' character of
extensions "installed" that way, but the storage place of the its
control data: filesystem vs system catalog. I'd either recommend
appropriate renaming to reflect that fact and to avoid confusing users;
or follow the "template" model better and decouple the extension from
its template - with implications on extensions requiring additional
binary code. Thinking of it, I kind of like that approach...

Dimitri responded promptly to a request to rebase the patch. Version 8
still applies cleanly to git master (as of Jul 5, 9ce9d). The patch
matches git revision c0c507022ec912854e6658c5a10a3dedb1c36d67 of dim's
github branch 'tmpl4' [2]. That's what I tested with.

The base of the patched tree builds just fine (i.e. plain 'make'),
although the compiler rightfully warns about an 'evi' variable being set
but not used. extension.c, line 1170. Jaime mentioned that already.

Compiling pg_upgrade_support in contrib fails:

> $SRC/contrib/pg_upgrade_support/pg_upgrade_support.c:193:8:
>   error: too few arguments to function ‘InsertExtensionTuple’

The build passes 'make check'. Additional tests are provided. Good.

As originally mentioned by Heikki, if the tplscript doesn't parse, the
error message is just "syntax error at or near". That matches the
behavior of extensions installed on the file system. However, given this
adds a second way, a hint about where this error actually comes from is
mandatory, IMO.

Trying to re-create a pre-existing template properly throws 'template
for extension "$NAME" version "$VERSION" already exists'. However, if
the extension is already enabled for that database, it instead says:
"extension "$NAME" already exists". I can see why that's fine if you
assume a strong binding between the "instantiation" and the "template".

However, it's possible to enable an extension and then rename its
template. The binding (as in pg_depend) is still there, but the above
error (in that case "extension $OLD_NAME already existing") certainly
doesn't make sense. One can argue whether or not an extension with a
different name is still the same extension at all...

Trying to alter an inexistent or file-system stored extension doesn't
throw an error, but silently succeeds. Especially in the latter case,
that's utterly misleading. Please fix.

That started to get me worried about the effects of a mixed
installation, but I quickly figured it's not possible to have a full
version on disk and then add an incremental upgrade via the system
catalogs. I think that's a fair limitation, as mixed installations would
pose their own set of issues. On the other hand, isn't ease of upgrades
a selling point for this feature?

In any case, the error message could again be more specific:
 (having extension 'pex' version '0.9' on the file-system)
 # CREATE TEMPLATE FOR EXTENSION pex VERSION '1.0' ... ERROR: extension "pex" already available
 [ This one could mention it exists on the file-system. ]
 # CREATE TEMPLATE FOR EXTENISON pex FROM '1.9' TO '1.10' AS ... ERROR: no template for extension "pex"

This last error is technically correct only if you consider file system
extensions to not be templates. In any case, there is *something*
relevant called "pex" on the file-system, that prevents creation of the
template in the system catalogs. The error message should definitely be
more specific.

With delight I note that renaming to an extension name that pre-exists
on the file-system is properly prohibited. Again, the error message
could be more specific and point to the appropriate place. However,
that's reasonably far down the wish-list.

[ Also note the nifty difference between "extension already available"
vs "extension already exists". The former seems to mean the "template"
exists, while the latter refers to the "instantiation". ]

However, the other way around cannot be prevented by Postgres: Files
representing an extension (template, if you want) can always be created
alongside a pre-existing system catalog installation. Postgres has no
way to prevent that (other than ignoring the files, maybe, but...). It
seems the file system variant takes precedence over anything
pre-existing in the system catalogs. This should be documented.

The documentation for ALTER TEMPLATE FOR EXTENSION says: "Currently, the
only supported functionality is to change the template's default
version." I don't understand what that's supposed to mean. You can
perfectly well rename and alter the template's code.

I didn't look too deep into the code, but it seems Jaime and Hitoshi
raised some valid points.

Assorted very minor nit-picks:

In catalog/objectaddress.c, get_object_address_unqualified(): the case
OBJECT_TABLESPACE is being moved down. That looks like an like an
unintended change. Please revert.

src/backend/commands/event_trigger.c, definition of
event_trigger_support: several unnecessary whitespaces added. These make
it hard(er) than necessary to review the patch.

Regards

Markus Wanner


[1]: CF: Extension Templates
https://commitfest.postgresql.org/action/patch_view?id=1032

[2]: Dimitri's github branch 'tmpl4':
https://github.com/dimitri/postgres/tree/tmpl4



Re: Review: extension template

From
Markus Wanner
Date:
On 07/05/2013 09:05 PM, Markus Wanner wrote:
> The patch has already
> been marked as 'returned with feedback', and I can support that
> resolution (for this CF).

Oops.. I just realize it's only set to "waiting on author", now. I guess
I confused the two states. Please excuse my glitch.

Dimitri, do you agree to resolve with "returned with feedback" for this
CF? Or how would you like to proceed?

Josh, thanks for linking the review in the CF.

Regards

Markus Wanner



Re: Review: extension template

From
Dimitri Fontaine
Date:
Hi,

Thanks a lot for your detailed review!

Markus Wanner <markus@bluegap.ch> writes:
> Initially, I was confused about what the patch is supposed to achieve.
> The 'template' naming certainly contributed to that confusion. My mental

Yes, I did share this viewpoint over the naming of the feature, but Tom
insisted that we already have those kind of templates for text search.

> The major distinguishing factor is not the 'template' character of
> extensions "installed" that way, but the storage place of the its
> control data: filesystem vs system catalog. I'd either recommend
> appropriate renaming to reflect that fact and to avoid confusing users;
> or follow the "template" model better and decouple the extension from
> its template - with implications on extensions requiring additional
> binary code. Thinking of it, I kind of like that approach...

Could you go into more details into your ideas here? I don't understand
what you're suggesting.

> Compiling pg_upgrade_support in contrib fails:
>
>> $SRC/contrib/pg_upgrade_support/pg_upgrade_support.c:193:8:
>>   error: too few arguments to function ‘InsertExtensionTuple’

I don't have that problem here. Will try to reproduce early next week.

> As originally mentioned by Heikki, if the tplscript doesn't parse, the
> error message is just "syntax error at or near". That matches the
> behavior of extensions installed on the file system. However, given this
> adds a second way, a hint about where this error actually comes from is
> mandatory, IMO.

Will have a look at what it takes to implement support for better error
messages. May I suggest to implement that later, though? I think this is
an improvement over the current system that will be complicated to get
right and I don't want that to swamp the current patch. After all, this
patch is already in its 3rd development cycle…

> Trying to re-create a pre-existing template properly throws 'template
> for extension "$NAME" version "$VERSION" already exists'. However, if
> the extension is already enabled for that database, it instead says:
> "extension "$NAME" already exists". I can see why that's fine if you
> assume a strong binding between the "instantiation" and the "template".

The idea here is to protect against mixing the file based extension
installation mechanism with the catalogs one. I can see now that the
already installed extension could have been installed using a template
in the first place, so that message now seems strange.

I still think that we shouldn't allow creating a template for an
extension that is already installed, though. Do you have any suggestions
for a better error message?

> However, it's possible to enable an extension and then rename its
> template. The binding (as in pg_depend) is still there, but the above
> error (in that case "extension $OLD_NAME already existing") certainly
> doesn't make sense. One can argue whether or not an extension with a
> different name is still the same extension at all...

When renaming a template, the check against existing extensions of the
same name is made against the new name of the template, so I don't see
what you say here in the code. Do you have a test case?

> Trying to alter an inexistent or file-system stored extension doesn't
> throw an error, but silently succeeds. Especially in the latter case,
> that's utterly misleading. Please fix.

Fixed in my github branch, not producing a new patch version as of yet,
I think we need to set the new error messages first and I'm running
short of inspiration tonight.

> That started to get me worried about the effects of a mixed
> installation, but I quickly figured it's not possible to have a full
> version on disk and then add an incremental upgrade via the system
> catalogs. I think that's a fair limitation, as mixed installations would
> pose their own set of issues. On the other hand, isn't ease of upgrades
> a selling point for this feature?

The main issue to fix when you want to have that feature, which I want
to have, is how to define a sane pg_dump policy around the thing. As we
couldn't define that in the last rounds of reviews, we decided not to
allow the case yet.

I think that's a fair remark that we want to get there eventually, and
that like the superuser only limitation, that's something for a future
patch and not this one.

> In any case, the error message could again be more specific:
>
>   (having extension 'pex' version '0.9' on the file-system)
>
>   # CREATE TEMPLATE FOR EXTENSION pex VERSION '1.0' ...
>   ERROR: extension "pex" already available
>
>   [ This one could mention it exists on the file-system. ]
>
>   # CREATE TEMPLATE FOR EXTENISON pex FROM '1.9' TO '1.10' AS ...
>   ERROR: no template for extension "pex"
>
> This last error is technically correct only if you consider file system
> extensions to not be templates. In any case, there is *something*
> relevant called "pex" on the file-system, that prevents creation of the
> template in the system catalogs. The error message should definitely be
> more specific.

Will fix early next week.

> With delight I note that renaming to an extension name that pre-exists
> on the file-system is properly prohibited. Again, the error message
> could be more specific and point to the appropriate place. However,
> that's reasonably far down the wish-list.
>
> [ Also note the nifty difference between "extension already available"
> vs "extension already exists". The former seems to mean the "template"
> exists, while the latter refers to the "instantiation". ]

Yeah, we might want to find some better naming for the on-file extension
"templates". We can't just call them extension templates though because
that is the new beast implemented in that patch and it needs to be part
of your pg_dump scripts, while the main feature of the file based kind
of templates is that they are *NOT* part of your dump.

That's the crux of that patch difficulties.

> However, the other way around cannot be prevented by Postgres: Files
> representing an extension (template, if you want) can always be created
> alongside a pre-existing system catalog installation. Postgres has no
> way to prevent that (other than ignoring the files, maybe, but...). It
> seems the file system variant takes precedence over anything
> pre-existing in the system catalogs. This should be documented.

Right. That will be documented in version 9 of the patch.

> The documentation for ALTER TEMPLATE FOR EXTENSION says: "Currently, the
> only supported functionality is to change the template's default
> version." I don't understand what that's supposed to mean. You can
> perfectly well rename and alter the template's code.

It's just something I forgot to cleanup when completing the feature set.
Cleaned up in my git branch.

> In catalog/objectaddress.c, get_object_address_unqualified(): the case
> OBJECT_TABLESPACE is being moved down. That looks like an like an
> unintended change. Please revert.

Fixed in my github branch already, will appear in next patch version.

> src/backend/commands/event_trigger.c, definition of
> event_trigger_support: several unnecessary whitespaces added. These make
> it hard(er) than necessary to review the patch.

Here with the following command I see no problem, please advise:

git diff --check --color-words postgres/master.. -- src/backend/commands/event_trigger.c

Markus Wanner <markus@bluegap.ch> writes:
> Dimitri, do you agree to resolve with "returned with feedback" for this
> CF? Or how would you like to proceed?

I'd like to proceed, it's the 3rd development cycle that I'm working on
this idea (used to be called "Inline Extensions", I also had a selective
pg_dump patch to allow dumping some extension scripts, etc). I really
wish we would be able to sort it out completely in this very CF and
that's why I'm not proposing any other patch this time.

Of course, we might still need another round at it, and that's life.

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



Re: Review: extension template

From
Markus Wanner
Date:
Salut Dimitri,

On 07/06/2013 10:30 PM, Dimitri Fontaine wrote:
> Yes, I did share this viewpoint over the naming of the feature, but Tom
> insisted that we already have those kind of templates for text search.

I think it's a question of what mental model we want extensions to
follow. See my other mail. IMO "inline" could well serve as a
differentiator against out-of-line, i.e. file-system based extensions
(or templates).

> Could you go into more details into your ideas here? I don't understand
> what you're suggesting.

Sorry for not making that clearer. See my follow-up mail "template-ify
(binary) extensions":
http://archives.postgresql.org/message-id/51D72C1D.7010609@bluegap.ch

>> Compiling pg_upgrade_support in contrib fails:
>>
>>> $SRC/contrib/pg_upgrade_support/pg_upgrade_support.c:193:8:
>>>   error: too few arguments to function ‘InsertExtensionTuple’
> 
> I don't have that problem here. Will try to reproduce early next week.

'make' followed by 'make -C contrib' reproduces that for me.

> Will have a look at what it takes to implement support for better error
> messages. May I suggest to implement that later, though?

Great, thanks. I agree this is not a major issue and can be deferred.

> The idea here is to protect against mixing the file based extension
> installation mechanism with the catalogs one. I can see now that the
> already installed extension could have been installed using a template
> in the first place, so that message now seems strange.
> 
> I still think that we shouldn't allow creating a template for an
> extension that is already installed, though. Do you have any suggestions
> for a better error message?

If we go for the template model, I beg to differ. In that mind-set, you
should be free to create (or delete) any kind of template without
affecting pre-existing extensions.

However, in case we follow the ancestor-derivate or link model with the
pg_depend connection, the error message seems fine.

>> However, it's possible to enable an extension and then rename its
>> template. The binding (as in pg_depend) is still there, but the above
>> error (in that case "extension $OLD_NAME already existing") certainly
>> doesn't make sense. One can argue whether or not an extension with a
>> different name is still the same extension at all...
> 
> When renaming a template, the check against existing extensions of the
> same name is made against the new name of the template, so I don't see
> what you say here in the code. Do you have a test case?

Consider this (not actually tested again, only off the top of my head):
 # CREATE TEMPLATE FOR EXTENSION foo ... ERROR: Extension "bar" already existing

"Uh? .. so what?" is my reaction to such an error message. It fails to
make the distinction between template and extension. Or rather parent
and derivate. The link between the two is the actual reason for the failure.

In any case, I'm arguing this "template" renaming behavior (and the
subsequent error) are the wrong thing to do, anyways. Because an
extension being linked to a parent of a different name is weird and IMO
not an acceptable state.

In the link model, the name should better be thought of as a property of
the parent. A rename of it should thus rename the derived extensions in
all databases. That would prevent the nasty case of having a parent with
different name than the derivative extension. (I note that existing
file-system extensions do not work that way. They are a lot closer to
the template model. However, that's just an argument for following that
as well for inline extensions and dropping the pg_depend link between
extension and template.)

In the template model, renaming the template should not have an effect
on any extension. Neither should creation or deletion of any template.
Thus, creating a template with the same name as a pre-existing extension
(in any database) is a completely fine and valid operation. No error
needs to be thrown. This nicely shows why I currently favor the template
approach: it seems easier to understand *and* easier to implement.

>> Trying to alter an inexistent or file-system stored extension doesn't
>> throw an error, but silently succeeds. Especially in the latter case,
>> that's utterly misleading. Please fix.
> 
> Fixed in my github branch

Nice.

>> That started to get me worried about the effects of a mixed
>> installation, but I quickly figured it's not possible to have a full
>> version on disk and then add an incremental upgrade via the system
>> catalogs. I think that's a fair limitation, as mixed installations would
>> pose their own set of issues. On the other hand, isn't ease of upgrades
>> a selling point for this feature?
> 
> The main issue to fix when you want to have that feature, which I want
> to have, is how to define a sane pg_dump policy around the thing. As we
> couldn't define that in the last rounds of reviews, we decided not to
> allow the case yet.

I bet that's because people have different mental models in mind. But I
probably stressed that point enough by now...

> I think that's a fair remark that we want to get there eventually, and
> that like the superuser only limitation, that's something for a future
> patch and not this one.

I agree to defer a specific implementation. I disagree in that we should
*not* commit something that follows a mixture of mental models
underneath and sets in stone a strange API we later need to be
compatible with.

Specifically, I request to either follow the template model more closely
(accompanied by a separate patch to adjust binary, out-of-line
templates) or follow the link model more closely. The current naming
doesn't match any of the two, so renaming seems inevitable.

> Yeah, we might want to find some better naming for the on-file extension
> "templates". We can't just call them extension templates though because
> that is the new beast implemented in that patch and it needs to be part
> of your pg_dump scripts, while the main feature of the file based kind
> of templates is that they are *NOT* part of your dump.

That's one issue where the mixture of concepts shines through, yeah. I
fear simply (re)naming things is not quite enough, at this point.
Especially because the thing on the file-system is closer to being a
template than the system catalog based variant.

[ Maybe we could even go with "template extensions" being the
file-system ones vs. "inline extensions" being the system catalog
ones...  In that case, they would follow different mental models. Which
might even be a reasonable solution. However, we need to be aware of
that difference and document it properly, so that users have a chance to
grasp the nifty difference. I'd rather prefer to eliminate such nifty
differences, though. ]

> It's just something I forgot to cleanup when completing the feature set.
> Cleaned up in my git branch.

Great!

>> src/backend/commands/event_trigger.c, definition of
>> event_trigger_support: several unnecessary whitespaces added. These make
>> it hard(er) than necessary to review the patch.
> 
> Here with the following command I see no problem, please advise:
> 
> git diff --check --color-words postgres/master.. -- src/backend/commands/event_trigger.c

<rant>That's why I personally don't trust git.</rant> Please look at the
patch you submitted.

> Markus Wanner <markus@bluegap.ch> writes:
>> Dimitri, do you agree to resolve with "returned with feedback" for this
>> CF? Or how would you like to proceed?
> 
> I'd like to proceed, it's the 3rd development cycle that I'm working on
> this idea

Okay, I'm certainly not opposed to that and welcome further development.
I didn't change the status, so it should still be "waiting on author",
which seems reasonable to me, ATM.

> (used to be called "Inline Extensions", I also had a selective
> pg_dump patch to allow dumping some extension scripts, etc). I really
> wish we would be able to sort it out completely in this very CF and
> that's why I'm not proposing any other patch this time.

I understand it's tedious work, sometimes. Please look at this as
incremental progress. I would not have been able to reason about mental
models without your patch. Thanks for that, it was a necessary and good
step from my point of view. Please keep up the good work!

Regards

Markus Wanner



Re: Review: extension template

From
Markus Wanner
Date:
On 07/06/2013 10:30 PM, Dimitri Fontaine wrote:
> I still think that we shouldn't allow creating a template for an
> extension that is already installed, though. Do you have any suggestions
> for a better error message?

Oh, I just realize that pg_extension_{template,control,uptmpl} are not
SHARED catalogs, but you need to install the template per-database and
then need to enable it - per-database *again*. Why is that?

If you want to just upload extensions to a database via libpq, that
should be a single step (maybe rather just CREATE EXTENSION ... AS ...)
If you want "templates", those should be applicable to all databases, no?

Regards

Markus Wanner



Re: Review: extension template

From
Markus Wanner
Date:
On 07/07/2013 02:55 PM, Markus Wanner wrote:
> If you want to just upload extensions to a database via libpq..

Let's rephrase this in a (hopefully) more constructive way: I get the
impression you are trying to satisfy many different needs. Way more that
you need to scratch your own itch. To the point that I had trouble
figuring out what exactly the goal of your patch is.

My advice would be: Be brave! Dare to put down any request for
"templates" (including mine) and go for the simplest possible
implementation that allows you to create extensions via libpq. (Provided
that really is the itch you want to scratch. I'm still not quite sure I
got that right.)

As it stands, I get the impression the patch is trying to sell me a
feature that it doesn't really provide. If you stripped the template
stuff, including the CREATE TEMPLATE FOR EXTENSION command, but just
sold me this patch as a simple way to create an extension via libpq,
i.e. something closer to CREATE EXTENSION AS .. , I would immediately
buy that. Currently, while allowing an upload, it seems far from simple,
but adds quite a bit of unwanted complexity. If all I want is to upload
code for an extension via libpq, I don't want to deal with nifty
distinctions between templates and extensions.

Just my opinion, though. Maybe I'm still missing something.

Regards

Markus Wanner



Re: Review: extension template

From
Dimitri Fontaine
Date:
Markus Wanner <markus@bluegap.ch> writes:
> Oh, I just realize that pg_extension_{template,control,uptmpl} are not
> SHARED catalogs, but you need to install the template per-database and
> then need to enable it - per-database *again*. Why is that?

Because the current model is not serving us well enough, with a single
module version per major version of PostgreSQL. Meaning for all the
clusters on the server, and all the databases in them.

We want to be able to have postgis 1.5 and 2.x installed in two
different databases in the same cluster, don't we?

Well the current patch we still can't because of the dynamically shared
object side of things, but that's not a good reason to impose the same
limitation on to the "template" idea.

>           Currently, while allowing an upload, it seems far from simple,
> but adds quite a bit of unwanted complexity. If all I want is to upload
> code for an extension via libpq, I don't want to deal with nifty
> distinctions between templates and extensions.
>
> Just my opinion, though. Maybe I'm still missing something.

Yes: dump & restore.

After playing around with several ideas around that in the past two
development cycles, the community consensus clearly is that "extensions"
are *NEVER* going to be part of your dump scripts.

Now when using templates you have no other source to install the
extensions from at pg_restore time, given what I just said.

The whole goal of the "template" idea is to offer a way to dump and
restore the data you need for CREATE EXTENSION to just work at restore
time, even when you sent the data over the wire.

Current extension are managed on the file system, the contract is that
it is the user's job to maintain and ship that, externally to PostgreSQL
responsibilities. All that PostgreSQL knows about is to issue the CREATE
EXTENSION command at pg_restore time.

With templates or in-line extensions, the contract is that the user asks
PostgreSQL to manage its extensions in the same way it does for the
other objects on the system. The design we found to address that is
called "Extension Templates" and is implemented in the current patch.

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



Re: Review: extension template

From
Markus Wanner
Date:
Hello Dimitri,

On 07/07/2013 09:51 PM, Dimitri Fontaine wrote:
> Markus Wanner <markus@bluegap.ch> writes:
>> Oh, I just realize that pg_extension_{template,control,uptmpl} are not
>> SHARED catalogs, but you need to install the template per-database and
>> then need to enable it - per-database *again*. Why is that?
> 
> Because the current model is not serving us well enough, with a single
> module version per major version of PostgreSQL. Meaning for all the
> clusters on the server, and all the databases in them.

That's not an excuse for letting the user duplicate the effort of
installing templates for every version of his extension in every database.

> We want to be able to have postgis 1.5 and 2.x installed in two
> different databases in the same cluster, don't we?

The extension, yes. The template versions, no. There's utterly no point
in having different 2.0 versions of the same extension in different
databases.

> Well the current patch we still can't because of the dynamically shared
> object side of things, but that's not a good reason to impose the same
> limitation on to the "template" idea.

Without a dynamically shared object, you can well have different
versions of an extension at work in different databases already.

> After playing around with several ideas around that in the past two
> development cycles, the community consensus clearly is that "extensions"
> are *NEVER* going to be part of your dump scripts.

Sounds strange to me. If you want Postgres to manage extensions, it
needs the ability to backup and restore them. Otherwise, it doesn't
really ... well ... manage them.

> Now when using templates you have no other source to install the
> extensions from at pg_restore time, given what I just said.
> 
> The whole goal of the "template" idea is to offer a way to dump and
> restore the data you need for CREATE EXTENSION to just work at restore
> time, even when you sent the data over the wire.

Which in turn violates the above cited community consensus, then. You
lost me here. What's your point? I thought the goal of your patch was
the ability to upload an extension via libpq.

How does that address my concerns about usability and understandability
of how these things work? Why the strange dependencies between templates
and extensions? Or the ability to rename the template, but not the
extension - while still having the later depend on the former?

These things are what I'm opposing to. And I don't believe it
necessarily needs to be exactly that way for dump and restore to work.
Quite the opposite, in fact. Simpler design usually means simpler backup
and restore procedures.

> Current extension are managed on the file system, the contract is that
> it is the user's job to maintain and ship that, externally to PostgreSQL
> responsibilities. All that PostgreSQL knows about is to issue the CREATE
> EXTENSION command at pg_restore time.
> 
> With templates or in-line extensions, the contract is that the user asks
> PostgreSQL to manage its extensions in the same way it does for the
> other objects on the system.

Understood.

> The design we found to address that is
> called "Extension Templates" and is implemented in the current patch.

I placed my concerns with the proposed implementation. It's certainly
not the only way how Postgres can manage its extensions. And I still
hope we can come up with something that's simpler to use and easier to
understand.

However, I'm not a committer nor have I written code for this. I did my
review and proposed two possible (opposite) directions for clean up and
simplification of the design. I would now like others to chime in.

Regards

Markus Wanner



Re: Review: extension template

From
Heikki Linnakangas
Date:
On 08.07.2013 00:48, Markus Wanner wrote:
> On 07/07/2013 09:51 PM, Dimitri Fontaine wrote:
>> The design we found to address that is
>> called "Extension Templates" and is implemented in the current patch.
>
> I placed my concerns with the proposed implementation. It's certainly
> not the only way how Postgres can manage its extensions. And I still
> hope we can come up with something that's simpler to use and easier to
> understand.

I'm just now dabbling back to this thread after skipping a lot of 
discussion, and I'm disappointed to see that this still seems to be 
running in circles on the same basic question: What exactly is the patch 
trying to accomplish.

The whole point of extensions, as they were originally implemented, is 
to allow them to be managed *outside* the database. In particular, they 
are not included in pg_dump. If you do want them to be included in 
pg_dump, why create it as an extension in the first place? Why not just 
run the create script and create the functions, datatypes etc. directly, 
like you always did before extensions were even invented.

I think the reason is that extensions provide some handy packaging of 
the functions etc, so that you can just do "DROP EXTENSION foo" to get 
rid of all of them. Also, pg_extension table keeps track of the 
currently installed version. Perhaps we need to step back and invent 
another concept that is totally separate from extensions, to provide 
those features. Let's call them "modules". A module is like an 
extension, in that all the objects in the module can be dropped with a 
simple "DROP MODULE foo" command. To create a module, you run "CREATE 
MODULE foo AS <SQL script to create the objects in the module>".

I believe that would be pretty much exactly what Dimitri's original 
inline extension patches did, except that it's not called an extension, 
but a module. I think it's largely been the naming that has been the 
problem with this patch from the very beginning. We came up with the 
concept of templates after we had decided that the originally proposed 
behavior was not what we want from something called extensions. But if 
you rewind to the very beginning, the problem was just with the name. 
The concept was useful, but not something we want to call an extension, 
because the distinguishing feature of an extension is that it lives 
outside the database and is *not* included in pg_dump.

- Heikki



Re: Review: extension template

From
Hannu Krosing
Date:
On 07/08/2013 09:26 AM, Heikki Linnakangas wrote:
> On 08.07.2013 00:48, Markus Wanner wrote:
>> On 07/07/2013 09:51 PM, Dimitri Fontaine wrote:
>>> The design we found to address that is
>>> called "Extension Templates" and is implemented in the current patch.
>>
>> I placed my concerns with the proposed implementation. It's certainly
>> not the only way how Postgres can manage its extensions. And I still
>> hope we can come up with something that's simpler to use and easier to
>> understand.
>
> I'm just now dabbling back to this thread after skipping a lot of
> discussion, and I'm disappointed to see that this still seems to be
> running in circles on the same basic question: What exactly is the
> patch trying to accomplish.
>
> The whole point of extensions, as they were originally implemented, is
> to allow them to be managed *outside* the database. In particular,
> they are not included in pg_dump. If you do want them to be included
> in pg_dump, why create it as an extension in the first place? Why not
> just run the create script and create the functions, datatypes etc.
> directly, like you always did before extensions were even invented.
>
> I think the reason is that extensions provide some handy packaging of
> the functions etc, so that you can just do "DROP EXTENSION foo" to get
> rid of all of them. Also, pg_extension table keeps track of the
> currently installed version. Perhaps we need to step back and invent
> another concept that is totally separate from extensions, to provide
> those features. Let's call them "modules". A module is like an
> extension, in that all the objects in the module can be dropped with a
> simple "DROP MODULE foo" command. To create a module, you run "CREATE
> MODULE foo AS <SQL script to create the objects in the module>".
>
> I believe that would be pretty much exactly what Dimitri's original
> inline extension patches did, except that it's not called an
> extension, but a module. I think it's largely been the naming that has
> been the problem with this patch from the very beginning. We came up
> with the concept of templates after we had decided that the originally
> proposed behavior was not what we want from something called
> extensions. But if you rewind to the very beginning, the problem was
> just with the name. The concept was useful, but not something we want
> to call an extension, because the distinguishing feature of an
> extension is that it lives outside the database and is *not* included
> in pg_dump.
Either MODULE or PACKAGE would be good name candidates.

Still, getting this functionality in seems more important than exact
naming, though naming them "right" would be nice.
>
> - Heikki
>
>




Re: Review: extension template

From
Dimitri Fontaine
Date:
> On 07/08/2013 09:26 AM, Heikki Linnakangas wrote:
>> I'm just now dabbling back to this thread after skipping a lot of
>> discussion, and I'm disappointed to see that this still seems to be
>> running in circles on the same basic question: What exactly is the
>> patch trying to accomplish.

Bypassing the file system entirely in order to install an extension. As
soon as I figure out how to, including C-coded extensions.

>> I think the reason is that extensions provide some handy packaging of
>> the functions etc, so that you can just do "DROP EXTENSION foo" to get
>> rid of all of them. Also, pg_extension table keeps track of the
>> currently installed version. Perhaps we need to step back and invent
>> another concept that is totally separate from extensions, to provide

The main feature of the extensions system is its ability to have a clean
pg_restore process even when you use some extensions. That has been the
only goal of the whole feature development.

Let me stress that the most important value in that behavior is to be
able to pg_restore using a newer version of the extension, the one that
works with the target major version. When upgrading from 9.2 to 9.3 if
you depend on keywords that now are reserved you need to install the
newer version of the extension at pg_restore time.

The main features I'm interested into beside a clean pg_restore are
UPDATE scripts for extensions and dependency management, even if that
still needs improvements. Those improvements will be relevant for both
ways to make extensions available for your system.

>> those features. Let's call them "modules". A module is like an
>> extension, in that all the objects in the module can be dropped with a
>> simple "DROP MODULE foo" command. To create a module, you run "CREATE
>> MODULE foo AS <SQL script to create the objects in the module>".

Not again the naming. A module is already documented as a shared object
library (.so, .dll or .dylib) that PostgreSQL will LOAD for you. A patch
has already been proposed to track which module is loaded in a session
and offer that in a new system's view, pg_module.

We can not use the name "module" for anything else, IMNSHO.

>> just with the name. The concept was useful, but not something we want
>> to call an extension, because the distinguishing feature of an
>> extension is that it lives outside the database and is *not* included
>> in pg_dump.

The main goal here is not to have the extension live inside the database
but rather to be able to bypass using the server's filesystem in order
to be able to CREATE EXTENSION foo; and then to still have pg_restore do
the right thing on its own.

If you want to scratch the new catalogs part, then just say that it's
expected to be really complex to pg_restore a database using extensions,
back to exactly how it was before 9.1: create the new database, create
the extensions your dump depends on in that new database, now pg_restore
your backup manually filtering away the extensions' objects or ignoring
the errors when pg_restore tries to duplicate functions you already
installed in the previous step. No fun.

Hannu Krosing <hannu@krosing.net> writes:
> Either MODULE or PACKAGE would be good name candidates.

The name "package" is even worse than the "module" one because lots of
people think they know exactly what is a package for having been using a
closed source product that you might have heard of: they are trying to
cope with our ability to implement new features on a yearly basis while
not breaking anything we already have.

> Still, getting this functionality in seems more important than exact
> naming, though naming them "right" would be nice.

Of course we want to do it right™.

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



Re: Review: extension template

From
Markus Wanner
Date:
On 06/10/2013 09:43 PM, Hannu Krosing wrote:
> On 07/08/2013 09:26 AM, Heikki Linnakangas wrote:
>> The concept was useful, but not something we want
>> to call an extension, because the distinguishing feature of an
>> extension is that it lives outside the database and is *not* included
>> in pg_dump.
> Either MODULE or PACKAGE would be good name candidates.
> 
> Still, getting this functionality in seems more important than exact
> naming, though naming them "right" would be nice.

Remember that we already have quite a lot of extensions. And PGXN. Are
we really so wedded to the idea of extensions "living" outside of the
database that we need to come up with something different and incompatible?

Or do you envision modules or packages to be compatible with extensions?
Just putting another label on it so we can still claim extensions are
strictly external to the database? Sorry, I don't get the idea, there.

From a users perspective, I want extensions, modules, or packages to be
managed somehow. Including upgrades, migrations (i.e. dump & restore)
and removal. The approach of letting the distributors handle that
packaging clearly has its limitations. What's so terribly wrong with
Postgres itself providing better tools to manage those?

Inventing yet another type of extension, module or package (compatible
or not) doesn't help, but increases confusion even further. Or how do
you explain to an author of an existing extension, whether or not he
should convert his extension to a module (if you want those to be
incompatible)?

If it's the same thing, just with different loading mechanisms, please
keep calling it the same: an extension. (And maintain compatibility
between the different ways to load it.)

I fully agree with the fundamental direction of Dimitri's patch. I think
Postgres needs to better manage its extensions itself. Including dump
and restore cycles. However, I think the implementation isn't optimal,
yet. I pointed out a few usability issues and gave reasons why
"template" is a misnomer (with the proposed implementation). "Extension"
is not.

(I still think "template" would be a good mental model. See my other
thread...
http://archives.postgresql.org/message-id/51D72C1D.7010609@bluegap.ch)

Regards

Markus Wanner



Re: Review: extension template

From
Markus Wanner
Date:
On 07/08/2013 10:20 AM, Dimitri Fontaine wrote:
> Bypassing the file system entirely in order to install an extension. As
> soon as I figure out how to, including C-coded extensions.

Do I understand correctly that you want to keep the extensions (or their
templates) out of the dump and require the user to "upload" it via libpq
prior to the restor; instead of him having to install them via .deb or .rpm?

This would explain why you keep the CREATE TEMPLATE FOR EXTENSION as a
separate step from CREATE EXTENSION. And why you, too, insist on wanting
templates, and not just a way to create an extension via libpq.

However, why don't you follow the template model more closely? Why
should the user be unable to create a template, if there already exists
an extension of the same name? That's an unneeded and disturbing
limitation, IMO.

My wish: Please drop the pg_depend link between template and extension
and make the templates shared across databases. So I also have to
install the template only once per cluster. Keep calling them templates,
then. (However, mind that file-system extension templates are templates
as well. In-line vs. out-of-line templates, if you want.)

I think you could then safely allow an upgrade of an extension that has
been created from an out-of-line template by an upgrade script that
lives in-line. And vice-versa. Just as an example. It all gets nicer and
cleaner, if the in-line thing better matches the out-of-line one, IMO.

An extension should look and behave exactly the same, independent of
what kind of template it has been created from. And as we obviously
cannot add a pg_depend link to a file on the file system, we better
don't do that for the in-line variant, either, to maintain the symmetry.

> The main feature of the extensions system is its ability to have a clean
> pg_restore process even when you use some extensions. That has been the
> only goal of the whole feature development.

Great! Very much appreciated.

> Let me stress that the most important value in that behavior is to be
> able to pg_restore using a newer version of the extension, the one that
> works with the target major version. When upgrading from 9.2 to 9.3 if
> you depend on keywords that now are reserved you need to install the
> newer version of the extension at pg_restore time.
> 
> The main features I'm interested into beside a clean pg_restore are
> UPDATE scripts for extensions and dependency management, even if that
> still needs improvements. Those improvements will be relevant for both
> ways to make extensions available for your system.

+1

> We can not use the name "module" for anything else, IMNSHO.

Agreed.

> The main goal here is not to have the extension live inside the database
> but rather to be able to bypass using the server's filesystem in order
> to be able to CREATE EXTENSION foo; and then to still have pg_restore do
> the right thing on its own.

Note that with the current, out-of-line approach, the *extension*
already lives inside the database. It's just the *template*, that
doesn't. (Modulo DSO, but the patch doesn't handle those either, yet. So
we're still kind of excluding those.)

Allowing for templates to live inside the database as well is a good
thing, IMO.

> If you want to scratch the new catalogs part, then just say that it's
> expected to be really complex to pg_restore a database using extensions,
> back to exactly how it was before 9.1: create the new database, create
> the extensions your dump depends on in that new database, now pg_restore
> your backup manually filtering away the extensions' objects or ignoring
> the errors when pg_restore tries to duplicate functions you already
> installed in the previous step. No fun.

Definitely not. Nobody wants to go back there. (And as Heikki pointed
out, if you absolutely want to, you can even punish yourself that way.)

Regards

Markus Wanner



Re: Review: extension template

From
Dimitri Fontaine
Date:
Hi,

Please find attached to this mail version 9 of the Extension Templates
patch with fixes for the review up to now.

Markus Wanner <markus@bluegap.ch> writes:
>> I still think that we shouldn't allow creating a template for an
>> extension that is already installed, though. Do you have any suggestions
>> for a better error message?
>
> If we go for the template model, I beg to differ. In that mind-set, you
> should be free to create (or delete) any kind of template without
> affecting pre-existing extensions.

Then what happens at pg_restore time? the CREATE EXTENSION in the backup
script will suddenly install the other extension's that happen to have
the same name? I think erroring out is the only safe way here.

> However, in case we follow the ancestor-derivate or link model with the
> pg_depend connection, the error message seems fine.

It's all about pg_restore really, but it's easy to forget about that and
get started into other views of the world. I'll try to be better at not
following those tracks and just hammer my answers with "pg_restore" now.

> In any case, I'm arguing this "template" renaming behavior (and the
> subsequent error) are the wrong thing to do, anyways. Because an
> extension being linked to a parent of a different name is weird and IMO
> not an acceptable state.

Yes, you're right on spot here. So I've amended the patch to implement
the following behavior (and have added a new regression test):

  # CREATE TEMPLATE FOR EXTENSION foo VERSION 'v' AS '';
  # CREATE EXTENSION foo;
  # ALTER TEMPLATE FOR EXTENSION foo RENAME TO bar;
  ERROR:  55006: template for extension "foo" is in use
  DETAIL:  extension "foo" already exists
  LOCATION:  AlterExtensionTemplateRename, template.c:1040
  STATEMENT:  ALTER TEMPLATE FOR EXTENSION foo RENAME TO bar;

> I bet that's because people have different mental models in mind. But I
> probably stressed that point enough by now...

FWIW, I do agree. But my understanding is that the community consensus
is not going that way.

> Specifically, I request to either follow the template model more closely
> (accompanied by a separate patch to adjust binary, out-of-line
> templates) or follow the link model more closely. The current naming
> doesn't match any of the two, so renaming seems inevitable.

I think we need to follow the link model more closely because that's the
consensus, and I will fix all the remaning discrepancies in between the
two models that we can find. Please continue showing them to me!

>>> src/backend/commands/event_trigger.c, definition of
>>> event_trigger_support: several unnecessary whitespaces added. These make
>>> it hard(er) than necessary to review the patch.

Fixed in the attached version 9 of the patch.

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


Attachment

Re: Review: extension template

From
Markus Wanner
Date:
Hello Dimitri,

On 07/08/2013 11:49 AM, Dimitri Fontaine wrote:
> Please find attached to this mail version 9 of the Extension Templates
> patch with fixes for the review up to now.

Thanks, cool.

> Markus Wanner <markus@bluegap.ch> writes:
>>> I still think that we shouldn't allow creating a template for an
>>> extension that is already installed, though. Do you have any suggestions
>>> for a better error message?
>>
>> If we go for the template model, I beg to differ. In that mind-set, you
>> should be free to create (or delete) any kind of template without
>> affecting pre-existing extensions.
> 
> Then what happens at pg_restore time? the CREATE EXTENSION in the backup
> script will suddenly install the other extension's that happen to have
> the same name? I think erroring out is the only safe way here.

Extensions are commonly identified by name (installed ones as well as
available ones, i.e. templates).

Thus I think if a user renames a template, he might have good reasons to
do so. He likely *wants* it to be a template for a different extension.
Likewise when (re-)creating a template with the very same name of a
pre-existing, installed extension.

Maybe the user just wanted to make a "backup" of the template prior to
modifying it. If he then gives the new template the same name as the old
one had, it very likely is similar, compatible or otherwise intended to
replace the former one.

The file-system templates work exactly that way (modulo DSOs). If you
create an extension, then modify (or rename and re-create under the same
name) the template on disk, then dump and restore, you end up with the
new version of it. That's how it worked so far. It's simple to
understand and use.

> It's all about pg_restore really, but it's easy to forget about that and
> get started into other views of the world. I'll try to be better at not
> following those tracks and just hammer my answers with "pg_restore" now.

That's unlikely to be of much help. It's not like pg_restore would stop
to work. It would just work differently. More like the file-system
templates. More like the users already knows and (likely) expects it.
And more like the "template" model that you advertise.

Or how do you think would pg_restore fail, if you followed the mental
model of the template?

>> In any case, I'm arguing this "template" renaming behavior (and the
>> subsequent error) are the wrong thing to do, anyways. Because an
>> extension being linked to a parent of a different name is weird and IMO
>> not an acceptable state.
> 
> Yes, you're right on spot here. So I've amended the patch to implement
> the following behavior (and have added a new regression test):
> 
>   # CREATE TEMPLATE FOR EXTENSION foo VERSION 'v' AS '';
>   # CREATE EXTENSION foo;
>   # ALTER TEMPLATE FOR EXTENSION foo RENAME TO bar;
>   ERROR:  55006: template for extension "foo" is in use
>   DETAIL:  extension "foo" already exists
>   LOCATION:  AlterExtensionTemplateRename, template.c:1040
>   STATEMENT:  ALTER TEMPLATE FOR EXTENSION foo RENAME TO bar;

Okay, good, this prevents the strange state.

However, this also means you restrict the user even further... How can
he save a copy of an existing template, before (re-)creating it with
CREATE TEMPLATE FOR EXTENSION?

On the file system, a simple cp or mv is sufficient before
(re)installing the package from your distribution, for example.

>> I bet that's because people have different mental models in mind. But I
>> probably stressed that point enough by now...
> 
> FWIW, I do agree.

Good. Why do you continue to propose the link model?

> But my understanding is that the community consensus
> is not going that way.

What way? And what community consensus?

Re-reading some of the past discussions, I didn't find anybody voting
for a dependency between the template and the created extension. And at
least Tom pretty clearly had the template model in mind, when he wrote
[1]: "We don't want it to look like manipulating a template has anything
to do with altering an extension of the same name (which might or might
not even be installed)." or [2]: "But conflating this functionality
[i.e. extension templates] with installed extensions is just going to
create headaches."

The closest I found was Robert Haas mentioning [3], that "[he doesn't]
see a problem having more than one kind of extensions". However, please
mind the context. He doesn't really sound enthusiastic, either.

I'm puzzled about some of your words in that thread. In the very message
Robert responded to, you wrote [4]: "Having more than one way to ship an
extension is good, having two different animals with two different
incompatible behaviors named the same thing is bad."

With the link-model, you are now proposing to create exactly that. Two
different kinds of extensions that are not compatible with each other.
One that is independent and one that depends on the "template" it got
created from.

>> Specifically, I request to either follow the template model more closely
>> (accompanied by a separate patch to adjust binary, out-of-line
>> templates) or follow the link model more closely. The current naming
>> doesn't match any of the two, so renaming seems inevitable.
> 
> I think we need to follow the link model more closely because that's the
> consensus

If that's really the case - which I doubt at the moment - I'll certainly
accept that. I really recommend to rename the feature (and the
commands), in that case, though. We may rather call the existing
file-system thingie an "extension template", instead, as it becomes a
good differentiator to what you're proposing.

> and I will fix all the remaning discrepancies in between the
> two models that we can find.

How about ALTER EXTENSION ADD (or DROP)? With the link on the
"template", you'd have to prohibit that ALTER as well, based on the
exact same grounds as the RENAME: The installed extension would
otherwise differ from the "template" it is linked to.

See how this creates an animal pretty different from the current
extensions? And IMO something that's needlessly restricted in many ways.

> Please continue showing them to me!

Sure, I'm happy to continue reviewing.

Regards

Markus Wanner


[1]: Tom Lane, Re: in-catalog Extension Scripts and Control parameters
(templates?)
http://www.postgresql.org/message-id/6466.1354817682@sss.pgh.pa.us

[2]: Tom Lane, Re: Dumping an Extension's Script
http://www.postgresql.org/message-id/6466.1354817682@sss.pgh.pa.us

[3]: Robert Haas, Re: Dumping an Extension's Script
http://www.postgresql.org/message-id/CA+TgmoabcTrThHV5xe8BP_TE+2wqZGNn-8o4kzQSmkL2W9QY8Q@mail.gmail.com

[4]: Dimitri Fontaine: Re: Dumping an Extension's Script
http://www.postgresql.org/message-id/m2ehj4u7bz.fsf@2ndQuadrant.fr



Re: Review: extension template

From
Dimitri Fontaine
Date:
Markus Wanner <markus@bluegap.ch> writes:
>> Then what happens at pg_restore time? the CREATE EXTENSION in the backup
>> script will suddenly install the other extension's that happen to have
>> the same name? I think erroring out is the only safe way here.
>
> Extensions are commonly identified by name (installed ones as well as
> available ones, i.e. templates).
>
> Thus I think if a user renames a template, he might have good reasons to
> do so. He likely *wants* it to be a template for a different extension.
> Likewise when (re-)creating a template with the very same name of a
> pre-existing, installed extension.

I can understand that as a incomplete step towards a "migration" of some
sorts, but if we just allow to rename a template we open ourselves to be
producing non-restorable dumps (see below). I'm not at ease with that.

> Maybe the user just wanted to make a "backup" of the template prior to
> modifying it. If he then gives the new template the same name as the old
> one had, it very likely is similar, compatible or otherwise intended to
> replace the former one.
>
> The file-system templates work exactly that way (modulo DSOs). If you
> create an extension, then modify (or rename and re-create under the same
> name) the template on disk, then dump and restore, you end up with the
> new version of it. That's how it worked so far. It's simple to
> understand and use.

We have absolutely no control over the file-system templates and that's
why they work differently, I think. There's not even the notion of a
transaction over there.

> Or how do you think would pg_restore fail, if you followed the mental
> model of the template?
 # create template for extension foo version 'x' as ''; # create extension foo; # alter template for extension foo
renameto bar; 
 $ pg_dump | psql

And now it's impossible to CREATE EXTENSION foo, because there's no
source to install it from available. I think we should actively prevent
that scenario to happen in the field (current patch prevents it).

Now, if I'm in the minority, let's just change that.

> However, this also means you restrict the user even further... How can
> he save a copy of an existing template, before (re-)creating it with
> CREATE TEMPLATE FOR EXTENSION?
>
> On the file system, a simple cp or mv is sufficient before
> (re)installing the package from your distribution, for example.

Usually what you do when you want to change an extension is that you
provide an upgrade script then run it with ALTER EXTENSION UPDATE.

Sometimes what you do is prepare a new installation script for a new
version of your extension and you don't provide an upgrade script, then
you update with the following method, in the case when you edited the
default_version property of the .control file:
 # drop extension foo;   -- drops version 1.0 # create extension foo; -- installs version 1.2

The current file system based extensions allow you to maintain
separately the files foo--1.0.sql and foo--1.2.sql, and you don't need
to copy a current version of the whole extension away before hacking the
new version.

The Extension Template facility in the current patch allows you to do
much the same:
 # create template for extension foo version '1.0' as $foo$     create function foo() returns int language sql as $$
select1; $$;   $foo$; 
 # create template for extension foo version '1.2' as $foo$     create function foo() returns int language sql as $$
select2; $$;   $foo$; 
 # select ctlname, ctldefault, ctlversion     from pg_extension_control    where ctlname = 'foo';  ctlname | ctldefault
|ctlversion  ---------+------------+------------  foo     | t          | 1.0  foo     | f          | 1.2 (2 rows) 
 # create extension foo; # select foo();  foo  -----    1 (1 row)

And now you can "upgrade" with:
 # drop extension foo; # create extension foo version '1.2'; # select foo();  foo  -----    2 (1 row)

Or even:
 # alter template for extension foo set default version '1.2'; # drop extension foo; # create extension foo; # select
foo(); foo  -----    2 (1 row) 

So I don't see that we've broken any use case here, really. I think I
understand your objection in principles, but it appears to me that we
would gain nothing here by allowing broken pg_dump scripts.

> What way? And what community consensus?

I see that you've spent extra time and effort to better understand any
community consensus that might exist around this patch series, and I
want to say thank you for that!

> Re-reading some of the past discussions, I didn't find anybody voting
> for a dependency between the template and the created extension. And at
> least Tom pretty clearly had the template model in mind, when he wrote
> [1]: "We don't want it to look like manipulating a template has anything
> to do with altering an extension of the same name (which might or might
> not even be installed)." or [2]: "But conflating this functionality
> [i.e. extension templates] with installed extensions is just going to
> create headaches."

That was a comment against that proposal:
  ALTER EXTENSION … CONFIGURATION FOR 'version' SET param = value, …;  ALTER EXTENSION … SET TEMPLATE FOR 'version' AS
$$… $$;  ALTER EXTENSION … SET TEMPLATE FROM 'version' TO 'version' AS … 

So the patch we have now stays away from conflating templates and
extensions. Still, a dependency in between the extension object and the
template it came from is put into place so that we trust the pg_dump
script to contain enough information to be able to actually restore the
extension later. I think it's important to keep that dependency.

Basically what I'm saying in this too long email is that I need other
contributors to voice-in.

> The closest I found was Robert Haas mentioning [3], that "[he doesn't]
> see a problem having more than one kind of extensions". However, please
> mind the context. He doesn't really sound enthusiastic, either.

That was talking about a very different proposal than the one we're
currently analysing. The whole point of the current proposal has been to
avoid having another kind of extensions.

> I'm puzzled about some of your words in that thread. In the very message
> Robert responded to, you wrote [4]: "Having more than one way to ship an
> extension is good, having two different animals with two different
> incompatible behaviors named the same thing is bad."
>
> With the link-model, you are now proposing to create exactly that. Two
> different kinds of extensions that are not compatible with each other.
> One that is independent and one that depends on the "template" it got
> created from.

I don't see that, actually. The goal here still is that once installed
the behavior of an extension is always the same. The only difference we
have now is how to guarantee that we can restore a dump:
 - using the filesystem based templates, no guarantee at all is offered   by PostgreSQL, that's the developer/sysadmin
responsibility,the   dump script has external dependencies that you must manage yourself; 
 - using the catalog based templates, PostgreSQL guarantees that any   dump you take of the database will be
self-containedwrt extensions,   so that you will be able to actually restore it. 

Any other discrepency would be qualified as a bug I'm willing to fix.

> If that's really the case - which I doubt at the moment - I'll certainly
> accept that. I really recommend to rename the feature (and the
> commands), in that case, though. We may rather call the existing
> file-system thingie an "extension template", instead, as it becomes a
> good differentiator to what you're proposing.

Any proposals?

> How about ALTER EXTENSION ADD (or DROP)? With the link on the
> "template", you'd have to prohibit that ALTER as well, based on the
> exact same grounds as the RENAME: The installed extension would
> otherwise differ from the "template" it is linked to.

You're "supposed" to be using that from within the template scripts
themselves. The main use case is the upgrade scripts from "unpackaged".

I could see foreclosing that danger by enforcing that creating_extension
is true in those commands.

> See how this creates an animal pretty different from the current
> extensions? And IMO something that's needlessly restricted in many ways.

Well really I'm not convinced. If you use ALTER EXTENSION ADD against an
extension that you did install from the file system, then you don't know
what will happen after a dump and restore cycle, because you didn't
alter the files to match what you did, presumably.

If you do the same thing against an extension that you did install from
a catalog template, you just managed to open yourself to the same
hazards… I'd be already patching that like proposed above if it was not
for getting some more input and better assess if my understanding
matches that of Tom and Heikki on those fine points.

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



Re: Review: extension template

From
Markus Wanner
Date:
Dimitri,

leaving the template vs link model aside for a moment, here are some
other issues I run into. All under the assumption that we want the link
model.

On 07/08/2013 11:49 AM, Dimitri Fontaine wrote:
> Please find attached to this mail version 9 of the Extension Templates
> patch with fixes for the review up to now.

First of all, I figured that creation of a template of a newer version
is prohibited in case an extension exists:

> db1=# CREATE TEMPLATE FOR EXTENSION foo VERSION '0.0' AS $foo$ SELECT 1; $foo$;
> CREATE TEMPLATE FOR EXTENSION
> db1=# CREATE EXTENSION foo;
> CREATE EXTENSION
> db1=# CREATE TEMPLATE FOR EXTENSION foo VERSION '0.1' AS $foo$ SELECT 2; $foo$;
> ERROR:  extension "foo" already exists

Why is that?

I then came to think of the upgrade scripts... what do we link against
if an extension has been created from some full version and then one or
more upgrade scripts got applied?

Currently, creation of additional upgrade scripts are not blocked. Which
is a good thing, IMO. I don't like the block on newer full versions.
However, the upgrade doesn't seem to change the dependency, so you can
still delete the update script after the update. Consider this:

> db1=# CREATE TEMPLATE FOR EXTENSION foo VERSION '0.0' AS $$ $$;
> CREATE TEMPLATE FOR EXTENSION
> db1=# CREATE EXTENSION foo;
> CREATE EXTENSION
> db1=# CREATE TEMPLATE FOR EXTENSION foo FROM '0.0' TO '0.1' AS $$ $$;
> CREATE TEMPLATE FOR EXTENSION
> db1=# ALTER EXTENSION foo UPDATE TO '0.1';
> ALTER EXTENSION
> db1=# SELECT * FROM pg_extension;
>  extname | extowner | extnamespace | extrelocatable | extversion | extconfig | extcondition 
> ---------+----------+--------------+----------------+------------+-----------+--------------
>  plpgsql |       10 |           11 | f              | 1.0        |           | 
>  foo     |       10 |         2200 | f              | 0.1        |           | 
> (2 rows)
> 
> db1=# DROP TEMPLATE FOR EXTENSION foo FROM '0.0' TO '0.1';
> DROP TEMPLATE FOR EXTENSION
>> db1=# SELECT * FROM pg_extension;
>>  extname | extowner | extnamespace | extrelocatable | extversion | extconfig | extcondition 
>> ---------+----------+--------------+----------------+------------+-----------+--------------
>>  plpgsql |       10 |           11 | f              | 1.0        |           | 
>>  foo     |       10 |         2200 | f              | 0.1        |           | 
>> (2 rows)
>> 

In this state, extension foo as of version '0.1' is installed, but
running this through dump & restore, you'll only get back '0.0'.

Interestingly, the following "works" (in the sense that the DROP of the
upgrade script is prohibited):

> db1=# CREATE TEMPLATE FOR EXTENSION foo VERSION '0.0' AS $$ $$;
> CREATE TEMPLATE FOR EXTENSION
> db1=# CREATE TEMPLATE FOR EXTENSION foo FROM '0.0' TO '0.1' AS $$ $$;
> CREATE TEMPLATE FOR EXTENSION
> db1=# CREATE EXTENSION foo VERSION '0.1';
> CREATE EXTENSION
> db1=# DROP TEMPLATE FOR EXTENSION foo FROM '0.0' TO '0.1';
> ERROR:  cannot drop update template for extension foo because other objects depend on it
> DETAIL:  extension foo depends on control template for extension foo
> HINT:  Use DROP ... CASCADE to drop the dependent objects too.

However, in that case, you are free to drop the full version:

> db1=# DROP TEMPLATE FOR EXTENSION foo VERSION '0.0';
> DROP TEMPLATE FOR EXTENSION

This certainly creates a bad state that leads to an error, when run
through dump & restore.


Maybe this one...

> db1=# CREATE TEMPLATE FOR EXTENSION foo VERSION '0.0' AS $$ $$;
> CREATE TEMPLATE FOR EXTENSION
> db1=# CREATE TEMPLATE FOR EXTENSION foo FROM '0.0' TO '0.1' AS $$ $$;
> CREATE TEMPLATE FOR EXTENSION
> db1=# DROP TEMPLATE FOR EXTENSION foo VERSION '0.0';
> DROP TEMPLATE FOR EXTENSION

... should already err out here ...

> db1=# CREATE EXTENSION foo;
> ERROR:  Extension "foo" is not available from "/tmp/pginst/usr/local/pgsql/share/extension" nor as a template
> db1=# CREATE EXTENSION foo VERSION '0.1';
> ERROR:  Extension "foo" is not available from "/tmp/pginst/usr/local/pgsql/share/extension" nor as a template

... and not only here.

I.e. the TO version should probably have a dependency on the FROM
version (that might even be useful in the template model).


Another thing that surprised me is the inability to have an upgrade
script *and* a full version (for the same extension target version).
Even if that's intended behavior, the error message could be improved:

> db1=# CREATE TEMPLATE FOR EXTENSION foo VERSION '0.0' AS $$ $$;
> CREATE TEMPLATE FOR EXTENSION
> db1=# CREATE TEMPLATE FOR EXTENSION foo FROM '0.0' TO '0.1' AS $$ $$;
> CREATE TEMPLATE FOR EXTENSION
> db1=# CREATE TEMPLATE FOR EXTENSION foo VERSION '0.1' AS $$ $$;
> ERROR:  duplicate key value violates unique constraint "pg_extension_control_name_version_index"
> DETAIL:  Key (ctlname, ctlversion)=(foo, 0.1) already exists.


Regards

Markus Wanner



Re: Review: extension template

From
Dimitri Fontaine
Date:
Markus Wanner <markus@bluegap.ch> writes:
> First of all, I figured that creation of a template of a newer version
> is prohibited in case an extension exists:

Ooops, that's a bug I need to fix.

> I then came to think of the upgrade scripts... what do we link against
> if an extension has been created from some full version and then one or
> more upgrade scripts got applied?
>
> Currently, creation of additional upgrade scripts are not blocked. Which
> is a good thing, IMO. I don't like the block on newer full versions.
> However, the upgrade doesn't seem to change the dependency, so you can
> still delete the update script after the update. Consider this:

We should allow both cases and move the dependency when a script is
installed, and also maintain dependencies in between an upgrade script
and its previous element in the link, either another upgrade script or
the default_full_version you can start with… actually, both.

> I.e. the TO version should probably have a dependency on the FROM
> version (that might even be useful in the template model).

Agreed.

> Another thing that surprised me is the inability to have an upgrade
> script *and* a full version (for the same extension target version).
> Even if that's intended behavior, the error message could be improved:

Will fix too. Thanks for your extended testing!

Josh, if running out of time on this CF, feel free to mark this one as
"returned with feedback": I will still try to make it under the current
commit fest, but with CHAR(13) starting soon and the current state of
the patch it's clearly reasonnable to say it's not ready yet.

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



Re: Review: extension template

From
Markus Wanner
Date:
Salut Dimitri,

On 07/09/2013 12:40 PM, Dimitri Fontaine wrote:
> Markus Wanner <markus@bluegap.ch> writes:
>> Or how do you think would pg_restore fail, if you followed the mental
>> model of the template?
> 
>   # create template for extension foo version 'x' as '';
>   # create extension foo;
>   # alter template for extension foo rename to bar;
> 
>   $ pg_dump | psql
> 
> And now it's impossible to CREATE EXTENSION foo, because there's no
> source to install it from available. I think we should actively prevent
> that scenario to happen in the field (current patch prevents it).

I see. You value that property a lot.

However, I don't think the plain ability to create an extension is quite
enough to ensure a consistent restore, though. You'd also have to ensure
you recreate the very *same* contents of the extension that you dumped.

Your patch doesn't do that. It seems to stop enforcing consistency at
some arbitrary point in between. For example, you can ALTER a function
that's part of the extension. Or ALTER TEMPLATE FOR EXTENSION while an
extension of that version is installed.

> Usually what you do when you want to change an extension is that you
> provide an upgrade script then run it with ALTER EXTENSION UPDATE.

As a developer, I often happen to work on one and the same version, but
test multiple modifications. This ability to change an extension
"on-the-fly" seems pretty valuable to me.

> The current file system based extensions allow you to maintain
> separately the files foo--1.0.sql and foo--1.2.sql, and you don't need
> to copy a current version of the whole extension away before hacking the
> new version.
>
> The Extension Template facility in the current patch allows you to do
> much the same

Sure. I'm aware of that ability and appreciate it.

> I see that you've spent extra time and effort to better understand any
> community consensus that might exist around this patch series, and I
> want to say thank you for that!

Thank you for your patience in pursuing this and improving user
experience with extensions. I really appreciate this work.

> Basically what I'm saying in this too long email is that I need other
> contributors to voice-in.

I fully agree. I don't want to hold this patch back any further, if it's
just me.

>> I really recommend to rename the feature (and the
>> commands), in that case, though. We may rather call the existing
>> file-system thingie an "extension template", instead, as it becomes a
>> good differentiator to what you're proposing.
> 
> Any proposals?

"Inline extension" is a bit contradictory. Maybe "managed extension"
describes best what you're aiming for.

In a similar vein, "out-of-line extension" sounds redundant to me, so
I'd rather characterize the file-system thingie as "extension templates"
wherever a clear distinction between the two is needed.

>> How about ALTER EXTENSION ADD (or DROP)? With the link on the
>> "template", you'd have to prohibit that ALTER as well, based on the
>> exact same grounds as the RENAME: The installed extension would
>> otherwise differ from the "template" it is linked to.
> 
> You're "supposed" to be using that from within the template scripts
> themselves. The main use case is the upgrade scripts from "unpackaged".

Agreed. The documentation says it's "mainly useful in extension update
scripts".

> I could see foreclosing that danger by enforcing that creating_extension
> is true in those commands.

For managed extensions only, right? It's not been prohibited for
extensions so far.

>> See how this creates an animal pretty different from the current
>> extensions? And IMO something that's needlessly restricted in many ways.
> 
> Well really I'm not convinced. If you use ALTER EXTENSION ADD against an
> extension that you did install from the file system, then you don't know
> what will happen after a dump and restore cycle, because you didn't
> alter the files to match what you did, presumably.

Yeah. The user learned to work according to the template model. Maybe
that was not the best model to start with. And I certainly understand
your desire to ensure a consistent dump & restore cycle. However...

> If you do the same thing against an extension that you did install from
> a catalog template, you just managed to open yourself to the same
> hazards… 

... I think the current patch basically just moves the potential
hazards. Maybe these moved hazards are less dramatic and justify the
move. Let's recap:

In either case (template model & link model) the patch:
a) guarantees to restore the template scripts and settings of   all managed extensions

With the link model (and v9 of your patch, which includes the RENAME
fix, and pretending there are no other bugs):
b) it guarantees *some* revision of an extension version (identified   by name and version) that has been created at
dumptime can be   restored from a template from the dump
 

If you'd additionally restrict ALTER EXTENSION ADD/DROP as well as ALTER
TEMPLATE FOR EXTENSION AS ..., you'd also get:
c) it guarantees the set of objects created by managed extensions   is restored to exactly the same set as the one that
gotdumped
 

So far, neither template nor link model guarantee:
d) the contents (functions, types, operators, ...) of the actual   extension installed matches the contents before the
dump

In the template model, you'd get the following benefit:
e) templates (esp. upgrade scripts) can be used in a mix and match   fashion, it doesn't matter whether in-line or
out-of-linewas   used, you can always apply update scripts from any of the two.
 

Given d), I personally don't value b) and c) all that much and rather
accuse them of pretending a bit of a false safety.

I'm pretty keen about a) and really appreciate that part of Dimitri's
patch. I see great value in e) as well.

What do others think?

Regards

Markus Wanner



Re: Review: extension template

From
Peter Eisentraut
Date:
On 7/8/13 4:20 AM, Dimitri Fontaine wrote:
> Let me stress that the most important value in that behavior is to be
> able to pg_restore using a newer version of the extension, the one that
> works with the target major version. When upgrading from 9.2 to 9.3 if
> you depend on keywords that now are reserved you need to install the
> newer version of the extension at pg_restore time.

I think there is an intrinsic conflict here.  You have things inside the
database and outside.  When they depend on each other, it gets tricky.
Extensions were invented to copy with that.  They do the job, more or
less.  Now you want to take the same mechanism and apply it entirely
inside the database.  But that wasn't the point of extensions!  That's
how you get definitional issues like, should extensions be dumped or not.

I don't believe the above use case.  (Even if I did, it's marginal.)
You should always be able to arrange things so that an upgrade of an
inside-the-database-package is possible before or after pg_restore.
pg_dump and pg_restore are interfaces between the database and the
outside.  They should have nothing to do with upgrading things that live
entirely inside the database.

There would be value to inside-the-database package management, but it
should be a separate concept.



Re: Review: extension template

From
Markus Wanner
Date:
Peter,

On 07/09/2013 11:04 PM, Peter Eisentraut wrote:
> I think there is an intrinsic conflict here.  You have things inside the
> database and outside.  When they depend on each other, it gets tricky.
> Extensions were invented to copy with that.  They do the job, more or
> less.

I agree. And to extend upon that, I think it's important to distinguish
between the created extension and the available one, i.e. the template.
Only the template lives outside. The created extension itself is firmly
sitting in the database, possibly with multiple dependencies from other
objects. It does not dependent on anything outside of the database
(assuming the absence of a DSO of the extension, which does not follow
that template concept).

And yes, we decided the objects that are part of the extension should
not get dumped with pg_dump. Nobody argues to change that. Note,
however, that this very decision is what raises the "intrinsic conflict"
for pg_restore, because CREATE EXTENSION in the dump depends on the
outside extension. If anything, Dimitri's patch solves that.

> Now you want to take the same mechanism and apply it entirely
> inside the database.  But that wasn't the point of extensions!  That's
> how you get definitional issues like, should extensions be dumped or not.

IMO the point of extensions is to extend Postgres (with code that's not
part of core). Whether their templates (SQL sources, if you want) are
stored on the file system (outside) or within the database is irrelevant
to the concept.

Think of it that way: Take one of those FUSE-Postgres-FS things [1],
which uses Postgres as the backend for a file system and allows you to
store arbitrary files. Mount that to the extensions directory of your
Postgres instance and make your extension templates available there
(i.e. copy them there). CREATE EXTENSION would just work, reading the
templates for the extension to create from itself, via that fuse
wrapper. (If the FUSE wrapper itself was using an extension, you'd get
into an interesting chicken and egg problem, but even that would be
resolvable, because the installed extension doesn't depend on the
template it was created from.)

Think of Dimitri's patch as a simpler and more elegant way to achieve
the very same thing. (Well, modulo our disagreement about the dependency
between extension and templates.) And as opposed to the file system or
fuse approach, you'd even gain transactional safety, consistency (i.e. a
constraint can enforce a full version exists as the basis for an upgrade
script), etc... But who am I to tell you the benefits of storing data in
a database?

Of course, you then also want to be able to backup your templates (not
the extensions) stored in the database. Just like you keep a backup of
your file-system templates. Either by simply making a copy, or maybe by
keeping an RPM or DEB package of it available. Thus, of course,
templates for extensions need to be dumped as well.

> I don't believe the above use case.  (Even if I did, it's marginal.)
> You should always be able to arrange things so that an upgrade of an
> inside-the-database-package is possible before or after pg_restore.

Dimitri's scenario assumes an old and a new version of an extension as
well as an old and a new Postgres major version. Where the old extension
is not compatible with the new Postgres major version. Which certainly
seems like a plausible scenario to me (postgis-2.0 is not compatible
with Postgres-9.3, for example - granted, it carries a DSO, so it's not
really a good example).

Given how extensions work, to upgrade to the new Postgres major version
*and* to the required new version of the extension, you don't ever need
to "upgrade an inside-the-database-package". Instead, you need to:

createdb -> provide templates -> CREATE EXTENSION -> restore data

Now, CREATE EXTENSION and restoring your data has effectively been
merged for the user, as pg_dump emits proper CREATE EXTENSION commands.
"Providing templates" so far meant installing an RPM or DEB. Or copying
the files manually.

But in fact, how and where you provide templates for the extension is
irrelevant to that order. And the possibility to merge the second step
into the 'restore data' step certainly sounds appealing to me.

> pg_dump and pg_restore are interfaces between the database and the
> outside. They should have nothing to do with upgrading things that live
> entirely inside the database.

I don't get your point here. In my view, libpq is intended to modify the
things that live inside the database, including extensions (i.e. ALTER
EXTENSION ADD FUNCTION). Or what kind of "things that live entirely
inside the database" do you have in mind.

> There would be value to inside-the-database package management, but it
> should be a separate concept.

Anything that's incompatible to extensions is not gonna fly. There are
too many of them available, already. We need to ease management of
those, not come up with yet another concept.

Regards

Markus Wanner


[1]: for example, pgfuse:
database in user-space filesystem accessing a Postgresql database:
https://github.com/andreasbaumann/pgfuse



Re: Review: extension template

From
Dimitri Fontaine
Date:
Hi,

Please find attached a new version (v10) of the patch that fixes the
reported dependencies problems and add some new regression tests to
cover them.

The patch implements the solution we discuted privately with Markus
while at the CHAR(13) conference:

  - create template for extension is now possible even if an extension
    is already installed, so that you can install a template for a new
    version of the extension;

  - all the scripts used to install an extension are now set as
    dependencies so that you can't drop parts of what you need at
    restore time;

  - you can create extension for template x version 'y' when you already
    had an upgrade path leading to that same version 'y', but only if
    your set of parameters for the version 'y' remains the same as
    what's already installed in the auxilliary control entry;

  - fix a pg_dump bug by using special dollar quoting $extname$.

Markus Wanner <markus@bluegap.ch> writes:
>> db1=# CREATE TEMPLATE FOR EXTENSION foo VERSION '0.1' AS $foo$ SELECT 2; $foo$;
>> ERROR:  extension "foo" already exists

Fixed in the attached.

>> db1=# DROP TEMPLATE FOR EXTENSION foo FROM '0.0' TO '0.1';
>> DROP TEMPLATE FOR EXTENSION
>
> In this state, extension foo as of version '0.1' is installed, but
> running this through dump & restore, you'll only get back '0.0'.

Fixed in the attached.

> This certainly creates a bad state that leads to an error, when run
> through dump & restore.

Fixed in the attached.

>> db1=# DROP TEMPLATE FOR EXTENSION foo VERSION '0.0';
>> DROP TEMPLATE FOR EXTENSION
>
> ... should already err out here ...

Fixed in the attached.

> Another thing that surprised me is the inability to have an upgrade
> script *and* a full version (for the same extension target version).
> Even if that's intended behavior, the error message could be improved:

Fixed in the attached by allowing both to co-exist.

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


Attachment