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(at)2ndquadrant(dot)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