Hi,
Thank you for your detailed reviewing, including a patch! Attached is current version of the patch, v11, with your work
andcomments included. Unfortunately it looks like the git repository won't receive any update until I'm back home, next
tuesday.
Le 25 oct. 2010 à 17:26, Alvaro Herrera a écrit :
> Ah, some reading of the patch reveals that the "script" defaults to the
> control file name, but it can be overridden.
Yes. The other mail seems to have made it finally :) twice :(
> 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.
I'm now holding the lock just while inserting in the catalogs, and rechecking for duplicates while I have the lock.
> 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.
Done in v11. I wasn't exactly sure about what to have in the CATCH block, so I've put the minimum, reseting the bool.
> (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.)
Updated to fit a better style, with bool create_extension set to true in the command only, and AddressObject extension
that'sused everywhere (&extension). No more palloc()ing here.
Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support