Re: [PATCH] postgres_fdw extension support

Andres, 
Thanks so much for the review!

I put all changes relative to your review here if you want a nice colorized place to check

https://github.com/pramsey/postgres/commit/ed33e7489601e659f436d6afda3cce28304eba50

On October 3, 2015 at 8:49:04 AM, Andres Freund (andres(at)anarazel(dot)de) wrote:

> + /* this must have already-installed extensions */ 

I don't understand that comment. 
Fixed, I hope.

> + /* extensions is available on server */ 
> + {"extensions", ForeignServerRelationId, false}, 

awkward spelling in comment. 
Fixed, I hope.

> + * throw up an error. 
> + */ 

s/throw up an error/throw an error/ or raise an error. 
But “throw up” is so evocative :) fixed.

> + /* Optional extensions to support (list of oid) */ 

*oids 
Fixed.

> + /* Always return false if we don't have any declared extensions */ 

Imo there's nothing declared here... 
Changed...

> + if (extension_list == NIL) 
> + return false; 
> + 
> + /* We need this relation to scan */ 

Not sure what that means. 
Me neither, removed.

> + if (foundDep->deptype == DEPENDENCY_EXTENSION && 
> + list_member_oid(extension_list, foundDep->refobjid)) 
> + { 
> + is_shippable = true; 
> + break; 
> + } 
> + } 

Hm. 
I think this “hm” is addressed lower down.

> + /* Always return false if we don't have any declared extensions */ 
> + if (extension_list == NIL) 
> + return false; 

I again dislike declared here ;) 
Altered.

> + key.objid = objnumber; 

Hm. Oids can conflict between different system relations. Shouldn't the 
key be over class (i.e. pg_proc, pg_type etc.) and object id? 
I’ve changed the lookup to use class/obj instead. I’m *hoping* I don’t get burned by it, but it regresses fine at least. Each call to is_shippable now has a hard-coded class oid in it depending on the context of the call. It seemed like the right way to do it.

> + /* 
> + * Right now "shippability" is exclusively a function of whether 
> + * the obj (proc/op/type) is in an extension declared by the user. 
> + * In the future we could additionally have a whitelist of functions 
> + * declared one at a time. 
> + */ 
> + bool shippable = lookup_shippable(objnumber, extension_list); 
> + 
> + entry = (ShippableCacheEntry *) 
> + hash_search(ShippableCacheHash, 
> + (void *) &key, 
> + HASH_ENTER, 
> + NULL); 
> + 
> + entry->shippable = shippable; 
> + } 

I suggest adding a comment that we consciously are only HASH_ENTERing 
the key after doing the lookup_shippable(), to avoid the issue that the 
lookup might accept cache invalidations and thus delete the entry again. 
I’m not understanding this one. I only ever delete cache entries in bulk, when InvalidateShippableCacheCallback gets called on updates to the foreign server definition. I must not be quite understanding your gist.

Thanks!

P