Re: Extensions support for pg_dump, patch v27 - Mailing list pgsql-hackers

On Mon, Jan 24, 2011 at 18:22, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
> Following recent commit of the hstore Makefile cleaning, that I included
> into my extension patch too, I have a nice occasion to push version 27
> of the patch.  Please find it enclosed.

Hi, I read the patch and test it in some degree.  It works as expected
generally, but still needs a bit more development in edge case.

* commands/extension.h needs cleanup.
- Missing "extern" declarations for create_extension and
create_extension_with_user_data variables.
- ExtensionControlFile and ExtensionList can be hidden from the header.
- extension_objects_fctx is not used at all.

* There are many recordDependencyOn(&myself, &CreateExtensionAddress, ...)
in some places, but I'm not sure we forget something -- TRIGGERs?

* Will we still need uninstaller scripts?
DROP EXTENSION depends on object dependency to drop objects,
but we have a few configurations that cannot be described in the
dependency. For example, rows inserted into pg_am for user AMs
or reverting default settings modified by the extension.

* Extensions installed in pg_catalog:
If we install an extension into pg_catalog, CREATE EXTENSION xxx WITH SCHEMA pg_catalog
pg_dump dumps nothing about the extension. We might need special care
for modules that install functions only in pg_catalog.

* I can install all extensions successfully, but there is a warning
in hstore. The warning was not reported to the client, but the whole
contents of hstore.sql.in was recorded in the server log.
 WARNING:  => is deprecated as an operator name

We sets client_min_messages for the issue in hstore.sql.in, but I think
we also need an additional "SET log_min_messages TO ERROR" around it.

* Is it support nested CREATE EXTENSION calls?
It's rare case, but we'd have some error checks for it.

* It passes all regression test for both backend and contrib modules,
but there are a couple of warnings during build and installation.

Three compiler warnings found. Please check.
pg_proc.c: In function 'ProcedureCreate':
pg_proc.c:110:8: warning: 'extensionOid' may be used uninitialized in
this function
pg_shdepend.c: In function 'shdepReassignOwned':
pg_shdepend.c:1335:6: warning: implicit declaration of function
'AlterOperatorOwner_oid'
operatorcmds.c:369:1: warning: no previous prototype for
'AlterOperatorOwner_oid'

* Five warnings also found during make install in contrib directory.
../../config/install-sh: ./uninstall_btree_gist.sql does not exist.
../../config/install-sh: ./uninstall_pageinspect.sql does not exist.
../../config/install-sh: ./uninstall_pgcrypto.sql does not exist.
../../config/install-sh: ./uninstall_pgrowlocks.sql does not exist.
../../config/install-sh: ./uninstall_pgstattuple.sql does not exist.

* You use "const = expr" in some places, ex. if( InvalidOid != ...), but we seem to prefer "expr = const".

* [off topic] I found some installer scripts are inconsistent.
They uses CREATE FUNCTION for some of functions, but CREATE OR REPLACE
FUNCTION for others. We'd better write documentation about how to write
installer scripts because CREATE EXTENSION has some implicit assumptions
in them. For example, "Don't use transaction", etc.

--
Itagaki Takahiro