Thread: detoast datum into the given buffer as a optimization.

Hi,

Currently detoast_attr always detoast the data into a palloc-ed memory
and then if user wants the detoast data in a different memory, user has to
copy them, I'm thinking if we could provide a buf as optional argument for
detoast_attr to save such wastage. 

current format:

/* ----------
 * detoast_attr -
 *
 *    Public entry point to get back a toasted value from compression
 *    or external storage.  The result is always non-extended varlena form.
 *
 * Note some callers assume that if the input is an EXTERNAL or COMPRESSED
 * datum, the result will be a pfree'able chunk.
 * ----------
 */
struct varlena *
detoast_attr(struct varlena *attr)

new format:

/* ----------
 * detoast_attr -
 
 * ...
 *
 *   Note if caller provides a non-NULL buffer, it is the duty of caller
 * to make sure it has enough room for the detoasted format (Usually
 * they can use toast_raw_datum_size to get the size)  Or else a
 * palloced memory under CurrentMemoryContext is used.
 */

struct varlena *
detoast_attr(struct varlena *attr, char *buffer)

There are 2 user cases at least:

1. The shared detoast datum patch at [1], where I want to avoid the
duplicated detoast effort for the same datum, for example:

SELECT f(big_toast_col) FROM t WHERE g(big_toast_col);

Current master detoast it twice now.

In that patch, I want to detoast the datum into a MemoryContext where the
lifespan is same as slot->tts_values[*] rather than CurrentMemoryContext
so that the result can be reused in the different expression. Within the
proposal here, we can detoast the datum into the desired MemoryContext
directly (just allocating the buffer in the desired MemoryContext is OK).

2. make printtup function a bit faster [2]. That patch already removed
some palloc, memcpy effort, but it still have some chances to 
optimize further. for example in text_out function, it is still detoast
the datum into a palloc memory and then copy them into a StringInfo.

One of the key point is we can always get the varlena rawsize cheaply
without any real detoast activity in advance, thanks to the existing
varlena design.

If this can be accepted, it would reduce the size of patch [2] at some
extend, and which part was disliked by Thomas (and me..) [3].

What do you think? 

[1] https://commitfest.postgresql.org/49/4759/
[2] https://www.postgresql.org/message-id/87wmjzfz0h.fsf%40163.com
[3] https://www.postgresql.org/message-id/6718759c-2dac-48e4-bf18-282de4d82204%40enterprisedb.com

-- 
Best Regards
Andy Fan

On Wed, Sep 18, 2024 at 05:35:56PM +0800, Andy Fan wrote:
> Currently detoast_attr always detoast the data into a palloc-ed memory
> and then if user wants the detoast data in a different memory, user has to
> copy them, I'm thinking if we could provide a buf as optional argument for
> detoast_attr to save such wastage. 
>
> [...]
> 
> What do you think? 

My first thought is that this seems reasonable if there are existing places
where we are copying the data out of the palloc'd memory, but otherwise it
might be more of a prerequisite patch for the other things you mentioned.

-- 
nathan

On Wed, Sep 18, 2024 at 2:23 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Wed, Sep 18, 2024 at 05:35:56PM +0800, Andy Fan wrote:
> > Currently detoast_attr always detoast the data into a palloc-ed memory
> > and then if user wants the detoast data in a different memory, user has to
> > copy them, I'm thinking if we could provide a buf as optional argument for
> > detoast_attr to save such wastage.
> >
> > [...]
> >
> > What do you think?
>
> My first thought is that this seems reasonable if there are existing places
> where we are copying the data out of the palloc'd memory, but otherwise it
> might be more of a prerequisite patch for the other things you mentioned.
>

This would also simplify data copying patterns many extensions have to do.
For instance, often they have to move the data from Postgres memory into
another language's allocation types. Or just for custom data structures,
of course.

I would suggest that this be added as new API, however, instead of a change
to `detoast_attr`. This would make different return types more sensical,
as there is no need to implicitly allocate. It could return an error type?

> *   Note if caller provides a non-NULL buffer, it is the duty of caller
> * to make sure it has enough room for the detoasted format (Usually
> * they can use toast_raw_datum_size to get the size)

I'm not entirely sure why the caller is being given the burden of checking,
but I suppose they probably did check? I can imagine scenarios where they
are not interested, however, and the callee always has to obtain the data
for len written anyways. So I would probably make writable length a third arg.

Andy Fan <zhihuifan1213@163.com> writes:
>  *   Note if caller provides a non-NULL buffer, it is the duty of caller
>  * to make sure it has enough room for the detoasted format (Usually
>  * they can use toast_raw_datum_size to get the size)

This is a pretty awful, unsafe API design.  It puts it on the caller
to know how to get the detoasted length, and it implies double
decoding of the toast datum.

> One of the key point is we can always get the varlena rawsize cheaply
> without any real detoast activity in advance, thanks to the existing
> varlena design.

This is not an assumption I care to wire into the API design.

How about a variant like

struct varlena *
detoast_attr_cxt(struct varlena *attr, MemoryContext cxt)

which promises to allocate the result in the specified context?
That would cover most of the practical use-cases, I think.

            regards, tom lane

Thank you all for the double check.

> Andy Fan <zhihuifan1213@163.com> writes:
>>  *   Note if caller provides a non-NULL buffer, it is the duty of caller
>>  * to make sure it has enough room for the detoasted format (Usually
>>  * they can use toast_raw_datum_size to get the size)
>
> ...,  It puts it on the caller to know how to get the detoasted length

Yes.

> and it implies double decoding of the toast datum.

Yes, We need to decoding the toast datum to know the rawsize as what we
did in toast_raw_datum_size, this is an extra effrot.

But I want to highlight that this "decoding" is different from
"detoast", the later one need to scan toast_relation or decompression
the data so it is a heavy work, but the former one just decoding some
existing memory at hand which should be very cheap.

>> One of the key point is we can always get the varlena rawsize cheaply
>> without any real detoast activity in advance, thanks to the existing
>> varlena design.
>
> This is not an assumption I care to wire into the API design.

OK. (I just was excited to find out we can get the rawsize so cheaply,
so we can find out an API to satify the both user cases.) 

> How about a variant like
>
> struct varlena *
> detoast_attr_cxt(struct varlena *attr, MemoryContext cxt)
>
> which promises to allocate the result in the specified context?
> That would cover most of the practical use-cases, I think.

I think this works for my user case 1 but doesn't work for my user case 2
which requires the detoasted data is writen into a given memory
buffer (not only a certain MemoryContext). IIUC the user cases Jubilee
provided is more like user case 2. 

""" (user case 2)
2. make printtup function a bit faster [2]. The patch there already
removed some palloc, memcpy effort, but it still have some chances to
optimize further.  for example text_out function, it is still detoast
the datum into a palloc memory and then copy them into a StringInfo.
"""

I really want to make some progress in this direction, so thank you for
the feedback. 

-- 
Best Regards
Andy Fan

Jubilee Young <workingjubilee@gmail.com> writes:


> On Wed, Sep 18, 2024 at 2:23 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>>
>> On Wed, Sep 18, 2024 at 05:35:56PM +0800, Andy Fan wrote:
>> > Currently detoast_attr always detoast the data into a palloc-ed memory
>> > and then if user wants the detoast data in a different memory, user has to
>> > copy them, I'm thinking if we could provide a buf as optional argument for
>> > detoast_attr to save such wastage.
>> >
>> > [...]
>> >
>> > What do you think?
>>
>> My first thought is that this seems reasonable if there are existing places
>> where we are copying the data out of the palloc'd memory, but otherwise it
>> might be more of a prerequisite patch for the other things you mentioned.
>>
>
> This would also simplify data copying patterns many extensions have to do.
> For instance, often they have to move the data from Postgres memory into
> another language's allocation types. Or just for custom data structures,
> of course.

I thought we have, but did't check it so far. If we figured out an API,
we can use them for optimizing some existing code.

> I would suggest that this be added as new API, however, instead of a change
> to `detoast_attr`.

I agree that new API would usually be clearer than adding a more
argument, just that I don't want copy-paste too much existing
code. Actually the thing in my mind is:

struct varlena *
detoast_attr(struct varlena *attr)
{
   int rawsize = toast_raw_datum_size(attr);
   char *buffer = palloc(rawsize)
   return detoast_attr_buffer(attr, buffer);
}

struct varlena *
detoast_attr_buffer(struct varlena *attr, char *buffer)
{
  ...
}

In this case:
- there is no existing code need to be changed.
- detoast_attr_buffer is tested sufficiently automatically.

> This would make different return types more sensical,
> as there is no need to implicitly allocate. It could return an error
> type?

I can't understand the error here.

>
>> *   Note if caller provides a non-NULL buffer, it is the duty of caller
>> * to make sure it has enough room for the detoasted format (Usually
>> * they can use toast_raw_datum_size to get the size)
>
> I'm not entirely sure why the caller is being given the burden of checking,
> but I suppose they probably did check? I can imagine scenarios where they
> are not interested, however, and the callee always has to obtain the data
> for len written anyways. So I would probably make writable length a
> third arg.

I didn't follow up here as well:(,  do you mind to explain a bit?

--
Best Regards
Andy Fan