Обсуждение: Life cycles of tuple descriptors

Hi,

There are some things about the life cycle of the common TupleDesc
that I'm not 100% sure about.

1. In general, if you get one from relcache or typcache, it is
   reference-counted, right?

2. The only exception I know of is if you ask the typcache for
   a blessed one (RECORD+typmod), and it is found in the shared-memory
   cache. It is not refcounted then. If it is found only locally,
   it is refcounted.

3. Is that shared case the only way you could see a non-refcounted
   TupleDesc handed to you by the typcache?

4. How long can such a non-refcounted TupleDesc from the typcache
   be counted on to live?

   There is a comment in expandedrecord.c: "It it's not refcounted, just
   assume it will outlive the expanded object."

   That sounds like confidence, but is it confidence in the longevity
   of shared TupleDescs, or in the fleeting lives of expanded records?

   The same comment says "(This can happen for shared record types,
   for instance.)" Does the "for instance" mean there are now, or just
   in future might be, other cases where the typcache hands you
   a non-refcounted TupleDesc?

5. When a constructed TupleDesc is blessed, the copy placed in the cache
   by assign_record_type_typmod is born with a refcount of 1. Assuming every
   later user of that TupleDesc plays nicely with balanced pin and release,
   what event(s), if any, could ever occur to decrease that initial 1 to 0?

Thanks for any help getting these points straight!

Regards,
-Chap

Chapman Flack <chap@anastigmatix.net> writes:
> There are some things about the life cycle of the common TupleDesc
> that I'm not 100% sure about.

> 1. In general, if you get one from relcache or typcache, it is
>    reference-counted, right?

Tupdescs for named composite types should be, since those are
potentially modifiable by DDL.  The refcount allows a stale tupdesc
to go away when it's no longer referenced anywhere.

Tupdescs for RECORD types are a different story: there's no way to
change them once created, so they'll live as long as the process
does (at least for tupdescs kept in the typcache).  Refcounting
isn't terribly necessary in that case; and at least for the shared
tupdesc case, we don't do it, to avoid questions of modifying a
piece of shared state.

> 3. Is that shared case the only way you could see a non-refcounted
>    TupleDesc handed to you by the typcache?

I'm not sure what happens for a non-shared RECORD tupdesc, but it
probably wouldn't be wise to assume anything either way.

> 5. When a constructed TupleDesc is blessed, the copy placed in the cache
>    by assign_record_type_typmod is born with a refcount of 1. Assuming every
>    later user of that TupleDesc plays nicely with balanced pin and release,
>    what event(s), if any, could ever occur to decrease that initial 1 to 0?

That refcount is describing the cache's own reference, so as long as
that reference remains it'd be incorrect to decrease the refcount to 0.

            regards, tom lane

On 12/14/21 18:03, Tom Lane wrote:

> Tupdescs for RECORD types are a different story: ... Refcounting
> isn't terribly necessary in that case; and at least for the shared
> tupdesc case, we don't do it, to avoid questions of modifying a
> piece of shared state.

Ok, that's kind of what I was getting at here:

>> 5. When a constructed TupleDesc is blessed, the copy placed in the cache
>>    by assign_record_type_typmod is born with a refcount of 1. ...
>>    what event(s), if any, could ever occur to decrease that initial 1 ...
>
> That refcount is describing the cache's own reference, so as long as
> that reference remains it'd be incorrect to decrease the refcount to 0.

In the case of a blessed RECORD+typmod tupdesc, *is* there any event that
could ever cause the cache to drop that reference? Or is the tupdesc just
going to live there for the life of the backend, its refcount sometimes
going above and back down to but never below 1?

That would fit with your "refcounting isn't terribly necessary in that
case". If that's how it works, it's interesting having the two different
patterns: if it's a shared one, it has refcount -1 and you never fuss
with it and it never goes away; if it's a local one it has a non-negative
refcount and you go through all the motions and it never goes away anyway.

>> 3. Is that shared case the only way you could see a non-refcounted
>>    TupleDesc handed to you by the typcache?
> 
> I'm not sure what happens for a non-shared RECORD tupdesc, but it
> probably wouldn't be wise to assume anything either way.

If I'm reading this right, for the non-shared case, the copy that goes
into the cache is made refcounted. (The copy you presented for blessing
gets the assigned typmod written in it, and no change to its refcount
field.)

There's really just one thing I'm interested in assuming:

*In general*, if I encounter a tupdesc with -1 refcount, I had better not
assume much about its longevity. It might be in a context that's about to
go away. If I'll be wanting it later, I had better defensively copy it
into some context I've chosen.

(Ok, I guess if it's a tupdesc provided to me in a function call, I can
assume it is good for the duration of the call, or if it's part of an
SPI result, I can assume it's good until SPI_finish.)

But if I have gone straight to the typcache to ask for a RECORD tupdesc,
and the one it gives me has -1 refcount, is it reasonable to assume
I can retain a reference to that without the defensive copy?

Regards,
-Chap

Chapman Flack <chap@anastigmatix.net> writes:
> But if I have gone straight to the typcache to ask for a RECORD tupdesc,
> and the one it gives me has -1 refcount, is it reasonable to assume
> I can retain a reference to that without the defensive copy?

The API contract for lookup_rowtype_tupdesc specifies that you must "call
ReleaseTupleDesc or DecrTupleDescRefCount when done using the tupdesc".
It's safe to assume that the tupdesc will stick around as long as you
haven't done that.

APIs that don't mention a refcount are handing you a tupdesc of uncertain
lifespan (no more than the current query, likely), so if you want the
tupdesc to last a long time you'd better copy it into storage you control.

            regards, tom lane

On 12/14/21 20:02, Tom Lane wrote:
> The API contract for lookup_rowtype_tupdesc specifies that you must "call
> ReleaseTupleDesc or DecrTupleDescRefCount when done using the tupdesc".
> It's safe to assume that the tupdesc will stick around as long as you
> haven't done that.

I think what threw me was having a function whose API contract mentions
reference counts, but that sometimes gives me things that don't have them.

But I guess, making the between-the-lines of the contract explicit,
if lookup_rowtype_tupdesc is contracted to give me a tupdesc that sticks
around for as long as I haven't called ReleaseTupleDesc, and it sometimes
elects to give me one for which ReleaseTupleDesc is a no-op, the contract
is still that the thing sticks around for (at least) as long as I haven't
done that.

Cool. :)

Oh, hmm, maybe one thing in that API comment ought to be changed. It says
I must call ReleaseTupleDesc *or* DecrTupleDescRefCount. Maybe that dates
from before the shared registry? ReleaseTupleDesc is safe, but anybody who
uses DecrTupleDescRefCount on a lookup_rowtype_tupdesc result could be
in for an assertion failure if a non-refcounted tupdesc is returned.

Regards,
-Chap

Chapman Flack <chap@anastigmatix.net> writes:
> On 12/14/21 20:02, Tom Lane wrote:
>> The API contract for lookup_rowtype_tupdesc specifies that you must "call
>> ReleaseTupleDesc or DecrTupleDescRefCount when done using the tupdesc".
>> It's safe to assume that the tupdesc will stick around as long as you
>> haven't done that.

> I think what threw me was having a function whose API contract mentions
> reference counts, but that sometimes gives me things that don't have them.

That's supposed to be hidden under ReleaseTupleDesc; you shouldn't have to
think about it.

> Oh, hmm, maybe one thing in that API comment ought to be changed. It says
> I must call ReleaseTupleDesc *or* DecrTupleDescRefCount. Maybe that dates
> from before the shared registry? ReleaseTupleDesc is safe, but anybody who
> uses DecrTupleDescRefCount on a lookup_rowtype_tupdesc result could be
> in for an assertion failure if a non-refcounted tupdesc is returned.

Yeah, I was just wondering the same.  I think DecrTupleDescRefCount
is safe if you know you are looking up a named composite type, but
maybe that's still too much familiarity with typcache innards.

            regards, tom lane

I wrote:
> Chapman Flack <chap@anastigmatix.net> writes:
>> Oh, hmm, maybe one thing in that API comment ought to be changed. It says
>> I must call ReleaseTupleDesc *or* DecrTupleDescRefCount. Maybe that dates
>> from before the shared registry? ReleaseTupleDesc is safe, but anybody who
>> uses DecrTupleDescRefCount on a lookup_rowtype_tupdesc result could be
>> in for an assertion failure if a non-refcounted tupdesc is returned.

> Yeah, I was just wondering the same.  I think DecrTupleDescRefCount
> is safe if you know you are looking up a named composite type, but
> maybe that's still too much familiarity with typcache innards.

Here's a draft patch for this.  There are several places that are
directly using DecrTupleDescRefCount after lookup_rowtype_tupdesc
or equivalent, which'd now be forbidden.  I think they are all safe
given the assumption that the typcache's tupdescs for named composites
are refcounted.  (The calls in expandedrecord.c could be working
with RECORD, but those code paths just checked that the tupdesc
is refcounted.)  So there's no actual bug here, and no reason to
back-patch, but this seems like a good idea to decouple callers
a bit more from typcache's internal logic.  None of these call
sites are so performance-critical that one extra test will hurt.

            regards, tom lane

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 47b29001d5..bf42587e38 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15401,7 +15401,7 @@ ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode)
                      errmsg("table \"%s\" has different type for column \"%s\"",
                             RelationGetRelationName(rel), type_attname)));
     }
