Re: [PATCH] postgres_fdw extension support

On October 6, 2015 at 6:32:36 AM, Andres Freund (andres(at)anarazel(dot)de(mailto:andres(at)anarazel(dot)de)) wrote:

> On 2015-10-06 06:28:34 -0700, Paul Ramsey wrote:
> > +/*
> > + * is_shippable
> > + * Is this object (proc/op/type) shippable to foreign server?
> > + * Check cache first, then look-up whether (proc/op/type) is
> > + * part of a declared extension if it is not cached.
> > + */
> > +bool
> > +is_shippable(Oid objnumber, Oid classnumber, List *extension_list)
> > +{
> > + ShippableCacheKey key;
> > + ShippableCacheEntry *entry;
> > +
> > + /* Always return false if the user hasn't set the "extensions" option */
> > + if (extension_list == NIL)
> > + return false;
> > +
> > + /* Find existing cache, if any. */
> > + if (!ShippableCacheHash)
> > + InitializeShippableCache();
> > +
> > + /* Zero out the key */
> > + memset(&key, 0, sizeof(key));
> > +
> > + key.objid = objnumber;
> > + key.classid = classnumber;
> > +
> > + entry = (ShippableCacheEntry *)
> > + hash_search(ShippableCacheHash,
> > + (void *) &key,
> > + HASH_FIND,
> > + NULL);
> > +
> > + /* Not found in ShippableCacheHash cache. Construct new entry. */
> > + if (!entry)
> > + {
> > + /*
> > + * 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, classnumber, extension_list);
> > +
> > + /*
> > + * Don't create a new hash entry until *after* we have the shippable
> > + * result in hand, as the shippable lookup might trigger a cache
> > + * invalidation.
> > + */
> > + entry = (ShippableCacheEntry *)
> > + hash_search(ShippableCacheHash,
> > + (void *) &key,
> > + HASH_ENTER,
> > + NULL);
> > +
> > + entry->shippable = shippable;
> > + }
> > +
> > + if (!entry)
> > + return false;
> > + else
> > + return entry->shippable;
> > +}
>
> Maybe I'm missing something major here. But given that you're looking up
> solely based on Oid objnumber, Oid classnumber, how would this cache
> work if there's two foreign servers with different extension lists?

*sigh*, no you’re not missing anything. In moving to the cache and marking things just as “shippable” I’ve lost the test that ensures they are shippable for this *particular* server (which only happens in the lookup stage). So yeah, the cache will be wrong in cases where different servers have different extension opti ons.

Thanks, 
P.

--
http://postgis.net
http://cleverelephant.ca