References: <87il4jrk1l.fsf@163.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>
 <875xyb1a6q.fsf@163.com>
 <a595a888-1dde-42a0-becd-c9c313ac781a@enterprisedb.com>
 <87zfvedbsr.fsf@163.com>
 <a70cfbef-45ae-4056-9c3d-cc6191ebc001@enterprisedb.com>
 <87y1axc1o1.fsf@163.com>
 <05aca065-f1f1-48af-b0e8-5eaa72ee6b6f@enterprisedb.com>
 <87ttllbd00.fsf@163.com>
 <CAN-LCVMpvTaWDO6RSKzS4D_-PVTnezRcVgP2O_ciUQiMuMFA1A@mail.gmail.com>
 <233cce7b-d9ae-4605-9b76-1f3b98cbc7f0@enterprisedb.com>
 <CAN-LCVOMnfGObt-BmRF20qE4Ha3XNDwstrngMHrG-JtJnYfgDQ@mail.gmail.com>
 <ad7c22ce-0e99-45a7-a843-05c7ecdc2826@enterprisedb.com>
 <CAN-LCVOSiWpZrG8F0zDFzE+q=Lp0GM=u5cL2hez=8czPD6Pk9w@mail.gmail.com>
 <CA+TgmoZaY5P=McfT7U=EL0GnNMATS8p=CktRN3MQN5=Z+Z8GuQ@mail.gmail.com>
User-agent: mu4e 1.10.7; emacs 29.1
From: Andy Fan <zhihuifan1213@163.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Nikita Malakhov <hukutoc@gmail.com>, Tomas Vondra
 <tomas.vondra@enterprisedb.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: Wed, 22 May 2024 09:02:55 +0800
In-reply-to: 
 <CA+TgmoZaY5P=McfT7U=EL0GnNMATS8p=CktRN3MQN5=Z+Z8GuQ@mail.gmail.com>
Message-ID: <875xv6k3k1.fsf@163.com>
MIME-Version: 1.0
Content-Type: text/plain
Archived-At: <https://www.postgresql.org/message-id/875xv6k3k1.fsf%40163.com>
Precedence: bulk


Hi Robert, 

> Andy Fan asked me off-list for some feedback about this proposal. I
> have hesitated to comment on it for lack of having studied the matter
> in any detail, but since I've been asked for my input, here goes:

Thanks for doing this! Since we have two totally different designs
between Tomas and me and I think we both understand the two sides of
each design, just that the following things are really puzzled to me.

- which factors we should care more.
- the possiblility to improve the design-A/B for its badness.

But for the point 2, we probably needs some prefer design first.

for now let's highlight that there are 2 totally different method to
handle this problem. Let's call:

design a:  detoast the datum into slot->tts_values (and manage the
memory with the lifespan of *slot*). From me.

design b:  detost the datum into a CACHE (and manage the memory with
CACHE eviction and at released the end-of-query). From Tomas.

Currently I pefer design a, I'm not sure if I want desgin a because a is
really good or just because design a is my own design. So I will
summarize the the current state for the easier discussion. I summarize
them so that you don't need to go to the long discussion, and I summarize
the each point with the link to the original post since I may
misunderstand some points and the original post can be used as a double
check. 

> I agree that there's a problem here, but I suspect this is not the
> right way to solve it. Let's compare this to something like the
> syscache. Specifically, let's think about the syscache on
> pg_class.relname.

OK.

> In the case of the syscache on pg_class.relname, there are two reasons
> why we might repeatedly look up the same values in the cache. One is
> that the code might do multiple name lookups when it really ought to
> do only one. Doing multiple lookups is bad for security and bad for
> performance, but we have code like that in some places and some of it
> is hard to fix. The other reason is that it is likely that the user
> will mention the same table names repeatedly; each time they do, they
> will trigger a lookup on the same name. By caching the result of the
> lookup, we can make it much faster. An important point to note here is
> that the queries that the user will issue in the future are unknown to
> us. In a certain sense, we cannot even know whether the same table
> name will be mentioned more than once in the same query: when we reach
> the first instance of the table name and look it up, we do not have
> any good way of knowing whether it will be mentioned again later, say
> due to a self-join. Hence, the pattern of cache lookups is
> fundamentally unknowable.

