Re: Extensions, this time with a patch

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, that's new in v10. In v11 I've kept that and removed the name property in the control file, so that we have:

cat contrib/intarray/intarray.control.in
# intarray
comment = 'one-dimensional arrays of integers: functions, operators, index support'
version = 'EXTVERSION'
script = '_int.sql'

> 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 wanted to protect from another backend trying to create the same extension at the same time. So the critical path is the inserting into the catalog. I now see I failed to include the duplicate object check into the critical path, when I added that later.

Do you confirm protecting the insertion in the catalog is not worthy of special locking? To get proper locking requires some more thinking than I did put in, but if you say I'd better remove it...

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

Good catches. I'm still uneasy with which memory context what allocation belongs too, hence the palloc()ing here.

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

In fact the goal of the test is to check if we're in the code path for CREATE EXTENSION, rather than pointer validity per-say. I'll go have it static, too, with a bool to determine the code path.

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

PS: I hope to be able to send this email, but uploading the git repo will be uneasy from the wifi here at best. Will send patches if email is ok.