Horton

Re: POC: Sharing record typmods between backends

Andres Freund <andres@anarazel.de>

View thread View on postgr.es
From: Andres Freund <andres@anarazel.de>
To: Thomas Munro <thomas.munro@enterprisedb.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, Pg Hackers <pgsql-hackers@postgresql.org>
Date: 2017-08-25T01:46:20Z
Lists: pgsql-hackers 
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

Commits

  1. Remove TupleDesc remapping logic from tqueue.c.

  2. Add support for coordinating record typmods among parallel workers.

  3. Improve division of labor between execParallel.c and nodeGather[Merge].c.

  4. Add minimal regression test for blessed record type transfer.

  5. Consolidate the function pointer types used by dshash.c.

  6. Fix unlikely shared memory leak after failure in dshash_create().

  7. Refactor typcache.c's record typmod hash table.

  8. Add a hash_combine function for mixing hash values.

  9. Backpatch introduction of TupleDescAttr(tupdesc, i).

  10. Partially flatten struct tupleDesc so that it can be used in DSM.

  11. Change tupledesc->attrs[n] to TupleDescAttr(tupledesc, n).