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
