Re: [PATCHES] TupleDesc refcounting
I wrote: 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. After much thrashing I concluded that my original instinct was right and we really do want to insist on cleanup of tupdesc references during normal processing. The approach I tried to take effectively meant leaking tupdesc references until end of query, which is really bad news for SQL-language functions --- a function that grabs a tupdesc reference during plan startup would then accumulate a tupdesc reference during every invocation, leading to indefinite bloat in the ResourceOwner over a long query. So I ended up applying something pretty close to Neil's patch. I did modify it to not bother reference-counting tupdescs that aren't actually in any cache. I also fixed up TupleTableSlots to do reference-count processing if ExecSetSlotDescriptor is handed a reference-counted tupdesc (my assertion yesterday that this never happens was wrong), but not to bother cleaning up non-ref-counted descriptors. All in all, a lot of thrashing for only marginal improvement :-( regards, tom lane ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] TupleDesc refcounting
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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] TupleDesc refcounting
On Sun, 2006-01-22 at 13:43 -0500, Tom Lane wrote: I object ... this is still trying to enforce tupdesc refcounting on much more of the system than I think useful or prudent. Well, the patch adds management of TupleDesc refcounts solely for the return value of lookup_rowtype_tupdesc(). If we're going to use reference counting there at all, I don't see a way do make the refcounting any less invasive. -Neil ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] TupleDesc refcounting
On Thu, 2006-01-19 at 04:14 -0500, Neil Conway wrote: The patch is WIP: a few regression tests fail due to TupleDesc leaks I haven't fixed yet but that should be easily fixable, and there are a few other minor issues to address. I'm just posting the patch now to get any feedback. Attached is a revised patch: all the regression tests patch, and I'm not aware of any major remaining issues. Barring any objections, I'll apply this tomorrow. (I'll take a look at using refcounting for TupleDescs in other parts of the system after that...) -Neil *** src/backend/access/common/tupdesc.c 83ca807d4fdd572c409bc9214922b6ba9da7ce18 --- src/backend/access/common/tupdesc.c 1a84f6faf6fc647c2bad0c2381f976f65b703f59 *** *** 23,28 --- 23,29 #include catalog/pg_type.h #include parser/parse_type.h #include utils/builtins.h + #include utils/resowner.h #include utils/syscache.h *** *** 84,89 --- 85,91 desc-tdtypeid = RECORDOID; desc-tdtypmod = -1; desc-tdhasoid = hasoid; + desc-refcount = 0; return desc; } *** *** 116,121 --- 118,124 desc-tdtypeid = RECORDOID; desc-tdtypmod = -1; desc-tdhasoid = hasoid; + desc-refcount = 0; return desc; } *** *** 214,219 --- 217,224 { int i; + Assert(tupdesc-refcount == 0); + if (tupdesc-constr) { if (tupdesc-constr-num_defval 0) *** *** 246,251 --- 251,277 pfree(tupdesc); } + void + IncrTupleDescRefCount(TupleDesc tupdesc) + { + Assert(tupdesc-refcount = 0); + + ResourceOwnerEnlargeTupleDescs(CurrentResourceOwner); + tupdesc-refcount++; + ResourceOwnerRememberTupleDesc(CurrentResourceOwner, tupdesc); + } + + void + DecrTupleDescRefCount(TupleDesc tupdesc) + { + Assert(tupdesc-refcount 0); + + tupdesc-refcount--; + ResourceOwnerForgetTupleDesc(CurrentResourceOwner, tupdesc); + if (tupdesc-refcount == 0) + FreeTupleDesc(tupdesc); + } + /* * Compare two TupleDesc structures for logical equality * *** src/backend/access/heap/tuptoaster.c f3ec822c0e6b6328bc0026716fcf4b133f89b092 --- src/backend/access/heap/tuptoaster.c bbdea6bdd98d250326fad68899d07a8b153fb263 *** *** 892,898 --- 892,901 * If nothing to untoast, just return the original tuple. */ if (!need_change) + { + DecrTupleDescRefCount(tupleDesc); return value; + } /* * Calculate the new size of the tuple. Header size should not change, *** *** 930,935 --- 933,939 if (toast_free[i]) pfree(DatumGetPointer(toast_values[i])); + DecrTupleDescRefCount(tupleDesc); return PointerGetDatum(new_data); } *** src/backend/executor/execQual.c eb1daef85341439578a3f6bb73549c4420292bc3 --- src/backend/executor/execQual.c 00e68d0f9dc2b692b994608acc001f640679f7a8 *** *** 94,99 --- 94,100 static Datum ExecEvalConvertRowtype(ConvertRowtypeExprState *cstate, ExprContext *econtext, bool *isNull, ExprDoneCond *isDone); + static void CleanupConvertRowtypeCallback(Datum arg); static Datum ExecEvalCase(CaseExprState *caseExpr, ExprContext *econtext, bool *isNull, ExprDoneCond *isDone); static Datum ExecEvalCaseTestExpr(ExprState *exprstate, *** *** 132,140 --- 133,143 static Datum ExecEvalFieldSelect(FieldSelectState *fstate, ExprContext *econtext, bool *isNull, ExprDoneCond *isDone); + static void CleanupFieldSelectCallback(Datum arg); static Datum ExecEvalFieldStore(FieldStoreState *fstate, ExprContext *econtext, bool *isNull, ExprDoneCond *isDone); + static void CleanupFieldStoreCallback(Datum arg); static Datum ExecEvalRelabelType(GenericExprState *exprstate, ExprContext *econtext, bool *isNull, ExprDoneCond *isDone); *** *** 707,712 --- 710,717 attrno, tupDesc, isNull); + + DecrTupleDescRefCount(tupDesc); return result; } *** *** 765,770 --- 770,777 attrno, tupDesc, isNull); + + DecrTupleDescRefCount(tupDesc); return result; } *** *** 1149,1157 /* * ExecMakeTableFunctionResult * ! * Evaluate a table function, producing a materialized result in a Tuplestore ! * object. *returnDesc is set to the tupledesc actually returned by the ! * function, or NULL if it didn't provide one. */ Tuplestorestate * ExecMakeTableFunctionResult(ExprState *funcexpr, --- 1156,1166 /* * ExecMakeTableFunctionResult * ! * Evaluate a table function, producing a materialized result in a ! * Tuplestore object. *returnDesc is set to the tupledesc actually ! * returned by the function, or NULL if it didn't
Re: [PATCHES] TupleDesc refcounting
On Tue, 2006-01-17 at 09:36 -0500, Tom Lane wrote: I suspect you'll find that it's convenient to treat typcache's own link to a tupdesc as a reference count too, so it's not strictly external references that should be counted. The problem with this is that incrementing a TupleDesc's refcount informs the CurrentResourceOwner. That causes the refcount to be decremented when the resource owner is released, which is wrong when the TupleDesc should live beyond the current query (as it should in the typcache). We could avoid that by creating a CacheResourceOwner that is never reset, as discussed earlier, but at present I'm not sure if there's any point, so I've just used a dead field. Attached is a revised patch. Reference counting is only used for lookup_rowtype_tupdesc(): as discussed there might be other places that would benefit from this, but I haven't looked at that yet. There was some call-sites of lookup_rowtype_tupdesc() where it doesn't seem to be easy to use reference counting. For example, consider the implementation of get_expr_result_type(): some code paths within that function call lookup_rowtype_tupdesc() to produce the returned TupleDesc, and some do not. The easiest fix seemed to be just making a copy of the TupleDesc for the lookup_rowtype_tupdesc() cases. The patch is WIP: a few regression tests fail due to TupleDesc leaks I haven't fixed yet but that should be easily fixable, and there are a few other minor issues to address. I'm just posting the patch now to get any feedback. (Apologies for not getting this done earlier, I had a touch of the flu yesterday...) -Neil *** src/backend/access/common/tupdesc.c 83ca807d4fdd572c409bc9214922b6ba9da7ce18 --- src/backend/access/common/tupdesc.c c423df65341ed283d20ff03c4437ad67876ef0ba *** *** 23,28 --- 23,29 #include catalog/pg_type.h #include parser/parse_type.h #include utils/builtins.h + #include utils/resowner.h #include utils/syscache.h *** *** 84,89 --- 85,92 desc-tdtypeid = RECORDOID; desc-tdtypmod = -1; desc-tdhasoid = hasoid; + desc-refcount = 0; + desc-dead = false; return desc; } *** *** 116,121 --- 119,126 desc-tdtypeid = RECORDOID; desc-tdtypmod = -1; desc-tdhasoid = hasoid; + desc-refcount = 0; + desc-dead = false; return desc; } *** *** 214,219 --- 219,226 { int i; + Assert(tupdesc-refcount == 0); + if (tupdesc-constr) { if (tupdesc-constr-num_defval 0) *** *** 246,251 --- 253,279 pfree(tupdesc); } + void + IncrTupleDescRefCount(TupleDesc tupdesc) + { + Assert(tupdesc-refcount = 0); + + ResourceOwnerEnlargeTupleDescs(CurrentResourceOwner); + tupdesc-refcount++; + ResourceOwnerRememberTupleDesc(CurrentResourceOwner, tupdesc); + } + + void + DecrTupleDescRefCount(TupleDesc tupdesc) + { + Assert(tupdesc-refcount 0); + + tupdesc-refcount--; + ResourceOwnerForgetTupleDesc(CurrentResourceOwner, tupdesc); + if (tupdesc-refcount == 0 tupdesc-dead) + FreeTupleDesc(tupdesc); + } + /* * Compare two TupleDesc structures for logical equality * *** src/backend/access/heap/tuptoaster.c f3ec822c0e6b6328bc0026716fcf4b133f89b092 --- src/backend/access/heap/tuptoaster.c bbdea6bdd98d250326fad68899d07a8b153fb263 *** *** 892,898 --- 892,901 * If nothing to untoast, just return the original tuple. */ if (!need_change) + { + DecrTupleDescRefCount(tupleDesc); return value; + } /* * Calculate the new size of the tuple. Header size should not change, *** *** 930,935 --- 933,939 if (toast_free[i]) pfree(DatumGetPointer(toast_values[i])); + DecrTupleDescRefCount(tupleDesc); return PointerGetDatum(new_data); } *** src/backend/executor/execQual.c eb1daef85341439578a3f6bb73549c4420292bc3 --- src/backend/executor/execQual.c 73a3d813603ce497b0a2c45a6f21fc001445d1b8 *** *** 707,712 --- 707,714 attrno, tupDesc, isNull); + + DecrTupleDescRefCount(tupDesc); return result; } *** *** 765,770 --- 767,774 attrno, tupDesc, isNull); + + DecrTupleDescRefCount(tupDesc); return result; } *** *** 1149,1157 /* * ExecMakeTableFunctionResult * ! * Evaluate a table function, producing a materialized result in a Tuplestore ! * object. *returnDesc is set to the tupledesc actually returned by the ! * function, or NULL if it didn't provide one. */ Tuplestorestate * ExecMakeTableFunctionResult(ExprState *funcexpr, --- 1153,1163 /* * ExecMakeTableFunctionResult * ! * Evaluate a table function, producing a materialized result
Re: [PATCHES] TupleDesc refcounting
Neil Conway [EMAIL PROTECTED] writes: On Tue, 2006-01-17 at 09:36 -0500, Tom Lane wrote: I suspect you'll find that it's convenient to treat typcache's own link to a tupdesc as a reference count too, so it's not strictly external references that should be counted. The problem with this is that incrementing a TupleDesc's refcount informs the CurrentResourceOwner. Not if it's just tupdesc-refcount++; ... ;-) Seriously, I was imagining a two-level structure where tupdesc itself just has operations on the order of refcount++; if (--refcount = 0) FreeTupleDesc(); and then a separate layer on top of that adds ResourceOwner management. This is precisely because the cache code requires non-resource-owner- managed links (or at least, has no particular use for the services of a ResourceOwner). There was some call-sites of lookup_rowtype_tupdesc() where it doesn't seem to be easy to use reference counting. For example, consider the implementation of get_expr_result_type(): some code paths within that function call lookup_rowtype_tupdesc() to produce the returned TupleDesc, and some do not. The easiest fix seemed to be just making a copy of the TupleDesc for the lookup_rowtype_tupdesc() cases. Yeah, I noticed this while making the back-branch patch. I agree that any given routine had better have a consistent return convention: if the tupdesc needs to be refcount-decremented when done, that must be the case in all code paths, since the caller can't be expected to know which case applies. My recollection though is that the non-lookup cases return freshly-built-in-local-storage tupdescs, which could certainly be set up as having refcount 1 so that the decrement would work correctly. It may or may not be worth the trouble compared to just copying, though. You should think about whether a routine is a performance hotspot before going out of your way to avoid a copy step. (Apologies for not getting this done earlier, I had a touch of the flu yesterday...) There's no hurry about it. Hope you're feeling better. regards, tom lane ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] TupleDesc refcounting
On Sun, 2006-01-15 at 12:08 -0500, Tom Lane wrote: My inclination at this point is to forget the whole thing and just patch the callers of lookup_rowtype_tupdesc that need to copy the tupdesc. Actually, I think I finally understand how to implement this patch sanely. I had thought that the lifetime of a TupleDesc should be dictated by either the memory context in which it is allocated, OR its reference count. This leads us down the road toward mandatory reference counting, which I agree is a net loss. However, since we're primarily concerned with TupleDescs allocated in CacheMemoryContext and that context is never reset, we can use the reference count *just* to manage the external references to TupleDescs. That should make the patch far less invasive. (I have the feeling you've been suggesting this all along, I've just been too thick-skulled to understand you.) I'll hopefully have a patch implementing this finished by tomorrow evening. -Neil ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] TupleDesc refcounting
Neil Conway [EMAIL PROTECTED] writes: On Thu, 2006-01-12 at 10:40 -0500, Tom Lane wrote: If you're finding yourself writing a large and invasive patch, I think you're doing it wrong. I think I might be :-) Yipes ... this seems far more invasive than I think is justified. In particular the notion of storing *every* tupdesc in TopMemoryContext seems completely wrong. That's not working with the context system, that's working against it. What I had in mind was an optional refcounting facility, whereas you seem to be going in the direction of making it mandatory. The reason I think it should be optional is that most uses of tupdescs just don't need it. We have a grand total of one place where a refcount (or forced local copy) seems demonstrably necessary. Surely changing a whole lot of code to accommodate that one place is not the right tradeoff. My inclination at this point is to forget the whole thing and just patch the callers of lookup_rowtype_tupdesc that need to copy the tupdesc. regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] TupleDesc refcounting
On Tue, 2006-01-10 at 18:05 -0500, Tom Lane wrote: Yeah. I was envisioning two different approaches: for TupleDesc references from long-lived data structures (ie, typcache or relcache) just increment or decrement the count when the referencing data structure changes. ResourceOwner would be used for dynamic within-query references --- in practice, local variables. If the reference would be forgotten during an elog longjmp then you need a ResourceOwner to backstop it, otherwise not. Ah, I see what you mean. In implementing this, I wasn't sure the best way to provide these two sorts of TupleDesc references. My first thought was to add a use ResourceOwner? boolean parameter to the routines that create and destroy references to TupleDescs: extern TupleDesc CreateTemplateTupleDesc(int natts, bool hasoid, bool use_resowner); extern TupleDesc CreateTupleDesc(int natts, bool hasoid, Form_pg_attribute *attrs, bool use_resowner); extern TupleDesc CreateTupleDescCopy(TupleDesc tupdesc, bool use_resowner); extern TupleDesc CreateTupleDescCopyConstr(TupleDesc tupdesc, bool use_resowner); extern void IncrTupleDescRefCount(TupleDesc tupdesc, bool use_resowner); extern void DecrTupleDescRefCount(TupleDesc tupdesc, bool use_resowner); But that requires making *all* the call-sites of these routines care about whether a ResourceOwner is being used. I think most call-sites will want to use a ResourceOwner, so that ought to be the default. Avoiding changing the function signatures also means we don't break a fairly widely-used API. Instead, how about defining: extern void IncrTupleDescPersistentRefCount(TupleDesc tdesc); (the name could do with improvement). This would increment the TupleDesc's persistent reference count -- that is, non-persistent references are automatically released at transaction end, whereas persistent ones are only released via: extern void DecrTupleDescPersistentRefCount(TupleDesc tdesc); One downside is that creating a new, persistent TupleDesc requires a little pain: TupleDesc tdesc = make_your_tdesc(); IncrTupleDescPersistentRefCount(tdesc); DecrTupleDescRefCount(tdesc); That means we will unnecessarily inform the ResourceOwner about the TupleDesc, and then tell it to immediately forget about it. However, I think that's acceptable: ResourceOwnerRememberTupleDesc() is trivial, and ResourceOwnerForgetTupleDesc() searches beginning at the most-recently-added TupleDesc, so it should also be very cheap. -Neil ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] TupleDesc refcounting
Neil Conway [EMAIL PROTECTED] writes: Ah, I see what you mean. In implementing this, I wasn't sure the best way to provide these two sorts of TupleDesc references. My first thought was to add a use ResourceOwner? boolean parameter to the routines that create and destroy references to TupleDescs: No, I wouldn't do that. I would keep the routines you mention ignorant of ResourceOwner, because I think that the vast majority of tupdesc usage will NOT be using ResourceOwners. Only the places where a pointer to a cached tupdesc is handed out need to deal with this. This excludes practically all of the executor, for instance. If you're finding yourself writing a large and invasive patch, I think you're doing it wrong. I'm envisioning something pretty localized. regards, tom lane ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
[PATCHES] TupleDesc refcounting
Attached is a WIP patch that adds reference counting for TupleDescs. Two issues that I ran into while implementing it: (1) How should the lifetime of a TupleDesc be managed? The existing ResourceOwner stuff seems to assume that it is managing per-query resources. A TupleDesc will often live beyond the lifetime of a single transaction, and might even be created before transactions can be started (e.g. formrdesc() in relcache.c, when we're creating relcache entries for nailed-in-cache relations early in InitPostgres()). In current sources, the lifetime of a TupleDesc is the lifetime of the memory context in which it is allocated (and/or whenever FreeTupleDesc() is invoked). We could imitate that behavior by optionally linking a ResourceOwner with each MemoryContext, and releasing the ResourceOwner when the MemoryContext is reset or deleted. However, I'm not sure that that's the right approach... (2) The existing ResourceOwner users issue a warning if the resource they are managing is not explicitly released before a transaction successfully commits (so they elog(WARNING)). I don't see the need to be that strict for TupleDescs -- as we do with palloc() without a matching pfree(), I think it should be okay to just silently clean up leaked TupleDescs when releasing a ResourceOwner. Thoughts? -Neil *** src/backend/access/common/tupdesc.c 83ca807d4fdd572c409bc9214922b6ba9da7ce18 --- src/backend/access/common/tupdesc.c 63efd0b6adc911b57cef00cf0c4611760a91fa2c *** *** 23,28 --- 23,29 #include catalog/pg_type.h #include parser/parse_type.h #include utils/builtins.h + #include utils/resowner.h #include utils/syscache.h *** *** 30,37 * CreateTemplateTupleDesc * This function allocates an empty tuple descriptor structure. * ! * Tuple type ID information is initially set for an anonymous record type; ! * caller can overwrite this if needed. */ TupleDesc CreateTemplateTupleDesc(int natts, bool hasoid) --- 31,42 * CreateTemplateTupleDesc * This function allocates an empty tuple descriptor structure. * ! * Tuple type ID information is initially set for an anonymous record ! * type; caller can overwrite this if needed. ! * ! * The reference count on the TupleDesc is initially 1: the caller ! * should decrement the reference count via DecrTupleDescRefCount() ! * when finished. */ TupleDesc CreateTemplateTupleDesc(int natts, bool hasoid) *** *** 85,90 --- 90,99 desc-tdtypmod = -1; desc-tdhasoid = hasoid; + /* Set to zero, then inform the resowner and increment refcount */ + desc-refcount = 0; + IncrTupleDescRefCount(desc); + return desc; } *** *** 93,103 * This function allocates a new TupleDesc pointing to a given * Form_pg_attribute array. * ! * Note: if the TupleDesc is ever freed, the Form_pg_attribute array * will not be freed thereby. * * Tuple type ID information is initially set for an anonymous record type; * caller can overwrite this if needed. */ TupleDesc CreateTupleDesc(int natts, bool hasoid, Form_pg_attribute *attrs) --- 102,116 * This function allocates a new TupleDesc pointing to a given * Form_pg_attribute array. * ! * Note: when the TupleDesc is destroyed, the Form_pg_attribute array * will not be freed thereby. * * Tuple type ID information is initially set for an anonymous record type; * caller can overwrite this if needed. + * + * The reference count on the TupleDesc is initially 1: the caller + * should decrement the reference count via DecrTupleDescRefCount() + * when finished. */ TupleDesc CreateTupleDesc(int natts, bool hasoid, Form_pg_attribute *attrs) *** *** 117,122 --- 130,139 desc-tdtypmod = -1; desc-tdhasoid = hasoid; + /* Set to zero, then inform the resowner and increment refcount */ + desc-refcount = 0; + IncrTupleDescRefCount(desc); + return desc; } *** *** 125,130 --- 142,151 * This function creates a new TupleDesc by copying from an existing * TupleDesc. * + * The reference count on the TupleDesc is initially 1: the caller + * should decrement the reference count via DecrTupleDescRefCount() + * when finished. + * * !!! Constraints and defaults are not copied !!! */ TupleDesc *** *** 144,150 desc-tdtypeid = tupdesc-tdtypeid; desc-tdtypmod = tupdesc-tdtypmod; ! return desc; } --- 165,171 desc-tdtypeid = tupdesc-tdtypeid; desc-tdtypmod = tupdesc-tdtypmod; ! return desc; } *** *** 152,157 --- 173,182 * CreateTupleDescCopyConstr * This function creates a new TupleDesc by copying from an existing * TupleDesc (including its constraints and defaults). + * + * The reference count on the TupleDesc is initially 1: the caller +
Re: [PATCHES] TupleDesc refcounting
Neil Conway [EMAIL PROTECTED] writes: (1) How should the lifetime of a TupleDesc be managed? The existing ResourceOwner stuff seems to assume that it is managing per-query resources. Yeah. I was envisioning two different approaches: for TupleDesc references from long-lived data structures (ie, typcache or relcache) just increment or decrement the count when the referencing data structure changes. ResourceOwner would be used for dynamic within-query references --- in practice, local variables. If the reference would be forgotten during an elog longjmp then you need a ResourceOwner to backstop it, otherwise not. You could conceivably set up a cache ResourceOwner with indefinite lifespan to make the cache case more like the local-variable case, but I think this would merely be a performance drag with no real value. There's no point in a ResourceOwner if it will never be called on to release resources, and a cache-lifespan ResourceOwner wouldn't be. In current sources, the lifetime of a TupleDesc is the lifetime of the memory context in which it is allocated (and/or whenever FreeTupleDesc() is invoked). We could imitate that behavior by optionally linking a ResourceOwner with each MemoryContext, and releasing the ResourceOwner when the MemoryContext is reset or deleted. However, I'm not sure that that's the right approach... No. We really only need this mechanism for the case where a tupledesc allocated in a long-lived context (again, think relcache or typcache) is going to be dynamically referenced by shorter-lived code. Tying it to MemoryContexts won't help because CacheMemoryContext never gets reset. (2) The existing ResourceOwner users issue a warning if the resource they are managing is not explicitly released before a transaction successfully commits (so they elog(WARNING)). I don't see the need to be that strict for TupleDescs -- as we do with palloc() without a matching pfree(), I think it should be okay to just silently clean up leaked TupleDescs when releasing a ResourceOwner. The reason why those warnings are issued is to catch code that is failing to manage references properly. I think that motivation applies perfectly well to tuple descriptors too. There is no good reason for code to forget to release the descriptor reference in non-error paths. No time to look at the patch itself right now ... regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend