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


Is the patch in context diff format?

Does the patch apply cleanly?
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

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

Reply via email to