RE: Copy data to DSA area

Hi

>From: Thomas Munro [mailto:thomas.munro@enterprisedb.com]
>Sent: Wednesday, November 14, 2018 9:50 AM
>To: Ideriha, Takeshi/出利葉 健 <ideriha.takeshi@jp.fujitsu.com>
>
>On Tue, Nov 13, 2018 at 10:59 PM Ideriha, Takeshi <ideriha.takeshi@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.takeshi@jp.fujitsu.com>
>wrote:
>> From: Thomas Munro [mailto:thomas.munro@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