Date: Mon, 13 Jan 2020 17:32:53 +0100
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
Cc: =?utf-8?B?5pu+5paH5peMKOS5ieS7jik=?= <wenjing.zwj@alibaba-inc.com>,
	Pavel Stehule <pavel.stehule@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
	=?utf-8?B?6JSh5p2+6ZyyKOWtkOWYiSk=?= <zijia@taobao.com>,
	"Cai, Le" <le.cai@alibaba-inc.com>,
	=?utf-8?B?6JCn5bCR6IGqKOmTgeW6tSk=?= <shaocong.xsc@alibaba-inc.com>
Subject: Re: [Proposal] Global temporary tables
Message-ID: <20200113163253.ja34iqsors3o44sv@development>
References: <80cf30c2-ebd5-a568-9f62-a970e4d91326@postgrespro.ru>
 <5767433E-1A77-4682-B3FD-4869911021C8@alibaba-inc.com>
 <6b7570aa-14ea-2f28-c3b5-74dd785bbafe@postgrespro.ru>
 <20200105200603.wrb4beacwb57hhpk@development>
 <33969545-BEF7-4518-A741-302CBF7FF269@alibaba-inc.com>
 <0498c45c-53cb-d9b0-c1e2-7d2d1a618511@postgrespro.ru>
 <20200109164829.mdl3z5xalmaqxs67@development>
 <3fef844f-0e73-8fcb-e4b6-595d6317b191@postgrespro.ru>
 <20200112015109.caab6qdr2ob6d5jv@development>
 <c5f9b7ab-9156-f2f4-d162-99448c3aecfc@postgrespro.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <c5f9b7ab-9156-f2f4-d162-99448c3aecfc@postgrespro.ru>
Precedence: bulk

On Mon, Jan 13, 2020 at 11:08:40AM +0300, Konstantin Knizhnik wrote:
>
>
>On 12.01.2020 4:51, Tomas Vondra wrote:
>>On Fri, Jan 10, 2020 at 11:47:42AM +0300, Konstantin Knizhnik wrote:
>>>
>>>
>>>On 09.01.2020 19:48, Tomas Vondra wrote:
>>>>
>>>>>The most complex and challenged task is to support GTT for all 
>>>>>kind of indexes. Unfortunately I can not proposed some good 
>>>>>universal solution for it.
>>>>>Just patching all existed indexes implementation seems to be 
>>>>>the only choice.
>>>>>
>>>>
>>>>I haven't looked at the indexing issue closely, but IMO we need to
>>>>ensure that every session sees/uses only indexes on GTT that were
>>>>defined before the seesion started using the table.
>>>
>>>Why? It contradicts with behavior of normal tables.
>>>Assume that you have active clients and at some point of time DBA 
>>>recognizes that them are spending to much time in scanning some 
>>>GTT.
>>>It cab create index for this GTT but if existed client will not be 
>>>able to use this index, then we need somehow make this clients to 
>>>restart their sessions?
>>>In my patch I have implemented building indexes for GTT on demand: 
>>>if accessed index on GTT is not yet initialized, then it is filled 
>>>with local data.
>>
>>Yes, I know the behavior would be different from behavior for regular
>>tables. And yes, it would not allow fixing slow queries in sessions
>>without interrupting those sessions.
>>
>>I proposed just ignoring those new indexes because it seems much simpler
>>than alternative solutions that I can think of, and it's not like those
>>other solutions don't have other issues.
>
>Quit opposite: prohibiting sessions to see indexes created before 
>session start to use GTT requires more efforts. We need to somehow 
>maintain and check GTT first access time.
>

Hmmm, OK. I'd expect such check to be much simpler than the on-demand
index building, but I admit I haven't tried implementing either of those
options.

