I wrote:
> With this patch, a CLOBBER_CACHE_ALWAYS build starts falling apart all
> over the place :-(. Looks like you blew the memory management somehow;
> it appears to be using a previously pfree'd pointer:
Actually, there were three different problems there:
1. Relying on a HASH_REMOVE'd hashtable entry to still be present and
valid. This is at least bad style. I wonder if we should tweak the
dynahash code to memset a free'd entry to 7F's like pfree does.
2. Assuming that a cache entry will remain in existence across a catalog
access. This will work except when it doesn't, ie, when a cache flush
occurs during the table access. You've just learned the hard way what
CLOBBER_CACHE_ALWAYS testing is good for ;-)
3. Invoking tablespace_reloptions while switched into
CacheMemoryContext. This isn't a crasher, but it strikes me as a bad
idea because if reloptions.c happens to leak anything, that'll represent
a permanent (session-lifespan) memory leak. And it's complicated enough
that being sure it doesn't leak anything is hard. I think you should
invoke tablespace_reloptions in the caller's memory context, and then if
it succeeds, copy the result into CacheMemoryContext. That would
probably require fixing the problem you noted earlier today about
making TableSpaceOpts be a valid bytea value, so that it'll be easy to
copy (then you can use datumCopy, for instance).
I fixed the first two because they were in the way of investigating
another problem, but #3 still needs your attention.
regards, tom lane