From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Andy Fan <zhihuifan1213@163.com>
Cc: Nikita Malakhov <hukutoc@gmail.com>
Cc: Michael Zhilin <m.zhilin@postgrespro.ru>
Cc: Peter Smith <smithpb2250@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Shared detoast Datum proposal
Date: Mon, 4 Mar 2024 13:07:58 +0100
Message-ID: <6718759c-2dac-48e4-bf18-282de4d82204@enterprisedb.com> (raw)
In-Reply-To: <87v862dbht.fsf@163.com>
References: <87il4jrk1l.fsf@163.com>
<87wmskm9qu.fsf@163.com>
<CAHut+PsehMtwQ4uE9VS2C7jB1yT0SZVOPU22u1=ma1K7dKFUow@mail.gmail.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>
On 3/4/24 02:29, Andy Fan wrote:
>
> Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
>
>> On 3/3/24 07:10, Andy Fan wrote:
>>>
>>> Hi,
>>>
>>> Here is a updated version, the main changes are:
>>>
>>> 1. an shared_detoast_datum.org file which shows the latest desgin and
>>> pending items during discussion.
>>> 2. I removed the slot->pre_detoast_attrs totally.
>>> 3. handle some pg_detoast_datum_slice use case.
>>> 4. Some implementation improvement.
>>>
>>
>> I only very briefly skimmed the patch, and I guess most of my earlier
>> comments still apply.
>
> Yes, the overall design is not changed.
>
>> 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? If with master, I
suggest to ignore that / treat that as a separate issue and leave it for
a different patch.
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.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@enterprisedb.com, zhihuifan1213@163.com, hukutoc@gmail.com, m.zhilin@postgrespro.ru, smithpb2250@gmail.com, vignesh21@gmail.com, boekewurm+postgres@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Shared detoast Datum proposal
In-Reply-To: <6718759c-2dac-48e4-bf18-282de4d82204@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox