Re: Extensions support for pg_dump, patch v27

Hi,

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> 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.

Thanks for the review!

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

Fixed in git. I'm not yet used to the project style where we declare
some structures into the c code rather than the headers… and then it's
cleanup.

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

I think we're good here. The extensions I know that use triggers are
installing the functions, it's still the user who CREATE TRIGGER and
uses the function. And even if we have an extension that contains some
CREATE TRIGGER commands in its script, the trigger will depend on a
function and the function on the extension.

If there's a use case for CREATE TRIGGER in an extension's script and
having the trigger not depend on another extension's object, like a
function, then I didn't see it. I'm ok to add the triggers as first
class dependency objects in the extension patch if there's a need…

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

Well I'm unconvinced by index AM extensions. Unfortunately, if you want
a crash safe AM, you need to patch core code, it's not an extension any
more. About reverting default settings, I'm not following.

What I saw is existing extensions that didn't need uninstall script once
you can DROP EXTENSION foo; but of course it would be quite easy to add
a parameter for that in the control file. Do we need it, though?

> * 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 didn't spot that, I just didn't think about that use case. On a quick
test here it seems like the dependency is not recorded in this
case. Will fix.

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

We can do that, but what's happening now is my understanding of the
consensus we reached on the list.

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

In fact earthdistance could CREATE EXTENSION cube; itself in its install
script. As you say, though, it's a rare case and it was agreed that
this feature could wait until a later version, so I didn't spend time on
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'

Oops sorry about that, I miss them too easily. What's the trick to turn
warnings into errors already please?

Fixed in the git repository.

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

That's the DATA = uninstall_ lines in the Makefile. Removed in the git
repository. Cleared.

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

It allows to get a compiler error when you mistakenly use = rather than
== because the Left Hand Side is a constant, so I got used to writing
things this way. Should I review my patch and adapt? Will do after
some votes :)

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

Will add some more documentation, ok. As far as contrib goes, I didn't
rework the install scripts.

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