True. 

> But that's not the case in the motivating example that started this
> thread. In that case, the target list includes three separate
> references to the same column. We can therefore predict that if the
> column value is toasted, we're going to de-TOAST it exactly three
> times.

True.

> If, on the other hand, the column value were mentioned only
> once, it would be detoasted just once. In that latter case, which is
> probably quite common, this whole cache idea seems like it ought to
> just waste cycles, which makes me suspect that no matter what is done
> to this patch, there will always be cases where it causes significant
> regressions.

I agree.

> In the former case, where the column reference is
> repeated, it will win, but it will also hold onto the detoasted value
> after the third instance of detoasting, even though there will never
> be any more cache hits.

True.

> Here, unlike the syscache case, the cache is
> thrown away at the end of the query, so future queries can't benefit.
> Even if we could find a way to preserve the cache in some cases, it's
> not very likely to pay off. People are much more likely to hit the
> same table more than once than to hit the same values in the same
> table more than once in the same session.

I agree.

One more things I want to highlight it "syscache" is used for metadata
and *detoast cache* is used for user data. user data is more 
likely bigger than metadata, so cache size control (hence eviction logic
run more often) is more necessary in this case. The first graph in [1]
(including the quota text) highlight this idea.

> Suppose we had a design where, when a value got detoasted, the
> detoasted value went into the place where the toasted value had come
> from. Then this problem would be handled pretty much perfectly: the
> detoasted value would last until the scan advanced to the next row,
> and then it would be thrown out. So there would be no query-lifespan
> memory leak.

I think that is same idea as design a.

> Now, I don't really know how this would work, because the
> TOAST pointer is coming out of a slot and we can't necessarily write
> one attribute of any arbitrary slot type without a bunch of extra
> overhead, but maybe there's some way.

I think the key points includes:

1. Where to put the *detoast value* so that ExprEval system can find out
it cheaply. The soluation in A slot->tts_values[*].

2. How to manage the memory for holding the detoast value.

The soluation in A is:

- using a lazy created MemoryContext in slot.
- let the detoast system detoast the datum into the designed
MemoryContext.

3. How to let Expr evalution run the extra cycles when needed.

The soluation in A is:

EEOP_XXX_TOAST step is designed to get riding of some *run-time* *if
(condition)" . I found the existing ExprEval system have many
specialized steps.

I will post a detailed overall design later and explain the its known
badness so far.

> If it could be done cheaply enough, it would gain all the benefits of
> this proposal a lot more cheaply and with fewer
> downsides.

I agree. 

> Alternatively, maybe there's a way 
> to notice the multiple references and introduce some kind of
> intermediate slot or other holding area where the detoasted value can
> be stored.
>
> In other words, instead of computing
>
> big_jsonb_col->'a',  big_jsonb_col->'b',  big_jsonb_col->'c'
>
> Maybe we ought to be trying to compute this:
>
> big_jsonb_col=detoast(big_jsonb_col)
> big_jsonb_col->'a',  big_jsonb_col->'b',  big_jsonb_col->'c'
>
> Or perhaps this:
>
> tmp=detoast(big_jsonb_col)
> tmp->'a', tmp->'b', tmp->'c'

Current the design a doesn't use 'tmp' so that the existing ExprEval
engine can find out "detoast datum" easily, so it is more like:

> big_jsonb_col=detoast(big_jsonb_col)
> big_jsonb_col->'a',  big_jsonb_col->'b',  big_jsonb_col->'c'


[1] https://www.postgresql.org/message-id/87ttllbd00.fsf%40163.com

-- 
Best Regards
Andy Fan