On 01/17/2011 06:53 PM, Dimitri Fontaine wrote:
Usability review:
The patch implements a way to create extensions. While the patch is labeled
extensions support for pg_dump, it actually implements more. It implements a
new way to package and install extension, and changes contrib extensions to
use the new way.
Well, all there's in the patch is infrastructure to be able to issue
those single lines in your dump :)
Is this supposed to be used mainly by contrib and PGXN extensions? When
I saw the documentation, I immediately thought that this is a nice way
to package my application's stored procedures. If this is not one of the
intended usages, it should be documented. I can see that this could be
problematic when updating PostgreSQL and when recovering from backups.
When recovering from backup, you need to have the locally created
extension available. But it might be that the extension files are lost
when the system went down in flames. Now, the backup is unusable
(right?) until extension files can be recovered from source control or
where ever they might be stored. This is why I suggested having multiple
locations for the extensions. It would be easy to backup locally created
extensions if those were in a single directory. All in all, I have a
nervous feeling that somebody someday will have an unusable dump because
they used this feature, but do not have the extension files available...
Also, this part of documentation:
The goal of using extensions is so that <application>pg_dump</> knows
not to dump all the object definitions into your backups, but rather
issue a single <xref linkend="SQL-CREATEEXTENSION"> command.
From that, it is not entirely clear to me why this is actually wanted
in PostgreSQL. I suppose this will make dump/restore easier to use. But
from that paragraph, I get the feeling the only advantage is that your
dump will be smaller. And I still have a feeling this implements more.
Not that it is a bad thing at all.
It used to work this way with \i, obviously. Should the extension patch
care about that and how? Do we want to RESET search_path or to manually
manage it like we do for the client_min_messages and log_min_messages?
It was unintuitive to me to have search_path changed by SQL command as a
side effect. When using \i, not so much.
When trying to load extensions which contain identical signatured functions,
if the loading will succeed is dependent on the search path. If there is a
conflicting function in search path (first extension loaded to schema
public), then the second extension load will fail. But if the order is
reversed (first extension loaded to schema foo which is not in search path),
then the second extension can be loaded to the public schema.
Well I think that's an expected limitation here. In the future we might
want to add suport for inter-extension dependencies and conflicts, but
we're certainly not there yet.
OK with this as is. It is just a bit surprising that you can create the
extensions in one order but not in another.
There is no validation for the extension names in share/contrib/. It is
possible to have extensions control files named ".control", ".name.control",
"name''.control" etc. While it is stupid to name them so, it might be better
to give an explicit warning / error in these cases. It is of course possible
to use these extensions.
I don't have a strong opinion here, will wait for some votes.
Yeah, this is not a big problem in practice. Just don't name them like
that. And if you do, you will find out soon enough that you made a
mistake. By the way, in my comment above "It is of course possible to
use these extensions." -> "It is of course NOT possible ...".
I haven't had time to review the pg_dump part of the patch yet, will do that
next (tomorrow). I hope it is OK to post a partial review...
From here, it's very good! Will you continue from the git repository,
where the fixes are available, or do you want a v26 already?
It is easy for me to continue from the Git repo. I will next continue
doing the pg_dump part of the review. I hope I have time to complete
that today.
- Anssi
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
