Re: Extensions, patch v20 (bitrot fixes) (was: Extensions, patch v19 (encoding brainfart fix))

On Sat, Dec 18, 2010 at 2:39 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:
> Here it is, attached. The problem was of course due to git branches and
> missing local pulling and merging. Going gradually from 7 to 2 branches
> causes that, sometimes.

I spent a little time looking at this tonight.  I'm going to give you
the same general advice that I've given other people who have
submitted very large patches of this type: it'll be a lot easier to
get this committed if we can break it into smaller pieces.  A pretty
easy thing to do would be to separate the changes that turn the
existing contrib modules into extensions into its own patch.  A
possibly more controversial idea is to actually remove the notion of a
relocatable extension from this patch, and all the supporting
machinery, and make that a follow-on patch.  I think it was arranged
like that before and somehow got collapsed, but maybe the notion is
worth revisiting, because this is a lot of code:
224 files changed, 4713 insertions(+), 293 deletions(-)

That issue aside, I think there is still a fair amount of cleanup and
code review that needs to happen here before this can be considered
for commit.  Here are a few things I noticed on a first read-through:

- pg_execute_sql_string() and pg_execute_sql_file() are documented,
but are not actually part of the patch.

- The description of the NO USER DATA option is almost content-free.
It basically says "this option is here because it wouldn't work if we
didn't have this option".

- listDependentObjects() appears to be largely cut-and-pasted from
some other function.  It calls AcquireDeletionLock() and the comments
mention constructing a list of objects to delete.  It sure doesn't
seem right to be acquiring AccessExclusiveLocks on lots of things in a
function that's there to support the pg_extension_objects SRF.

- This bit certainly violates our message style guidelines:

+       elog(NOTICE,
+                "Installing extension '%s' from '%s', in schema %s,
%s user data",
+                stmt->extname,
+                get_extension_absolute_path(control->script),
+                schema ? schema : "public",
+                create_extension_with_user_data ? "with" : "without");

User-facing messages need to be ereport, not elog, so they can be
translated, and mustn't be assembled from pieces, as you've done with
"%s user data".

- Has the issue of changing custom_variable_classes from PGC_SIGHUP to
PGC_SUSET been discussed?  I am not sure whether that's an OK thing to
do.  If it is OK, then the documentation also needs updating.

- This comment looks like leftovers:

+       /* FIXME: add PGC_EXTENSION so that we don't abuse PGC_SIGHUP here? */
+       SetConfigOption("custom_variable_classes",
+                                       newval, PGC_SIGHUP, PGC_S_EXTENSION);

Apologies if I missed the previous discussion of this, but why are we
adding a new GUC context?

- Did we decide to ditch the encoding parameter for extension scripts
and mandate UTF-8?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company