Re: TupleDesc refcounting

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Neil Conway <neilc(at)samurai(dot)com>
Cc: pgsql-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: TupleDesc refcounting
Date: 2006-06-15 19:13:12
Message-ID: 29987.1150398792@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-patches

I'm finally getting back to looking at the problem of reference-counting
cached TupleDescs as was discussed in January. I had objected to the
last patch Neil posted:
http://archives.postgresql.org/pgsql-patches/2006-01/msg00243.php
on the grounds that it seemed too complicated. On looking at it
closely, I realize that a lot of the complexity comes from my insistence
that the ResourceOwner mechanism ought to warn about any tupdesc
references that haven't been explicitly released before transaction end.
(Neil had questioned upthread whether that was really necessary ... he
was right.) The problem is that execQual.c would like to hang onto
tupdesc references across multiple executions of expression nodes, and
there's not any explicit code to clean up expression trees, so no easy
place to put a refcount decrement. Neil's patch arranges to clean up
such references by using expression shutdown callbacks. The patch
doesn't quite work as-is because it neglects the fact that shutdown
callbacks are triggered during a node rescan as well as node shutdown.
This could be fixed, but I'm thinking that it's a lot of mechanism to
add when the ResourceOwner could perfectly well clean up the refcounts
instead.

The point of the no-remaining-refs rule was to catch code that should
have released a refcount and failed to. We could keep that error
checking if we made ResourceOwner recognize two different kinds of
tupdesc refcount, one to warn about and one not to. I'm not sure if
that's worth the trouble or not --- any opinions?

Also, I looked at whether TupleTableSlots could sensibly use refcounted
tuples, and I'm now of the opinion that it'd be a waste of time.
There aren't any places where the executor uses a tupdesc obtained from
cache as a TupleTableSlot's descriptor; rather, all the tupdescs
involved are built in query-lifespan memory during plan startup.
So I'm thinking we should actually get rid of tts_shouldFreeDesc and
not have execTuples.c do any tupdesc-freeing at all. Retail freeing
of structures that are in a memory context that's about to be freed
is a waste of cycles. We've gotten rid of a fair amount of redundant
pfree's that used to occur during plan shutdown, and these are some
more that we could do without.

Comments?

regards, tom lane

In response to

Responses

Browse pgsql-patches by date

  From Date Subject
Next Message Bruce Momjian 2006-06-15 19:15:01 Re: Free WAL caches on switching segments
Previous Message Bruce Momjian 2006-06-15 18:02:12 Re: [PATCHES] PL/pgSQL: SELECT INTO EXACT