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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers