References: <87il4jrk1l.fsf@163.com> <87v87kvboq.fsf@163.com>
 <71321d83-391b-473b-aaf6-618c7f682b96@postgrespro.ru>
 <87r0i7vos3.fsf@163.com> <874je3hcwn.fsf@163.com>
 <c066e988-38b9-460f-9e9d-aedd2482bff2@enterprisedb.com>
 <8734tmg88e.fsf@163.com>
 <7f4c8673-4c5d-4dbf-b8e5-0ec11cd2fba0@enterprisedb.com>
 <87h6i2djf9.fsf@163.com> <87jzms0xza.fsf@163.com>
 <CAN-LCVMU_AzYNDQ-MgX3xc5qCnhTgO1LWKUCygOu3neBM-y94g@mail.gmail.com>
 <87frxf1gld.fsf@163.com>
 <fea81484-09b5-499d-80d0-acb8df43d047@enterprisedb.com>
 <CAN-LCVPv1f0DKk9UvpJ_OnY6CcpNvo7M-RkZVfLi7xRGdBNjqQ@mail.gmail.com>
 <871q8z19ux.fsf@163.com>
 <CAN-LCVON0vAr6tyiK4jsLmkCPd6PxkfxzXDyG8rjM9t3jOVM6g@mail.gmail.com>
 <87msrgdq93.fsf@163.com> <87frx7et1j.fsf@163.com>
 <d2e3ef5c-ac09-4b10-a99f-4c964b490f8c@enterprisedb.com>
 <87v862dbht.fsf@163.com>
 <6718759c-2dac-48e4-bf18-282de4d82204@enterprisedb.com>
User-agent: mu4e 1.10.7; emacs 29.1
From: Andy Fan <zhihuifan1213@163.com>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Nikita Malakhov <hukutoc@gmail.com>, Michael Zhilin
 <m.zhilin@postgrespro.ru>, Peter Smith <smithpb2250@gmail.com>, vignesh C
 <vignesh21@gmail.com>, Matthias van de Meent
 <boekewurm+postgres@gmail.com>, pgsql-hackers@lists.postgresql.org
Subject: Re: Shared detoast Datum proposal
Date: Mon, 04 Mar 2024 20:13:20 +0800
In-reply-to: <6718759c-2dac-48e4-bf18-282de4d82204@enterprisedb.com>
Message-ID: <87a5nechqz.fsf@163.com>
MIME-Version: 1.0
Content-Type: text/plain
Archived-At: <https://www.postgresql.org/message-id/87a5nechqz.fsf%40163.com>
Precedence: bulk


Tomas Vondra <tomas.vondra@enterprisedb.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