Re: [PATCH] postgres_fdw extension support
| От | Paul Ramsey |
|---|---|
| Тема | Re: [PATCH] postgres_fdw extension support |
| Дата | |
| Msg-id | etPan.561091a8.6b8b4567.72cd@Butterfly.local обсуждение исходный текст |
| Ответ на | Re: [PATCH] postgres_fdw extension support (Andres Freund <andres@anarazel.de>) |
| Ответы |
Re: [PATCH] postgres_fdw extension support
(Michael Paquier <michael.paquier@gmail.com>)
Re: [PATCH] postgres_fdw extension support (Andres Freund <andres@anarazel.de>) |
| Список | pgsql-hackers |
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@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
Вложения
В списке pgsql-hackers по дате отправления: