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 (and...@anarazel.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
20151003_postgres_fdw_extensions.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
