Re: Extensions, patch v20 (bitrot fixes)

On Sun, Dec 19, 2010 at 5:30 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> 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.
>
> Those are pretty mechanical, so applying them after would be quite
> easy. That said I don't know exactly how much it would ease the review,
> but if you say it does, I'll separate the patches.

Well, I don't know who is ultimately going to end up applying this. I
can only say what helps me. If somebody else wants to jump in here
and say they're good to review and commit the whole thing in one go,
so be it...

>>  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(-)
>
> It was like that before indeed. The problem is how to reliably implement
> the CREATE EXTENSION WITH SCHEMA, which is like the second best thing
> about all this infrastructure work just behind the pg_dump support.  I'm
> not clear that this option should come on its own in a later patch or
> that doing so simplifies anything, but not opinionated about that: let's
> organise ourselves as best as we are able to.

I'm not totally certain of it either, but I think it might.

>> - 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".
>
> +         (see <xref linkend="functions-extension">), this option allow
> +         extension authors to prepare installation scripts that will avoid
> +         creating user data when restoring.
>
> I'd need some specifics here to be able to do much better. My guess is
> that an example is what needed, and it would fit in the main section of
> the manual about it, 35.16. Putting it all together: create extension.

No, I think you need to describe what the option actually does, as
opposed to what problem it solves.

>> - 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.
>
> Ah well, true. AccessShareLock seems the minimum we can take, as we
> surely want to prevent the extension and its objects disappearing under
> us, right?

On two minutes thought, I'm not entirely sure that we need to take
locks at all here, but if we do, AccessShareLock is certainly the
right one. One idea I have is that we might want to move
AcquireDeletionLock to objectaddress.c or maybe even somewhere in
storage/lmgr, rename it to something like LockObjectByAddress, and
give it a second argument for the lock strength.

>> - 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".
>
> This message is pretty useful for debug and review of the patch, but I'm
> not sure we want to keep after that...

Either fix it or take it out. If you're proposing this for commit, it
shouldn't contain anything you don't want committed.

>> - 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.
>
> I've been talking about it since the very firsts versions of the patch
> on this list but didn't receive much answer. You have a detailed mail
> about that later in the thread, will start a new thread about that from
> there.

Sorry, I missed that discussion. As you know I try to follow -hackers
pretty closely, but apparently not closely enough in this case. Can
you provide links to the archives?

>> - 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?
>
> Well we're using PGC_SIGHUP and PGC_S_EXTENSION, should we use
> PGC_EXTENSION too is what the comment asks, so it's not very clear but
> not a leftover. We're adding a new GUC source here, not a new context.
>
> Let's include that in the new thread about cvc, though.

OK.

>> - Did we decide to ditch the encoding parameter for extension scripts
>> and mandate UTF-8?
>
> No we didn't, we decided that the default encoding is UTF-8 and that any
> contrib script defaults to UTF-8, so that it's not necessary to care
> about the 'encoding' parameter in the control file there.
>
> Itagaki required that the extension code is encoding aware, and it
> wasn't clear that the control file 'encoding' parameter would be enough
> in his side of the world, so the encoding support is currently offered
> to authors in the control file and users still can override it in the
> CREATE EXTENSION command.
>
> I'm about sure that we don't want to remove that support, and I think
> that the command part of it ain't required, but that's for people with
> complex encoding problems to tell us IMO.

Oh, I wasn't aware that Itagaki-san had objected to Tom's proposal.

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