-    DecrTupleDescRefCount(typeTupleDesc);
+    ReleaseTupleDesc(typeTupleDesc);

     /* Any remaining columns at the end of the table had better be dropped. */
     for (; table_attno <= tableTupleDesc->natts; table_attno++)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 892b4e17e0..7d343f0678 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1465,7 +1465,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
                 /* find out the number of columns in the composite type */
                 tupDesc = lookup_rowtype_tupdesc(fstore->resulttype, -1);
                 ncolumns = tupDesc->natts;
-                DecrTupleDescRefCount(tupDesc);
+                ReleaseTupleDesc(tupDesc);

                 /* create workspace for column values */
                 values = (Datum *) palloc(sizeof(Datum) * ncolumns);
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 313d7b6ff0..2d857a301b 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -1484,7 +1484,7 @@ transformOfType(CreateStmtContext *cxt, TypeName *ofTypename)
         n->location = -1;
         cxt->columns = lappend(cxt->columns, n);
     }
-    DecrTupleDescRefCount(tupdesc);
+    ReleaseTupleDesc(tupdesc);

     ReleaseSysCache(tuple);
 }
diff --git a/src/backend/utils/adt/expandedrecord.c b/src/backend/utils/adt/expandedrecord.c
index e19491ecf7..38d5384c00 100644
--- a/src/backend/utils/adt/expandedrecord.c
+++ b/src/backend/utils/adt/expandedrecord.c
@@ -171,7 +171,7 @@ make_expanded_record_from_typeid(Oid type_id, int32 typmod,

         /* If we called lookup_rowtype_tupdesc, release the pin it took */
         if (type_id == RECORDOID)
-            DecrTupleDescRefCount(tupdesc);
+            ReleaseTupleDesc(tupdesc);
     }
     else
     {
@@ -854,7 +854,7 @@ expanded_record_fetch_tupdesc(ExpandedRecordHeader *erh)
         tupdesc->tdrefcount++;

         /* Release the pin lookup_rowtype_tupdesc acquired */
-        DecrTupleDescRefCount(tupdesc);
+        ReleaseTupleDesc(tupdesc);
     }
     else
     {
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index 70e5c51297..d140ef6655 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -1820,8 +1820,11 @@ lookup_rowtype_tupdesc_internal(Oid type_id, int32 typmod, bool noError)
  * for example from record_in().)
  *
  * Note: on success, we increment the refcount of the returned TupleDesc,
- * and log the reference in CurrentResourceOwner.  Caller should call
- * ReleaseTupleDesc or DecrTupleDescRefCount when done using the tupdesc.
+ * and log the reference in CurrentResourceOwner.  Caller must call
+ * ReleaseTupleDesc when done using the tupdesc.  (There are some
+ * cases in which the returned tupdesc is not refcounted, in which
+ * case PinTupleDesc/ReleaseTupleDesc are no-ops; but in these cases
+ * the tupdesc is guaranteed to live till process exit.)
  */
 TupleDesc
 lookup_rowtype_tupdesc(Oid type_id, int32 typmod)

On Thu, Dec 16, 2021 at 11:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Here's a draft patch for this.  There are several places that are
> directly using DecrTupleDescRefCount after lookup_rowtype_tupdesc
> or equivalent, which'd now be forbidden.  I think they are all safe
> given the assumption that the typcache's tupdescs for named composites
> are refcounted.  (The calls in expandedrecord.c could be working
> with RECORD, but those code paths just checked that the tupdesc
> is refcounted.)  So there's no actual bug here, and no reason to
> back-patch, but this seems like a good idea to decouple callers
> a bit more from typcache's internal logic.  None of these call
> sites are so performance-critical that one extra test will hurt.

LGTM.

On 12/15/21 17:50, Tom Lane wrote:

> Here's a draft patch for this.  There are several places that are
> directly using DecrTupleDescRefCount after lookup_rowtype_tupdesc
> or equivalent, which'd now be forbidden.  I think they are all safe
> given the assumption that the typcache's tupdescs for named composites
> are refcounted.  (The calls in expandedrecord.c could be working
> with RECORD, but those code paths just checked that the tupdesc
> is refcounted.)  So there's no actual bug here, and no reason to
> back-patch, but this seems like a good idea to decouple callers
> a bit more from typcache's internal logic.

I agree with the analysis at each of those sites, and the new comment
clears up everything that had puzzled me before.

Regards,
-Chap

Thomas Munro <thomas.munro@gmail.com> writes:
> On Thu, Dec 16, 2021 at 11:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Here's a draft patch for this.

> LGTM.

Pushed, thanks for looking.

            regards, tom lane