Re: [Proposal] Global temporary tables

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