Re: REVIEW: Extensions support for pg_dump - Mailing list pgsql-hackers

Hi,

Thanks for your review!

Anssi Kääriäinen <anssi.kaariainen@thl.fi> writes:
> Does the patch apply cleanly?
> No:

That was some bitrot, has been fixed, thanks you for working from the
git repository meanwhile.

> pg_dump.c:3748: warning: too many arguments for format

Fixed in v25 already sent this morning.

> And, make check gives me the following errors:
> test sanity_check             ... FAILED
> test rules                    ... FAILED

Fixed too.  Was due to git conflict solving where it adds a new line in
the tests and keep the embedded rowcount the same.  I think.

> Does it include reasonable tests, necessary doc patches, etc?
>
> There is enough documentation, but I think the documentation needs some
> polishing. I am not a native English speaker, so it might be it is my
> English that is failing. The data is there, but the representation might be
> a little off.

We already made lots of improvements there thanks to David Wheeler
reviews in the previous commitfest (which shows up already), and I'll be
happy to continue improving as much as I can.  If all it needs is
english native review, I guess that'll be part of the usual marathon
Bruce runs every year there?

> I didn't spot any tests. This could be that I don't know what to look for...

make -C contrib installcheck will exercise CREATE EXTENSION for each
contrib module.

> 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 :)

> I do think we want these features, and that we do not have those already. As
> far as I understand, there is nothing in the standard regarding this
> feature.
>
> I wonder if the structure of having all the extensions in share/contrib/ is
> a good idea. It might be better if one could separate the contrib extensions
> in one place, and user created extensions in another place. This could make
> it easy to see what user created extensions is installed in (or installable
> to) the cluster. I think this might be helpful to DBAs upgrading PostgreSQL.

It's always been this way and I though it wouldn't be in this patch
scope to re-organise things.  Also I think we should include the UPGRADE
needs when we discuss file system level layout.

> Overall, the system seems intuitive to use. It is relatively easy to create
> extension using this feature, and loading contrib extensions is really
> easy. I haven't tested how easy it is to create C-language extensions using
> this. If I am not mistaken it just adds the compile & install the
> C-functions before installing the extension.

It's using PGXS which existed before, all you have to do that's new is
care about the Makefile EXTENSION variable and the control file.  Even
when doing C coded extension work.

> When using the plain CREATE EXTENSION foobar command without schema
> qualifier, the extension is created in schema public (by name) without
> regard to the current search path. This is inconsistent with create table,
> and if the public schema is renamed, the command gives error:
>
> ERROR: schema "public" does not exist
>
> This makes the name "public" have a special meaning, and I suspect that is
> not wanted.

Fixed in git, thanks for reporting!

~:5490=# set client_min_messages to debug1;
SET
~:5490=# set search_path to utils;
SET
~:5490=# create extension unaccent;
DEBUG:  parse_extension_control_file(unaccent, '/home/dfontaine/pgsql/exts/share/contrib/unaccent.control')
DEBUG:  CreateExtensionStmt: with user data, schema = 'utils', encoding = ''
DEBUG:  Installing extension 'unaccent' from '/home/dfontaine/pgsql/exts/share/contrib/unaccent.sql', in schema utils,
withuser data 
CREATE EXTENSION
~:5490=# \dx                                List of extensionsSchema |     Name      | Version  |
Description                   
--------+---------------+----------+-------------------------------------------------utils  | citext        | 9.1devel
|case-insensitive character string typeutils  | hstore        | 9.1devel | storing sets of key/value pairsutils  |
int_aggregate| 9.1devel | integer aggregator and an enumerator (obsolete)utils  | lo            | 9.1devel | managing
LargeObjectsutils  | unaccent      | 9.1devel | text search dictionary that removes accents 
(5 rows)

> When issuing CREATE EXTENSION foo SCHEMA bar, and the extension foo is not
> relocatable and it's control file uses the SET search_path TO @extschema@,
> the search_path is set to bar for the session. That is, it is not changed to
> the original search_path after the command has completed.

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?

> 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.

> While it is not possible to drop functions in extensions, it is possible to
> rename a function, and also to CREATE OR REPLACE a function in an
> extension. After renaming or CORing a function, it is possible to drop the
> function. I also wonder if alter function ... set parameter should be
> allowed?

Well there's no specific restrictions in what you can put in an
extension's SQL file.  I see that as a feature… think upgrade scripts, too.

> 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.

> If there is no comment for a given extension in the .control file, then \dx
> will not show the extension installed even if it is installed.

Fixed in the git repository.

> I was able to make it crash:
>
> postgres=# alter extension foo.bar set schema baz;

Fixed:

~:5490=# alter extension foo.bar set schema baz;
ERROR:  syntax error at or near "."
LINE 1: alter extension foo.bar set schema baz;                          ^

~:5490=# alter extension bar set schema baz;
ERROR:  extension "bar" does not exist

> I haven't done anything that could be called review on the code level, but I
> have quickly glanced over the patch. Some things that caught my eye:
>
> In file /src/backend/commands/extension.c:
> + * The extension control file format is the most simple name = value, we
> + * don't need anything more there. The SQL file to execute commands from is
> + * hardcoded to `pg_config --sharedir`/<extension>.install.sql.
>
> This seems to be outdated.

Yes it is.  Updated, but now that is covered extensively in the
documentation, maybe it could just be removed from the file here?

> In file src/bin/pg_dump/pg_dump.c:
>
> ! /*
> ! * So we want the namespaces, but we want to filter out any
> ! * namespace created by an extension's script. That's unless the
> ! * user went over his head and created objects into the extension's
> ! * schema: we now want the schema not to be filtered out to avoid:
> ! *
> ! * pg_dump: schema with OID 77869 does not exist
> ! */
>
> I wonder what that last line is doing there...

That's the error message you can easily get when you want to have
something more simple than the provided SQL…

> 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?

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