Re: Extensions, this time with a patch

Excerpts from Alvaro Herrera's message of lun oct 25 10:37:22 -0300 2010:
> Excerpts from Alvaro Herrera's message of vie oct 22 17:02:22 -0300 2010:
>
> > > I'll go rework the patch sometime later to drop the name from the
> > > control file, but that also means fixing several contrib modules by
> > > changing their file names, operation for which the project has no policy
> > > yet as far as I understand (we used CVS before).
> >
> > Change what file names? You lost me there. I thought the extension
> > name was going to be equal to the control file name, and said control
> > file doesn't exist yet, so you don't need to rename any existant file.
> > Am I confusing something?
>
> Hmm, after reading the latest blog post, it seems that the patch
> requires that the control file is equal to the .sql install script. Is
> this the case? I don't see a reason for this requirement; why not
> simply have a line for the install script in the control file? That
> way, you don't need to rename the .sql files.

Ah, some reading of the patch reveals that the "script" defaults to the
control file name, but it can be overridden.

I noticed that you're using ExclusiveLock when creating an extension,
citing the handling of the global variable create_extension for this.
There are two problems here: one is that you're releasing the lock way
too early: if you wanted this to be effective, you'd need to hold on to
the lock until after you've registered the extension.

The other is that there is no need for this at all, because this backend
cannot be concurrently running another CREATE EXTENSION comand, and
this is a backend-local variable. So there's no point.

Why palloc create_extension every time? Isn't it better to initialize
it properly and have a boolean value telling whether it's to be used?
Also, if an extension fails partway through creation, the var will be
left set. I think you need a PG_TRY block to reset it.

(I find the repeated coding pattern that tests create_extension for
NULL-ness before calling recordDependencyOn a bit awkward; maybe hide it
in a function or macro? But then maybe that's just me. Also, why
palloc it? Seems better to have it static. Notice your new calls to
recordDependencyOn are the only ones with operands not using the &
operator.)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support