>>
>>For example, I've looked at the "on demand" building as implemented in
>>global_private_temp-8.patch, I kinda doubt adding a bunch of index build
>>calls into various places in index code seems somewht suspicious.
>
>We in any case has to initialize GTT indexes on demand even if we 
>prohibit usages of indexes created after first access by session to 
>GTT.
>So the difference is only in one thing: should we just initialize 
>empty index or populate it with local data (if rules for index 
>usability are the same for GTT as for normal tables).
From implementation point of view there is no big difference. Actually 
>building index in standard way is even simpler than constructing empty 
>index. Originally I have implemented
>first approach (I just forgot to consider case when GTT was already 
>user by a session). Then I rewrited it using second approach and patch 
>even became simpler.
>
>>
>>* brinbuild is added to brinRevmapInitialize, which is meant to
>>  initialize state for scanning. It seems wrong to build the index we're
>>  scanning from this function (layering and all that).
>>
>>* btbuild is called from _bt_getbuf. That seems a bit ... suspicious?
>
>
>As I already mentioned - support of indexes for GTT is one of the most 
>challenged things in my patch.
>I didn't find good and universal solution. So I agreed that call of 
>btbuild from _bt_getbuf may be considered as suspicious.
>I will be pleased if you or sombody else can propose better 
>elternative and not only for B-Tree, but for all other indexes.
>
>But as I already wrote above, prohibiting session to used indexes 
>created after first access to GTT doesn't solve the problem.
>For normal tables (and for local temp tables) indexes are initialized 
>at the time of their creation.
>With GTT it doesn't work, because each session has its own local data 
>of GTT.
>We should either initialize/build index on demand (when it is first 
>accessed), either at the moment of session start initialize indexes 
>for all existed GTTs.
>Last options seem to be much worser from my point of view: there may 
>me huge number of GTT and session may not need to access GTT at all.
>>
>>... and so on for other index types. Also, what about custom indexes
>>implemented in extensions? It seems a bit strange each of them has to
>>support this separately.
>
>I have already complained about it: my patch supports GTT for all 
>built-in indexes, but custom indexes has to handle it themselves.
>Looks like to provide some generic solution we need to extend index 
>API, providing two diffrent operations: creation and initialization.
>But extending index API is very critical change... And also it doesn't 
>solve the problem with all existed extensions: them in any case have
>to be rewritten to implement new API version in order to support GTT.
>

Why not to allow creating only indexes implementing this new API method
(on GTT)?

>>
>>IMHO if this really is the right solution, we need to make it work for
>>existing indexes without having to tweak them individually. Why don't we
>>track a flag whether an index on GTT was initialized in a given session,
>>and if it was not then call the build function before calling any other
>>function from the index AM?
>>But let's talk about other issues caused by "on demand" build. Imagine
>>you have 50 sessions, each using the same GTT with a GB of per-session
>>data. Now you create a new index on the GTT, which forces the sessions
>>to build it's "local" index. Those builds will use maintenance_work_mem
>>each, so 50 * m_w_m. I doubt that's expected/sensible.
>
>I do not see principle difference here with scenario when 50 sessions 
>create (local) temp table,
>populate it with GB of data and create index for it.
>

I'd say the high memory consumption is pretty significant.

>>
>>So I suggest we start by just ignoring the *new* indexes, and improve
>>this in the future (by building the indexes on demand or whatever).
>
>Sorry, but still do not agree with this suggestions:
>- it doesn't simplify things
>- it makes behavior of GTT incompatible with normal tables.
>- it doesn't prevent some bad or unexpected behavior which can't be 
>currently reproduced with normal (local) temp tables.
>
>>
>>>>
>>>>I think someone pointed out pushing stuff directly into the cache is
>>>>rather problematic, but I don't recall the details.
>>>>
>>>I have not encountered any problems, so if you can point me on 
>>>what is wrong with this approach, I will think about alternative 
>>>solution.
>>>
>>
>>I meant this comment by Robert:
>>
>>https://www.postgresql.org/message-id/CA%2BTgmoZFWaND4PpT_CJbeu6VZGZKi2rrTuSTL-Ykd97fexTN-w%40mail.gmail.com
>>
>>
>"if any code tried to access the statistics directly from the table, 
>rather than via the caches".
>
>Currently optimizer is accessing statistic though caches. So this 
>approach works. If somebody will rewrite optimizer or provide own 
>custom optimizer in extension which access statistic directly
>then it we really be a problem. But I wonder why bypassing catalog 
>cache may be needed.
>

I don't know, but it seems extensions like hypopg do it.

>Moreover, if we implement alternative solution - for example make 
>pg_statistic a view which combines results for normal tables and GTT, 
>then existed optimizer has to be rewritten
>because it can not access statistic in the way it is doing now. And 
>there will be all problem with all existed extensions which are 
>accessing statistic in most natural way - through system cache.
>

Perhaps. I don't know enough about this part of the code to have a
strong opinion.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services