Re: [PATCH] postgres_fdw extension support

On 2015-10-03 19:40:40 -0700, Paul Ramsey wrote:
> > > + /* 
> > > + * 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.

The problem is basically that cache invalidations can arrive while
you're building new cache entries. Everytime you e.g. open a relation
cache invalidations can arrive. Assume you'd written the code like:

entry = hash_search(HASH_ENTER, key, &found);

if (found)
return entry->whateva;

entry->whateva = lookup_shippable(...);
return entry->whateva;

lookup_shippable() opens a relation, which accepts invalidations. Which
then go and delete the contents of the hashtable. And you're suddenly
accessing free()d memory...

You're avoiding that by only entering into the hashtable *after* the
lookup. And I think that deserves a comment.

Andres Freund