Re: POC: Sharing record typmods between backends

On 2017-08-21 11:02:52 +1200, Thomas Munro wrote:
> 2. Andres didn't like what I did to DecrTupleDescRefCount, namely
> allowing to run when there is no ResourceOwner. I now see that this
> is probably an indication of a different problem; even if there were a
> worker ResourceOwner as he suggested (or perhaps a session-scoped one,
> which a worker would reset before being reused), it wouldn't be the
> one that was active when the TupleDesc was created. I think I have
> failed to understand the contracts here and will think/read about it
> some more.

Maybe I'm missing something, but isn't the issue here that using
DecrTupleDescRefCount() simply is wrong, because we're not actually
necessarily tracking the TupleDesc via the resowner mechanism?

If you look at the code, in the case it's a previously unknown tupledesc
it's registered with:

entDesc = CreateTupleDescCopy(tupDesc);
...
/* mark it as a reference-counted tupdesc */
entDesc->tdrefcount = 1;
...
RecordCacheArray[newtypmod] = entDesc;
...

Note that there's no PinTupleDesc(), IncrTupleDescRefCount() or
ResourceOwnerRememberTupleDesc() managing the reference from the
array. Nor was there one before.

We have other code managing TupleDesc lifetimes similarly, and look at
how they're freeing it:
/* Delete tupdesc if we have it */
if (typentry->tupDesc != NULL)
{
/*
* Release our refcount, and free the tupdesc if none remain.
* (Can't use DecrTupleDescRefCount because this reference is not
* logged in current resource owner.)
*/
Assert(typentry->tupDesc->tdrefcount > 0);
if (--typentry->tupDesc->tdrefcount == 0)
FreeTupleDesc(typentry->tupDesc);
typentry->tupDesc = NULL;
}

This also made me think about how we're managing the lookup from the
shared array:

/*
* Our local array can now point directly to the TupleDesc
* in shared memory.
*/
RecordCacheArray[typmod] = tupdesc;

Uhm. Isn't that highly highly problematic? E.g. tdrefcount manipulations
which are done by all lookups (cf. lookup_rowtype_tupdesc()) would in
that case manipulate shared memory in a concurrency unsafe manner.

Greetings,

Andres Freund