Re: Extensions, patch v20 (bitrot fixes)

Hi,

Thanks for your review and your time. Trying to answer some of your
points there:

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.

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

Again, will split the code in a third patch if that's what's necessary.

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

Ouch, git ain't that magic after all. Will clean-up next version(s).

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

> - 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?

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

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

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

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

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