Hi >From: Thomas Munro [mailto:thomas.mu...@enterprisedb.com] >Sent: Wednesday, November 14, 2018 9:50 AM >To: Ideriha, Takeshi/出利葉 健 <ideriha.take...@jp.fujitsu.com> > >On Tue, Nov 13, 2018 at 10:59 PM Ideriha, Takeshi ><ideriha.take...@jp.fujitsu.com> >wrote: >> Can I check my understanding? >> The situation you are talking about is the following: >> Data structure A and B will be allocated on DSA. Data A has a pointer to B. >> Another data X is already allocated on some area (might be on local >> heap) and has a pointer variable X->a, which will be linked to A. >> >> old_context = MemoryContextSwitchTo(dsa_memory_context); >> A = palloc(); >> B = palloc(); >> A->b = B; >> X->a = A; >> MemoryContextSwitchTo(old_context); >> >> If error happens in the way of this flow, palloc'd data (A & B) should >> be freed and pointer to freed data (X->a) should be back to its original one. >> So handling these cases introduces an "transaction" API like >> begin_allocate() and >end_allocate(). > >I wasn't thinking of resetting X->a to its original value, just freeing A and >B. For a >shared cache in this memory area, you need to make sure that you never leak >memory; it must either be referenced by the cache book keeping data structures >so >that you can find it and manually free it eventually (probably with an LRU >system?), or >it must somehow be cleaned up if an error occurs before you manage to insert >it into >the cache. During construction of an object graph, before you attach it to >your >manual book keeping data structures, there is a danger zone. > >For example, what if, while copying a query plan or a catcache entry, we >successfully >allocate a new cache entry with palloc(), but then another palloc() call fails >because >we run out of memory when copying the keys? It looks like the current >catcache.c >coding just doesn't >care: memory will be leaked permanently in CacheMemoryContext. That's OK >because it is process-local. Such a process is probably doomed anyway. If >you exit >and then start a new backend the problem is gone. >Here we need something better, because once this new kind of memory fills up >with >leaked objects, you have to restart the whole cluster to recover.
Thank you for the clarification. I agree that leaving memory leaking in shared memory without freeing it ends up cluster-wide problem. >If the solution involves significantly different control flows (like PG_TRY(), >PG_FINALLY() manual cleanup all over the place), then lots of existing >palloc-based >code will need to be reviewed and rewritten very carefully for use in this new >kind of >memory context. If it can be done automagically, then more of our existing >code will >be able to participate in the construction of stuff that we want to put in >shared >memory. About resetting X->a I was thinking about how to treat dangling pointer inside a new memory context machinery instead of using PG_TRY() stuff. That's because putting PG_TRY() all over the place becomes responsible for a developer trying to shared-memory-version MemoryContext and it needs a lot of efforts as you noted. But in my mind even if PG_TRY() is used, it only affects new code uses shared-memory version MemoryContext and doesn't affect current code. I may be missing something here... >I first thought about this in the context of shared plan caches, a topic that >appears on >this mailing list from time to time. There are some other fun problems, like >cache >invalidation for shared caches, space recycling (LRUs etc), and of course >locking/concurrency >(dsa_allocate() and dsa_free() are concurrency safe, but whatever data >structures >you build with the memory of course need their own plan for dealing with >concurrency). >But a strategy for clean-up on errors seems to be a major subproblem to deal >with >early in the project. I will be very happy if we can come up with something >:-) Yeah, I hope we can do it somehow. >On Wed, Nov 14, 2018 at 12:40 AM Ideriha, Takeshi ><ideriha.take...@jp.fujitsu.com> >wrote: >> From: Thomas Munro [mailto:thomas.mu...@enterprisedb.com] >> >postgres=# call hoge_list(3); >> >NOTICE: Contents of list: >> >NOTICE: 1 >> >NOTICE: 2 >> >NOTICE: 3 >> >CALL >> > >> >You can call that procedure from various different backends to add >> >and remove integers from a List. The point is that it could just as >> >easily be a query plan or any other palloc-based stuff in our tree, and the >> >pointers >work in any backend. >> >> Thanks for the patch. I know it's a rough sketch but basically that's what I >> want to >do. >> I tried it and it works well. > >After your message I couldn't resist a small experiment. But it seems there >are >plenty more problems to be solved to make it useful... Thank you for kind remark. By the way I just thought meta variable "hoge" is used only in Japan :) Regards, Takeshi Ideriha
