Re: Shared detoast Datum proposal

Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> writes:

>>> But I'm a bit surprised the patch needs to pass a
>>> MemoryContext to so many places as a function argument - that seems to
>>> go against how we work with memory contexts. Doubly so because it seems
>>> to only ever pass CurrentMemoryContext anyway. So what's that about?
>>
>> I think you are talking about the argument like this:
>>
>> /* ----------
>> - * detoast_attr -
>> + * detoast_attr_ext -
>> *
>> * Public entry point to get back a toasted value from compression
>> * or external storage. The result is always non-extended varlena form.
>> *
>> + * ctx: The memory context which the final value belongs to.
>> + *
>> * Note some callers assume that if the input is an EXTERNAL or COMPRESSED
>> * datum, the result will be a pfree'able chunk.
>> * ----------
>> */
>>
>> +extern struct varlena *
>> +detoast_attr_ext(struct varlena *attr, MemoryContext ctx)
>>
>> This is mainly because 'detoast_attr' will apply more memory than the
>> "final detoast datum" , for example the code to scan toast relation
>> needs some memory as well, and what I want is just keeping the memory
>> for the final detoast datum so that other memory can be released sooner,
>> so I added the function argument for that.
>>
>
> What exactly does detoast_attr allocate in order to scan toast relation?
> Does that happen in master, or just with the patch?

It is in the current master, for example the TupleTableSlot creation
needed by scanning the toast relation needs a memory allocating.

> If with master, I
> suggest to ignore that / treat that as a separate issue and leave it for
> a different patch.

OK, I can make it as a seperate commit in the next version.

> In any case, the custom is to allocate results in the context that is
> set in CurrentMemoryContext at the moment of the call, and if there's
> substantial amount of allocations that we want to free soon, we either
> do that by explicit pfree() calls, or create a small temporary context
> in the function (but that's more expensive).
>
> I don't think we should invent a new convention where the context is
> passed as an argument, unless absolutely necessary.

Hmm, in this case, if we don't add the new argument, we have to get the
detoast datum in Context-1 and copy it to the desired memory context,
which is the thing I want to avoid. I think we have a same decision to
make on the TOAST cache method as well.

--
Best Regards
Andy Fan