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

I used the patch from CommitFest application and applied the following
commit to fix a known issue:

http://git.postgresql.org/gitweb?p=postgresql-extension.git;a=commitdiff;h=d4991d35283ae0ceeb7f9e4203cf6a9dfb5d128d

Is the patch in context diff format?
Yes.

Does the patch apply cleanly?
No:
patching file src/include/commands/defrem.h
Hunk #2 succeeded at 107 with fuzz 2 (offset 27 lines).
Hunk #3 FAILED at 105.
1 out of 5 hunks FAILED -- saving rejects to file
src/include/commands/defrem.h.rej

I have used the git head of

http://git.postgresql.org/gitweb?p=postgresql-extension.git branch extensions

to do the rest of reviewing. There is one compiler warning:

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith

-Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing
-fwrapv -g -I../../../src/interfaces/libpq -I../../../src/include -D_GNU_
SOURCE   -c -o pg_dump.o pg_dump.c
pg_dump.c: In function ‘getTables’:
pg_dump.c:3748: warning: too many arguments for format


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

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.

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

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.

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.

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.

Feature test:

The packaging feature works as advertised, expect for the following bugs
and inconsistencies.

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.

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.

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.

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?

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.

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.

I was able to make it crash:

postgres=# alter extension foo.bar set schema baz;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

Log contains this:
TRAP: FailedAssertion("!(list_length(name) == 1)", File: "extension.c",
Line: 1163)

It doesn't matter if the schema foo exists or not, and if the extension
is there or not.

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

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

- Anssi