subject:"Global temporary tables"

On Wed, Jan 29, 2020 at 10:30 AM Konstantin Knizhnik
 wrote:
> But please consider two arguments:
>
> 1. Index for GTT in any case has to be initialized on demand. In case of
> regular tables index is initialized at the moment of its creation. In
> case of GTT it doesn't work.
> So we should somehow detect that accessed index is not initialized and
> perform lazy initialization of the index.
> The only difference with the approach proposed by Pavel  (allow index
> for empty GTT but prohibit it for GTT filled with data) is whether we
> also need to populate index with data or not.
> I can imagine that implicit initialization of index in read-only query
> (select) can be unsafe and cause some problems. I have not encountered
> such problems yet after performing many tests with GTTs, but certainly I
> have not covered all possible scenarios (not sure that it is possible at
> all).
> But I do not understand how populating  index with data can add some
> extra unsafety.
>
> So I can not prove that building index for GTT on demand is safe, but it
> is not more unsafe than initialization of index on demand which is
> required in any case.

I think that the idea of calling ambuild() on the fly is not going to
work, because, again, I don't think that calling that from random
places in the code is safe. What I expect we're going to need to do
here is model this on the approach used for unlogged tables. For an
unlogged table, each table and index has an init fork which contains
the correct initial contents for that relation - which is nothing at
all for a heap table, and a couple of boilerplate pages for an index.
In the case of an unlogged table, the init forks get copied over the
main forks after crash recovery, and then we have a brand-new, empty
relation with brand-new empty indexes which everyone can use. In the
case of global temporary tables, I think that we should do the same
kind of copying, but at the time when the session first tries to
access the table. There is some fuzziness in my mind about what
exactly constitutes accessing the table - it probably shouldn't be
when the relcache entry is built, because that seems too early, but
I'm not sure what is exactly right. In any event, it's easier to find
a context where copying some files on disk (that are certain not to be
changing) is safe than it is to find a context where index builds are
safe.

> 2. Actually I do not propose some completely new approach. I try to
> provide behavior with is compatible with regular tables.
> If you create index for regular table, then it can be used in all
> sessions, right?

Yes. :-)

> And all "various backend-local data structures in the relcache, the
> planner, and the executor that remember information about indexes"
> have to be properly updated.  It is done using invalidation mechanism.
> The same mechanism is used in case of DDL operations with GTT, because
> we change system catalog.

I mean, that's not really a valid argument. Invalidations can only
take effect at certain points in the code, and the whole argument here
is about which places in the code are safe for which operations, so
the fact that some things (like accepting invalidations) are safe at
some points in the code (like the places where we accept them) does
not prove that other things (like calling ambuild) are safe at other
points in the code (like wherever you are proposing to call it). In
particular, if you've got a relation open, there's currently no way
for another index to show up while you've still got that relation
open. That means that the planner and executor (which keep the
relevant relations open) don't ever have to worry about updating their
data structures, because it can never be necessary. It also means that
any code anywhere in the system that keeps a lock on a relation can
count on the list of indexes for that relation staying the same until
it releases the lock. In fact, it can hold on to pointers to data
allocated by the relcache and count on those pointers being stable for
as long as it holds the lock, and RelationClearRelation contain
specific code that aims to make sure that certain objects don't get
deallocated and reallocated at a different address precisely for that
reason. That code, however, only works as long as nothing actually
changes. The upshot is that it's entirely possible for changing
catalog entries in one backend with an inadequate lock level -- or at
unexpected point in the code -- to cause a core dump either in that
backend or in some other backend. This stuff is really subtle, and
super-easy to screw up.

I am speaking a bit generally here, because I haven't really studied
*exactly* what might go wrong in the relcache, or elsewhere, as a
result of creating an index on the fly. However, I'm very sure that a
general appeal to invalidation messages is not sufficient to make
something like what you want to do saf

Re: [Proposal] Global temporary tables

2020-01-29 Thread Pavel Stehule

st 29. 1. 2020 v 18:21 odesílatel Konstantin Knizhnik <
k.knizh...@postgrespro.ru> napsal:

>
>
> On 29.01.2020 20:08, Pavel Stehule wrote:
>
>
>
>
> 2. Actually I do not propose some completely new approach. I try to
>> provide behavior with is compatible with regular tables.
>> If you create index for regular table, then it can be used in all
>> sessions, right?
>>
>
> I don't understand to this point. Regular tables shares data, shares
> files. You cannot to separate it. More - you have to uses relatively
> aggressive locks to be this operation safe.
>
> Nothing from these points are valid for GTT.
>
>
> GTT shares metadata.
> As far as them are not sharing data, then GTT are safer than regular
> table, aren't them?
> "Safer" means that we need less "aggressive" locks for them: we need to
> protect only metadata, not data itself.
>
> My point is that if we allow other sessions to access created indexes for
> regular tables, then it will be not more complex to support it for GTT.
> Actually "not more complex" in this case means "no extra efforts are
> needed".
>

It is hard to say. I see a significant difference. When I do index on
regular table, then I don't change a context of other processes. I have to
wait for lock, and after I got a lock then other processes waiting.

With GTT, I don't want to wait for others - and other processes should
build indexes inside - without expected sequence of operations. Maybe it
can have positive effect, but it can have negative effect too. In this case
I prefer (in this moment) zero effect on other sessions. So I would to
build index in my session and I don't would to wait for other sessions, and
if it is possible other sessions doesn't need to interact or react on my
action too. It should be independent what is possible. The most simple
solution is request on unique usage. I understand so it can be not too
practical. Better is allow to usage GTT by other tables, but the changes
are invisible in other sessions to session reset. It is minimalistic
strategy. It has not benefits for other sessions, but it has not negative
impacts too.

> --
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>

Re: [Proposal] Global temporary tables

2020-01-29 Thread Konstantin Knizhnik




On 29.01.2020 20:08, Pavel Stehule wrote:




2. Actually I do not propose some completely new approach. I try to
provide behavior with is compatible with regular tables.
If you create index for regular table, then it can be used in all
sessions, right?


I don't understand to this point. Regular tables shares data, shares 
files. You cannot to separate it. More - you have to uses relatively 
aggressive locks to be this operation safe.


Nothing from these points are valid for GTT.


GTT shares metadata.
As far as them are not sharing data, then GTT are safer than regular 
table, aren't them?
"Safer" means that we need less "aggressive" locks for them: we need to 
protect only metadata, not data itself.


My point is that if we allow other sessions to access created indexes 
for regular tables, then it will be not more complex to support it for GTT.
Actually "not more complex" in this case means "no extra efforts are 
needed".


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [Proposal] Global temporary tables

2020-01-29 Thread Pavel Stehule

2. Actually I do not propose some completely new approach. I try to
> provide behavior with is compatible with regular tables.
> If you create index for regular table, then it can be used in all
> sessions, right?
>

I don't understand to this point. Regular tables shares data, shares files.
You cannot to separate it. More - you have to uses relatively aggressive
locks to be this operation safe.

Nothing from these points are valid for GTT.

Regards

Pavel


> And all "various backend-local data structures in the relcache, the
> planner, and the executor that remember information about indexes"
> have to be properly updated.  It is done using invalidation mechanism.
> The same mechanism is used in case of DDL operations with GTT, because
> we change system catalog.
>
> So my point here is that creation index of GTT is almost the same as
> creation of index for regular tables and the same mechanism will be used
> to provide correctness of this operation.
>
>
> --
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>

Re: [Proposal] Global temporary tables

2020-01-29 Thread Konstantin Knizhnik





On 29.01.2020 17:47, Robert Haas wrote:

On Wed, Jan 29, 2020 at 3:13 AM Konstantin Knizhnik
 wrote:

But I heard only two arguments:

1. Concurrent building of indexes by all backends may consume much memory 
(n_backends * maintenance_work_mem) and consume a lot of disk/CPU resources.
2. GTT in one session can contains large amount of data and we need index for 
it, but small amount of data in another session and we do not need index for it.

You seem to be ignoring the fact that two committers told you this
probably wasn't safe.

Perhaps your view is that those people made no argument, and therefore
you don't have to respond to it. But the onus is not on somebody else
to tell you why a completely novel idea is not safe. The onus is on
you to analyze it in detail and prove that it is safe. What you need
to show is that there is no code anywhere in the system which will be
confused by an index springing into existence at whatever time you're
creating it.

One problem is that there are various backend-local data structures in
the relcache, the planner, and the executor that remember information
about indexes, and that may not respond well to having more indexes
show up unexpectedly. On the one hand, they might crash; on the other
hand, they might ignore the new index when they shouldn't. Another
problem is that the code which creates indexes might fail or misbehave
when run in an environment different from the one in which it
currently runs. I haven't really studied your code, so I don't know
exactly what it does, but for example it would be really bad to try to
build an index while holding a buffer lock, both because it might
cause (low-probability) undetected deadlocks and also because it might
block another process that wants that buffer lock in a
non-interruptible wait state for a long time.

Now, maybe you can make an argument that you only create indexes at
points in the query that are "safe." But I am skeptical, because of
this example:

rhaas=# create table foo (a int primary key, b text, c text, d text);
CREATE TABLE
rhaas=# create function blump() returns trigger as $$begin create
index on foo (b); return new; end$$ language plpgsql;
CREATE FUNCTION
rhaas=# create trigger thud before insert on foo execute function blump();
CREATE TRIGGER
rhaas=# insert into foo (a) select generate_series(1,10);
ERROR:  cannot CREATE INDEX "foo" because it is being used by active
queries in this session
CONTEXT:  SQL statement "create index on foo (b)"
PL/pgSQL function blump() line 1 at SQL statement

That prohibition is there for some reason. Someone did not just decide
to arbitrarily prohibit it. A CREATE INDEX command run in that context
won't run afoul of many of the things that might be problems in other
places -- e.g. there won't be a buffer lock held. Yet, despite the
fact that a trigger context is safe for executing a wide variety of
user-defined code, this particular operation is not allowed here. That
is the sort of thing that should worry you.

At any rate, even if this somehow were or could be made safe,
on-the-fly index creation is a feature that cannot and should not be
combined with a patch to implement global temporary tables. Surely, it
will require a lot of study and work to get the details right. And so
will GTT. As I said in the other email I wrote, this feature is hard
enough without adding this kind of thing to it. There's a reason why I
never got around to implementing this ten years ago when I did
unlogged tables; I was intending that to be a precursor to the GTT
work. I found that it was too hard and I gave up. I'm glad to see
people trying again, but the idea that we can afford to add in extra
features, or frankly that either of the dueling patches on this thread
are close to committable, is just plain wrong.



Sorry, I really didn't consider statements containing word "probably" as 
arguments.
But I agree with you: it is task of developer of new feature to prove 
that proposed approach is safe rather than of reviewers to demonstrate 
that it is unsafe.

Can I provide such proof now? I afraid that not.
But please consider two arguments:

1. Index for GTT in any case has to be initialized on demand. In case of 
regular tables index is initialized at the moment of its creation. In 
case of GTT it doesn't work.
So we should somehow detect that accessed index is not initialized and 
perform lazy initialization of the index.
The only difference with the approach proposed by Pavel  (allow index 
for empty GTT but prohibit it for GTT filled with data) is whether we 
also need to populate index with data or not.
I can imagine that implicit initialization of index in read-only query 
(select) can be unsafe and cause some problems. I have not encountered 
such problems yet after performing many tests with GTTs, but certainly I 
have not covered all possible scenarios (not sure that it is possible at 
all).
But I do not understand how populating  inde

Re: [Proposal] Global temporary tables

On Wed, Jan 29, 2020 at 3:13 AM Konstantin Knizhnik
 wrote:
> But I heard only two arguments:
>
> 1. Concurrent building of indexes by all backends may consume much memory 
> (n_backends * maintenance_work_mem) and consume a lot of disk/CPU resources.
> 2. GTT in one session can contains large amount of data and we need index for 
> it, but small amount of data in another session and we do not need index for 
> it.

You seem to be ignoring the fact that two committers told you this
probably wasn't safe.

Perhaps your view is that those people made no argument, and therefore
you don't have to respond to it. But the onus is not on somebody else
to tell you why a completely novel idea is not safe. The onus is on
you to analyze it in detail and prove that it is safe. What you need
to show is that there is no code anywhere in the system which will be
confused by an index springing into existence at whatever time you're
creating it.

One problem is that there are various backend-local data structures in
the relcache, the planner, and the executor that remember information
about indexes, and that may not respond well to having more indexes
show up unexpectedly. On the one hand, they might crash; on the other
hand, they might ignore the new index when they shouldn't. Another
problem is that the code which creates indexes might fail or misbehave
when run in an environment different from the one in which it
currently runs. I haven't really studied your code, so I don't know
exactly what it does, but for example it would be really bad to try to
build an index while holding a buffer lock, both because it might
cause (low-probability) undetected deadlocks and also because it might
block another process that wants that buffer lock in a
non-interruptible wait state for a long time.

Now, maybe you can make an argument that you only create indexes at
points in the query that are "safe." But I am skeptical, because of
this example:

rhaas=# create table foo (a int primary key, b text, c text, d text);
CREATE TABLE
rhaas=# create function blump() returns trigger as $$begin create
index on foo (b); return new; end$$ language plpgsql;
CREATE FUNCTION
rhaas=# create trigger thud before insert on foo execute function blump();
CREATE TRIGGER
rhaas=# insert into foo (a) select generate_series(1,10);
ERROR:  cannot CREATE INDEX "foo" because it is being used by active
queries in this session
CONTEXT:  SQL statement "create index on foo (b)"
PL/pgSQL function blump() line 1 at SQL statement

That prohibition is there for some reason. Someone did not just decide
to arbitrarily prohibit it. A CREATE INDEX command run in that context
won't run afoul of many of the things that might be problems in other
places -- e.g. there won't be a buffer lock held. Yet, despite the
fact that a trigger context is safe for executing a wide variety of
user-defined code, this particular operation is not allowed here. That
is the sort of thing that should worry you.

At any rate, even if this somehow were or could be made safe,
on-the-fly index creation is a feature that cannot and should not be
combined with a patch to implement global temporary tables. Surely, it
will require a lot of study and work to get the details right. And so
will GTT. As I said in the other email I wrote, this feature is hard
enough without adding this kind of thing to it. There's a reason why I
never got around to implementing this ten years ago when I did
unlogged tables; I was intending that to be a precursor to the GTT
work. I found that it was too hard and I gave up. I'm glad to see
people trying again, but the idea that we can afford to add in extra
features, or frankly that either of the dueling patches on this thread
are close to committable, is just plain wrong.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [Proposal] Global temporary tables

On Tue, Jan 28, 2020 at 12:12 PM 曾文旌(义从)  wrote:
>> Opinion by Pavel
>> + rel->rd_islocaltemp = true;  <<< if this is valid, then the name of 
>> field "rd_islocaltemp" is not probably best
>> I renamed rd_islocaltemp
>
> I don't see any change?
>
> Rename rd_islocaltemp to rd_istemp  in global_temporary_table_v8-pg13.patch

In view of commit 6919b7e3294702adc39effd16634b2715d04f012, I think
that this has approximately a 0% chance of being acceptable. If you're
setting a field in a way that is inconsistent with the current use of
the field, you're probably doing it wrong, because the field has an
existing purpose to which new code must conform. And if you're not
doing that, then you don't need to rename it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [Proposal] Global temporary tables

On Mon, Jan 27, 2020 at 4:11 AM Konstantin Knizhnik
 wrote:
> I do not see any reasons to allow build local indexes for global table. 
> Yes,it can happen that some session will have small amount of data in 
> particular GTT and another - small amount of data in this table. But if 
> access pattern is the same  (and nature of GTT assumes it), then index in 
> either appreciate, either useless in both cases.

I agree. I think allowing different backends to have different indexes
is overly complicated.

Regarding another point that was raised, I think it's not a good idea
to prohibit DDL on global temporary tables altogether. It should be
fine to change things when no sessions are using the GTT. Going
further and allowing changes when there are attached sessions would be
nice, but I think we shouldn't try. Getting this feature committed is
going to be a huge amount of work with even a minimal feature set;
complicating the problem by adding what are essentially new
DDL-concurrency features on top of the basic feature seems very much
unwise.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [Proposal] Global temporary tables

2020-01-29 Thread 曾文旌(义从)



> 2020年1月29日 上午1:54，Pavel Stehule  写道：
> 
> 
> 
> út 28. 1. 2020 v 18:13 odesílatel Pavel Stehule  > napsal:
> 
> 
> út 28. 1. 2020 v 18:12 odesílatel 曾文旌(义从)  > napsal:
> 
> 
>> 2020年1月29日 上午12:40，Pavel Stehule > > 写道：
>> 
>> 
>> 
>> út 28. 1. 2020 v 17:01 odesílatel 曾文旌(义从) > > napsal:
>> 
>> 
>>> 2020年1月24日 上午4:47，Robert Haas >> > 写道：
>>> 
>>> On Sat, Jan 11, 2020 at 8:51 PM Tomas Vondra
>>> mailto:tomas.von...@2ndquadrant.com>> wrote:
 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.
>>> 
>>> +1.
>> I complete the implementation of this feature.
>> When a session x create an index idx_a on GTT A then
>> For session x, idx_a is valid when after create index.
>> For session y, before session x create index done, GTT A has some data, then 
>> index_a is invalid.
>> For session z, before session x create index done, GTT A has no data, then 
>> index_a is valid.
>> 
>>> 
 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.
>>> 
>>> +1. I can't imagine that's a safe or sane thing to do.
>>> 
>>> -- 
>>> Robert Haas
>>> EnterpriseDB: http://www.enterprisedb.com 
>>> The Enterprise PostgreSQL Company
>> 
>> Opinion by Pavel
>> +rel->rd_islocaltemp = true;  <<< if this is valid, then the name of 
>> field "rd_islocaltemp" is not probably best
>> I renamed rd_islocaltemp
>> 
>> I don't see any change?
> Rename rd_islocaltemp to rd_istemp  in global_temporary_table_v8-pg13.patch
> 
> ok :)
> 
> I found a bug
> 
> postgres=# create global temp table x(a int);
> CREATE TABLE
> postgres=# insert into x values(1);
> INSERT 0 1
> postgres=# create index on x (a);
> CREATE INDEX
> postgres=# create index on x((a + 1));
> CREATE INDEX
> postgres=# analyze x;
> WARNING:  oid 16468 not a relation
> ANALYZE
Thanks for review.

The index expression need to store statistics on index, I missed it and I'll 
fix it later.


Wenjing

> 
> other behave looks well for me.
> 
> Regards
> 
> Pavel
> 
> 
> Pavel
> 
> 
> Wenjing
> 
> 
> 
>> 
>> 
>> 
>> Opinion by Konstantin Knizhnik
>> 1 Fixed comments
>> 2 Fixed assertion
>> 
>> 
>> Please help me review.
>> 
>> 
>> Wenjing
>> 
>

Re: [Proposal] Global temporary tables

2020-01-29 Thread Konstantin Knizhnik




On 27.01.2020 22:44, Pavel Stehule wrote:


I don't think so compatibility with Oracle is valid point in this 
case. We need GTT, but the mechanism of index building should be 
designed for Postgres, and for users.


Maybe the method proposed by you can be activated by some option like 
CREATE INDEX IMMEDIATELY FOR ALL SESSION. When you use GTT without 
index, then
it should to work some time more, and if you use short life sessions, 
then index build can be last or almost last operation over table and 
can be suboptimal.


Anyway, this behave can be changed later without bigger complications 
- and now I am have strong opinion to prefer don't allow to any DDL 
(with index creation) on any active GTT in other sessions.
Probably your proposal - build indexes on other sessions when GTT is 
touched can share code with just modify metadata and wait on session 
reset or GTT reset


Well, compatibility with Oracle was never treated as important argument 
in this group:)

But I hope that you agree that it real argument against your proposal.
Much more important argument is incompatibility with behavior of regular 
table.
If you propose such incompatibility, then you should have some very 
strong arguments for such behavior which will definitely confuse users.


But I heard only two arguments:

1. Concurrent building of indexes by all backends may consume much 
memory (n_backends * maintenance_work_mem) and consume a lot of disk/CPU 
resources.


First of all it is not completely true. Indexes will be created on 
demand when GTT will be accessed and chance that all sessions will 
become building indexes simultaneously is very small.


But what will happen if we prohibit access to this index for existed 
sessions? If we need index for GTT, then most likely it is used for joins.
If there is no index, then optimizer has to choose some other plan to 
perform this join. For example use hash join. Hash join also requires 
memory,
so if all backends will perform such join simultaneously, then them 
consume (n_backends * work_mem) memory.
Yes, work_mem is used to be smaller than maintenance_work_mem. But in 
any case DBA has a choice to adjust this parameters to avoid this problem.
And in case of your proposal (prohibit access to this index) you give 
him no choice to optimize query execution in existed sessions.


Also if all sessions will simultaneously perform sequential scan of GTT 
instead of building index for it, then them will read the same amount of 
data and consume comparable CPU time.
So prohibiting access to the indexes will not save us from high 
resources consumption if all existed sessions are really actively 
working with this GTT.


2. GTT in one session can contains large amount of data and we need 
index for it, but small amount of data in another session and we do not 
need index for it.


Such situation definitely can happen. But it contradicts to the main 
assumption of GTT use case (that it is accessed in the same way by all 
sessions).
Also I may be agree with this argument if you propose to create indexes 
locally for each sessions.
But your proposal is to prohibit access to the index to the sessions 
which already have populated GTT with data but allow it for sessions 
which have not accessed this GTT yet.
So if some session stores some data in GTT after index was created, then 
it will build index for it, doesn't matter whether size of table is 
small or large.
Why do we make an exception for sessions which already have data in GTT 
in this case?


So from my point of view both arguments are doubtful and can not explain 
why rules of index usability for GTT should be different from regular 
tables.


Usually it is not hard problem to refresh sessions, and what I know 
when you update plpgsql code, it is best practice to refresh session 
early.




I know may systems where session is established once client is connected 
to the system and not closed until client is disconnected.
And any attempt to force termination of the session will cause 
application errors which are not expected by the client.



Sorry, I think that it is principle point in discussion concerning GTT 
design.
Implementation of GTT can be changed in future, but it is bad if 
behavior of GTT will be changed.
It is not clear for me why from the very beginning we should provide 
inconsistent behavior which is even more difficult to implement than 
behavior compatible with regular tables.

And say that in the future it can be changed...

Sorry, but I do not consider proposals to create indexes locally for 
each session (i.e. global tables but private indexes) or use some 
special complicated SQL syntax constructions like
CREATE INDEX IMMEDIATELY FOR ALL SESSION as some real alternatives which 
have to be discussed.


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [Proposal] Global temporary tables

2020-01-28 Thread Pavel Stehule

út 28. 1. 2020 v 18:13 odesílatel Pavel Stehule 
napsal:

>
>
> út 28. 1. 2020 v 18:12 odesílatel 曾文旌(义从) 
> napsal:
>
>>
>>
>> 2020年1月29日 上午12:40，Pavel Stehule  写道：
>>
>>
>>
>> út 28. 1. 2020 v 17:01 odesílatel 曾文旌(义从) 
>> napsal:
>>
>>>
>>>
>>> 2020年1月24日 上午4:47，Robert Haas  写道：
>>>
>>> On Sat, Jan 11, 2020 at 8:51 PM Tomas Vondra
>>>  wrote:
>>>
>>> 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.
>>>
>>>
>>> +1.
>>>
>>> I complete the implementation of this feature.
>>> When a session x create an index idx_a on GTT A then
>>> For session x, idx_a is valid when after create index.
>>> For session y, before session x create index done, GTT A has some data,
>>> then index_a is invalid.
>>> For session z, before session x create index done, GTT A has no data,
>>> then index_a is valid.
>>>
>>>
>>> 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.
>>>
>>>
>>> +1. I can't imagine that's a safe or sane thing to do.
>>>
>>> --
>>> Robert Haas
>>> EnterpriseDB: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>>
>>> Opinion by Pavel
>>> + rel->rd_islocaltemp = true;  <<< if this is valid, then the name
>>> of field "rd_islocaltemp" is not probably best
>>> I renamed rd_islocaltemp
>>>
>>
>> I don't see any change?
>>
>> Rename rd_islocaltemp to rd_istemp
>> in global_temporary_table_v8-pg13.patch
>>
>
> ok :)
>

I found a bug

postgres=# create global temp table x(a int);
CREATE TABLE
postgres=# insert into x values(1);
INSERT 0 1
postgres=# create index on x (a);
CREATE INDEX
postgres=# create index on x((a + 1));
CREATE INDEX
postgres=# analyze x;
WARNING:  oid 16468 not a relation
ANALYZE

other behave looks well for me.

Regards

Pavel


> Pavel
>
>>
>>
>> Wenjing
>>
>>
>>
>>
>>
>>
>>> Opinion by Konstantin Knizhnik
>>> 1 Fixed comments
>>> 2 Fixed assertion
>>>
>>>
>>> Please help me review.
>>>
>>>
>>> Wenjing
>>>
>>>
>>

Re: [Proposal] Global temporary tables

2020-01-28 Thread Pavel Stehule

út 28. 1. 2020 v 18:12 odesílatel 曾文旌(义从) 
napsal:

>
>
> 2020年1月29日 上午12:40，Pavel Stehule  写道：
>
>
>
> út 28. 1. 2020 v 17:01 odesílatel 曾文旌(义从) 
> napsal:
>
>>
>>
>> 2020年1月24日 上午4:47，Robert Haas  写道：
>>
>> On Sat, Jan 11, 2020 at 8:51 PM Tomas Vondra
>>  wrote:
>>
>> 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.
>>
>>
>> +1.
>>
>> I complete the implementation of this feature.
>> When a session x create an index idx_a on GTT A then
>> For session x, idx_a is valid when after create index.
>> For session y, before session x create index done, GTT A has some data,
>> then index_a is invalid.
>> For session z, before session x create index done, GTT A has no data,
>> then index_a is valid.
>>
>>
>> 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.
>>
>>
>> +1. I can't imagine that's a safe or sane thing to do.
>>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>>
>> Opinion by Pavel
>> + rel->rd_islocaltemp = true;  <<< if this is valid, then the name of
>> field "rd_islocaltemp" is not probably best
>> I renamed rd_islocaltemp
>>
>
> I don't see any change?
>
> Rename rd_islocaltemp to rd_istemp
> in global_temporary_table_v8-pg13.patch
>

ok :)

Pavel

>
>
> Wenjing
>
>
>
>
>
>
>> Opinion by Konstantin Knizhnik
>> 1 Fixed comments
>> 2 Fixed assertion
>>
>>
>> Please help me review.
>>
>>
>> Wenjing
>>
>>
>

Re: [Proposal] Global temporary tables

2020-01-28 Thread Pavel Stehule

út 28. 1. 2020 v 17:01 odesílatel 曾文旌(义从) 
napsal:

>
>
> 2020年1月24日 上午4:47，Robert Haas  写道：
>
> On Sat, Jan 11, 2020 at 8:51 PM Tomas Vondra
>  wrote:
>
> 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.
>
>
> +1.
>
> I complete the implementation of this feature.
> When a session x create an index idx_a on GTT A then
> For session x, idx_a is valid when after create index.
> For session y, before session x create index done, GTT A has some data,
> then index_a is invalid.
> For session z, before session x create index done, GTT A has no data,
> then index_a is valid.
>
>
> 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.
>
>
> +1. I can't imagine that's a safe or sane thing to do.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
> Opinion by Pavel
> + rel->rd_islocaltemp = true;  <<< if this is valid, then the name of
> field "rd_islocaltemp" is not probably best
> I renamed rd_islocaltemp
>

I don't see any change?



> Opinion by Konstantin Knizhnik
> 1 Fixed comments
> 2 Fixed assertion
>
>
> Please help me review.
>
>
> Wenjing
>
>

Re: [Proposal] Global temporary tables

2020-01-27 Thread Pavel Stehule

po 27. 1. 2020 v 10:11 odesílatel Konstantin Knizhnik <
k.knizh...@postgrespro.ru> napsal:

>
>
> On 24.01.2020 22:39, Pavel Stehule wrote:
>
> I cannot to evaluate your proposal, and I am sure, so you know more about
> this code.
>
> There is a question if we can allow to build local temp index on global
> temp table. It is different situation. When I work with global properties
> personally I prefer total asynchronous implementation of any DDL operations
> for other than current session. When it is true, then I have not any
> objection. For me, good enough design of any DDL can be based on catalog
> change without forcing to living tables.
>
>
> From my point of view there are two difference uses cases of temp tables:
> 1. Backend needs some private data source which is specific to this
> session and has no relation with activities of other sessions.
> 2. We need a table  containing private session data, but which is used in
> the same way by all database users.
>
> In the first case current Postgres temp tables works well (if we forget
> for a moment about all known issues related with temp tables).
> Global temp tables are used to address the second scenario.  Assume that
> we write some stored procedure or implement some business logic  outside
> database and
> what to perform some complex analtic query which requires tepmp table for
> storing intermediate results. In this case we can create GTT with all
> needed index at the moment of database initialization
> and do not perform any extra DDL during query execution. If will prevent
> catalog bloating and makes execution of query more efficient.
>
> I do not see any reasons to allow build local indexes for global table.
> Yes,it can happen that some session will have small amount of data in
> particular GTT and another - small amount of data in this table. But if
> access pattern is the same  (and nature of GTT assumes it), then index in
> either appreciate, either useless in both cases.
>
>
>
>
> I see following disadvantage of your proposal. See scenario
>
> 1. I have two sessions
>
> A - small GTT with active owner
> B - big GTT with some active application.
>
> session A will do new index - it is fast, but if creating index is forced
> on B on demand (when B was touched), then this operation have to wait after
> index will be created.
>
> So I afraid build a index on other sessions on GTT when GTT tables in
> other sessions will not be empty.
>
>
>
> Yes, it is true. But is is not the most realistic scenario from my point
> of view.
> As I explained above, GTT should be used when we need temporary storage
> accessed in the same way by all clients.
> If (as with normal tables) at some moment of time DBA realizes, that
> efficient execution of some queries needs extra indexes,
> then it should be able to do it. It is very inconvenient and unnatural to
> prohibit DBA to do it until all sessions using this GTT are closed (it may
> never happen)
> or require all sessions to restart to be able to use this index.
>
> So it is possible to imagine different scenarios of working with GTTs.
> But from my point of view the only non-contradictory model of their
> behavior is to make it compatible with normal tables.
> And do not forget about compatibility with Oracle. Simplifying of porting
> existed applications from Oracle to Postgres  may be the
> main motivation of adding GTT to Postgres. And making them incompatible
> with Oracle will be very strange.
>

I don't think so compatibility with Oracle is valid point in this case. We
need GTT, but the mechanism of index building should be designed for
Postgres, and for users.

Maybe the method proposed by you can be activated by some option like
CREATE INDEX IMMEDIATELY FOR ALL SESSION. When you use GTT without index,
then
it should to work some time more, and if you use short life sessions, then
index build can be last or almost last operation over table and can be
suboptimal.

Anyway, this behave can be changed later without bigger complications - and
now I am have strong opinion to prefer don't allow to any DDL (with index
creation) on any active GTT in other sessions.
Probably your proposal - build indexes on other sessions when GTT is
touched can share code with just modify metadata and wait on session reset
or GTT reset

Usually it is not hard problem to refresh sessions, and what I know when
you update plpgsql code, it is best practice to refresh session early.



>
> --
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>

Re: [Proposal] Global temporary tables

2020-01-27 Thread Konstantin Knizhnik




On 25.01.2020 18:15, 曾文旌(义从) wrote:

I wonder why do we need some special check for GTT here.
From my point of view cleanup at startup of local storage of temp 
tables should be performed in the same way for local and global temp 
tables.
After oom kill, In autovacuum, the Isolated local temp table will be 
cleaned like orphan temporary tables. The definition of local temp 
table is deleted with the storage file.

But GTT can not do that. So we have the this implementation in my patch.
If you have other solutions, please let me know.

I wonder if it is possible that autovacuum or some other Postgres 
process is killed by OOM and postmaster is not noticing it can doens't 
restart Postgres instance?
as far as I know, crash of any process connected to Postgres shared 
memory (and autovacuum definitely has such connection) cause Postgres 
restart.




In my design
1 Because different sessions have different transaction information, I 
choose to store the transaction information of GTT in MyProc,not catalog.
2 About the XID wraparound problem, the reason is the design of the 
temp table storage(local temp table and global temp table) that makes 
it can not to do vacuum by autovacuum.

It should be completely solve at the storage level.



My point of view is that vacuuming of temp tables is common problem for 
local and global temp tables.
So it has to be addressed in the common way and so we should not try to 
fix this problem only for GTT.




In fact, The dba can still complete the DDL of the GTT.
I've provided a set of functions for this case.
If the dba needs to modify a GTT A(or drop GTT or create index on 
GTT), he needs to do:
1 Use the pg_gtt_attached_pids view to list the pids for the session 
that is using the GTT A.

2 Use pg_terminate_backend(pid)terminate they except itself.
3 Do alter GTT A.


IMHO forced terminated of client sessions is not acceptable solution.
And it is not an absolutely necessary requirement.
So from my point of view we should not add such limitations to GTT design.





What are the reasons of using RowExclusiveLock for GTT instead of 
AccessExclusiveLock?
Yes, GTT data is access only by one backend so no locking here seems 
to be needed at all.
But I wonder what are the motivations/benefits of using weaker lock 
level here?
1 Truncate GTT deletes only the data in the session, so no need use 
high-level lock.
2 I think it still needs to be block by DDL of GTT, which is why I use 
RowExclusiveLock.


Sorry, I do not understand your arguments: we do not need exclusive lock 
because we drop only local (private) data

but we need some kind of lock. I agree with 1) and not 2).




There should be no conflicts in any case...

+        /* We allow to create index on global temp table only this 
session use it */

+        if (is_other_backend_use_gtt(heapRelation->rd_node))
+            elog(ERROR, "can not create index when have other 
backend attached this global temp table");

+

The same argument as in case of dropping GTT: I do not think that 
prohibiting DLL operations on GTT used by more than one backend is 
bad idea.
The idea was to give the GTT almost all the features of a regular 
table with few code changes.
The current version DBA can still do all DDL for GTT, I've already 
described.


I absolutely agree with you that GTT should be given the same features 
as regular tables.
The irony is that this most natural and convenient behavior is most easy 
to implement without putting some extra restrictions.
Just let indexes for GTT be constructed on demand. It it can be done 
using the same function used for regular index creation.







+    /* global temp table not support foreign key constraint yet */
+    if (RELATION_IS_GLOBAL_TEMP(pkrel))
+        ereport(ERROR,
+                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                 errmsg("referenced relation \"%s\" is not a global 
temp table",

+ RelationGetRelationName(pkrel;
+

Why do we need to prohibit foreign key constraint on GTT?
It may be possible to support FK on GTT in later versions. Before 
that, I need to check some code.


Ok,  may be approach to prohibit everything except minimally required 
functionality  is safe and reliable.
But frankly speaking I prefer different approach: if I do not see any 
contradictions of new feature with existed operations
and it is passing tests, then we should  not prohibit this operations 
for new feature.




I have already described my point in previous emails.

1. The core problem is that the data contains transaction information 
(xid), which needs to be vacuum(freeze) regularly to avoid running out 
of xid.
The autovacuum supports vacuum regular table but local temp does not. 
autovacuum also does not support GTT.


2. However, the difference between the local temp table and the global 
temp table(GTT) is that
a) For local temp table: one table hava one piece of data. the 
frozenxid of one local temp table is store in the catalog(pg_class).
b) For global temp

Re: [Proposal] Global temporary tables

2020-01-27 Thread Konstantin Knizhnik




On 24.01.2020 22:39, Pavel Stehule wrote:
I cannot to evaluate your proposal, and I am sure, so you know more 
about this code.


There is a question if we can allow to build local temp index on 
global temp table. It is different situation. When I work with global 
properties personally I prefer total asynchronous implementation of 
any DDL operations for other than current session. When it is true, 
then I have not any objection. For me, good enough design of any DDL 
can be based on catalog change without forcing to living tables.




From my point of view there are two difference uses cases of temp tables:
1. Backend needs some private data source which is specific to this 
session and has no relation with activities of other sessions.
2. We need a table  containing private session data, but which is used 
in the same way by all database users.


In the first case current Postgres temp tables works well (if we forget 
for a moment about all known issues related with temp tables).
Global temp tables are used to address the second scenario.  Assume that 
we write some stored procedure or implement some business logic  outside 
database and
what to perform some complex analtic query which requires tepmp table 
for storing intermediate results. In this case we can create GTT with 
all needed index at the moment of database initialization
and do not perform any extra DDL during query execution. If will prevent 
catalog bloating and makes execution of query more efficient.


I do not see any reasons to allow build local indexes for global table. 
Yes,it can happen that some session will have small amount of data in 
particular GTT and another - small amount of data in this table. But if 
access pattern is the same  (and nature of GTT assumes it), then index 
in either appreciate, either useless in both cases.






I see following disadvantage of your proposal. See scenario

1. I have two sessions

A - small GTT with active owner
B - big GTT with some active application.

session A will do new index - it is fast, but if creating index is 
forced on B on demand (when B was touched), then this operation have 
to wait after index will be created.


So I afraid build a index on other sessions on GTT when GTT tables in 
other sessions will not be empty.



Yes, it is true. But is is not the most realistic scenario from my point 
of view.
As I explained above, GTT should be used when we need temporary storage 
accessed in the same way by all clients.
If (as with normal tables) at some moment of time DBA realizes, that 
efficient execution of some queries needs extra indexes,
then it should be able to do it. It is very inconvenient and unnatural 
to prohibit DBA to do it until all sessions using this GTT are closed 
(it may never happen)

or require all sessions to restart to be able to use this index.

So it is possible to imagine different scenarios of working with GTTs.
But from my point of view the only non-contradictory model of their 
behavior is to make it compatible with normal tables.
And do not forget about compatibility with Oracle. Simplifying of 
porting existed applications from Oracle to Postgres  may be the
main motivation of adding GTT to Postgres. And making them incompatible 
with Oracle will be very strange.



--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [Proposal] Global temporary tables

2020-01-25 Thread 曾文旌(义从)

Thank you for review patch.

> 2020年1月24日 下午4:20，Konstantin Knizhnik  写道：
> 
> 
> 
> On 23.01.2020 19:28, 曾文旌(义从) wrote:
>> 
>> I'm trying to improve this part of the implementation in 
>> global_temporary_table_v7-pg13.patch
>> Please check my patch and give me feedback.
>> 
>> 
>> Thanks
>> 
>> Wenjing
>> 
>> 
> 
> Below is my short review of the patch:
> 
> +/*
> + * For global temp table only
> + * use AccessExclusiveLock for ensure safety
> + */
> +{
> +{
> +"on_commit_delete_rows",
> +"global temp table on commit options",
> +RELOPT_KIND_HEAP | RELOPT_KIND_PARTITIONED,
> +ShareUpdateExclusiveLock
> +},
> +true
> +},
> 
> 
> The comment seems to be confusing: it says about AccessExclusiveLock but 
> actually uses ShareUpdateExclusiveLock.
There is a problem with the comment description, I will fix it.

> 
> -Assert(TransactionIdIsNormal(onerel->rd_rel->relfrozenxid));
> -Assert(MultiXactIdIsValid(onerel->rd_rel->relminmxid));
> +Assert((RELATION_IS_GLOBAL_TEMP(onerel) && onerel->rd_rel->relfrozenxid 
> == InvalidTransactionId) ||
> +(!RELATION_IS_GLOBAL_TEMP(onerel) && 
> TransactionIdIsNormal(onerel->rd_rel->relfrozenxid)));
> +Assert((RELATION_IS_GLOBAL_TEMP(onerel) && onerel->rd_rel->relminmxid == 
> InvalidMultiXactId) ||
> +(!RELATION_IS_GLOBAL_TEMP(onerel) && 
> MultiXactIdIsValid(onerel->rd_rel->relminmxid)));
>  
> It is actually equivalent to:
> 
> Assert(RELATION_IS_GLOBAL_TEMP(onerel) ^ 
> TransactionIdIsNormal(onerel->rd_rel->relfrozenxid);
> Assert(RELATION_IS_GLOBAL_TEMP(onerel) ^ 
> MultiXactIdIsValid(onerel->rd_rel->relminmxid));
Yes, Thank you for your points out, It's simpler.

> 
> +/* clean temp relation files */
> +if (max_active_gtt > 0)
> +RemovePgTempFiles();
> +
>  /*
>  
> I wonder why do we need some special check for GTT here.
> From my point of view cleanup at startup of local storage of temp tables 
> should be performed in the same way for local and global temp tables.
After oom kill, In autovacuum, the Isolated local temp table will be cleaned 
like orphan temporary tables. The definition of local temp table is deleted 
with the storage file. 
But GTT can not do that. So we have the this implementation in my patch.
If you have other solutions, please let me know.

> 
> 
> -new_rel_reltup->relfrozenxid = relfrozenxid;
> -new_rel_reltup->relminmxid = relminmxid;
> +/* global temp table not remember transaction info in catalog */
> +if (relpersistence == RELPERSISTENCE_GLOBAL_TEMP)
> +{
> +new_rel_reltup->relfrozenxid = InvalidTransactionId;
> +new_rel_reltup->relminmxid = InvalidMultiXactId;
> +}
> +else
> +{
> +new_rel_reltup->relfrozenxid = relfrozenxid;
> +new_rel_reltup->relminmxid = relminmxid;
> +}
> +
> 
> 
> Why do we need to do it for GTT?
> Did you check that there will be no problems with GTT in case of XID 
> wraparound?
> Right now if you create temp table and keep session open, then it will block 
> XID wraparound.
In my design
1 Because different sessions have different transaction information, I choose 
to store the transaction information of GTT in MyProc,not catalog.
2 About the XID wraparound problem, the reason is the design of the temp table 
storage(local temp table and global temp table) that makes it can not to do 
vacuum by autovacuum. 
It should be completely solve at the storage level.

> 
> +/* We allow to drop global temp table only this session use it */
> +if (RELATION_IS_GLOBAL_TEMP(rel))
> +{
> +if (is_other_backend_use_gtt(rel->rd_node))
> +elog(ERROR, "can not drop relation when other backend attached 
> this global temp table");
> +}
> +
> 
> Here we once again introduce incompatibility with normal (permanent) tables.
> Assume that DBA or programmer need to change format of GTT. But there are 
> some active sessions which have used this GTT sometime in the past.
> We will not be able to drop this GTT until all this sessions are terminated.
> I do not think that it is acceptable behaviour.
In fact, The dba can still complete the DDL of the GTT.
I've provided a set of functions for this case.
If the dba needs to modify a GTT A(or drop GTT or create index on GTT), he 
needs to do:
1 Use the pg_gtt_attached_pids view to list the pids for the session that is 
using the GTT A.
2 Use pg_terminate_backend(pid)terminate they except itself.
3 Do alter GTT A.

> 
> +LOCKMODElockmode = AccessExclusiveLock;
> +
> +/* truncate global temp table only need RowExclusiveLock */
> +if (get_rel_persistence(rid) == RELPERSISTENCE_GLOBAL_TEMP)
> +lockmode = RowExclusiveLock;
> 
> 
> What are the reasons of using RowExclusiveLock for GTT instead of 
> AccessExclusiveLock?
> Yes, GTT data is access only by one backend so no locking here seems to be 
> needed at all.
>

Re: [Proposal] Global temporary tables

2020-01-24 Thread Pavel Stehule

pá 24. 1. 2020 v 14:17 odesílatel Konstantin Knizhnik <
k.knizh...@postgrespro.ru> napsal:

>
>
> On 24.01.2020 15:15, Pavel Stehule wrote:
>
> You will see a effect of DDL in current session (where you did the
> change), all other sessions should to live without any any change do
> reconnect or to RESET connect
>
> Why? I found this requirement quit unnatural and contradicting to the
> behavior of normal tables.
> Actually one of motivation for adding global tempo tables to Postgres is
> to provide compatibility with Oracle.
> Although I know that Oracle design decisions were never considered as
> axioms by Postgres community,
> but ni case of GTT design I think that we should take in account Oracle
> approach.
> And GTT in Oracle behaves exactly as in my implementation:
>
> https://www.oracletutorial.com/oracle-basics/oracle-global-temporary-table/
>
> It is not clear from this documentation whether index created for GTT in
> one session can be used in another session which already has some data in
> this GTT.
> But I did experiment with install Oracle server and  can confirm that
> actually works in this way.
>
> So I do not understand why do we need to complicate our GTT implementation
> in order to prohibit useful functionality and introduce inconsistency
> between behavior of normal and global temp tables.
>
>
>
> I don't like 2 - when I do index on global temp table, I don't would to
> wait on indexing on all other sessions. These operations should be
> maximally independent.
>
>
> Nobody suggest to wait building index in all sessions.
> Indexes will be constructed on demand when session access this table.
> If session will no access this table at all, then index will never be
> constructed.
>

> Once again: logic of dealing with indexes in GTT is very simple.
> For normal tables, indexes are initialized at the tame when them are
> created.
> For GTT it is not true. We have to initialize index on demand when it is
> accessed first time in session.
>
> So it has to be handled in any way.
> The question is only whether we should allow creation of index for table
> already populated with some data?
> Actually doesn't require some additional efforts. We can use existed
> build_index function which initialize index and populates it with data.
> So the solution proposed for me is most natural, convenient and simplest
> solution at the same time. And compatible with Oracle.
>

I cannot to evaluate your proposal, and I am sure, so you know more about
this code.

There is a question if we can allow to build local temp index on global
temp table. It is different situation. When I work with global properties
personally I prefer total asynchronous implementation of any DDL operations
for other than current session. When it is true, then I have not any
objection. For me, good enough design of any DDL can be based on catalog
change without forcing to living tables.

I see following disadvantage of your proposal. See scenario

1. I have two sessions

A - small GTT with active owner
B - big GTT with some active application.

session A will do new index - it is fast, but if creating index is forced
on B on demand (when B was touched), then this operation have to wait after
index will be created.

So I afraid build a index on other sessions on GTT when GTT tables in other
sessions will not be empty.

Regards

Pavel



>
>
>
>
> Regards
>
> Pavel
>
>
>>
>>
>> --
>> Konstantin Knizhnik
>> Postgres Professional: http://www.postgrespro.com
>> The Russian Postgres Company
>>
>>
> --
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>

Re: [Proposal] Global temporary tables




On 24.01.2020 15:15, Pavel Stehule wrote:
You will see a effect of DDL in current session (where you did the 
change), all other sessions should to live without any any change do 
reconnect or to RESET connect


Why? I found this requirement quit unnatural and contradicting to the 
behavior of normal tables.
Actually one of motivation for adding global tempo tables to Postgres is 
to provide compatibility with Oracle.
Although I know that Oracle design decisions were never considered as  
axioms by Postgres community,
but ni case of GTT design I think that we should take in account Oracle 
approach.

And GTT in Oracle behaves exactly as in my implementation:

https://www.oracletutorial.com/oracle-basics/oracle-global-temporary-table/

It is not clear from this documentation whether index created for GTT in 
one session can be used in another session which already has some data 
in this GTT.
But I did experiment with install Oracle server and  can confirm that 
actually works in this way.


So I do not understand why do we need to complicate our GTT 
implementation in order to prohibit useful functionality and introduce 
inconsistency between behavior of normal and global temp tables.




I don't like 2 - when I do index on global temp table, I don't would 
to wait on indexing on all other sessions. These operations should be 
maximally independent.




Nobody suggest to wait building index in all sessions.
Indexes will be constructed on demand when session access this table.
If session will no access this table at all, then index will never be 
constructed.


Once again: logic of dealing with indexes in GTT is very simple.
For normal tables, indexes are initialized at the tame when them are 
created.
For GTT it is not true. We have to initialize index on demand when it is 
accessed first time in session.


So it has to be handled in any way.
The question is only whether we should allow creation of index for table 
already populated with some data?
Actually doesn't require some additional efforts. We can use existed 
build_index function which initialize index and populates it with data.
So the solution proposed for me is most natural, convenient and simplest 
solution at the same time. And compatible with Oracle.






Regards

Pavel



-- 
Konstantin Knizhnik

Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company



--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [Proposal] Global temporary tables

2020-01-24 Thread Pavel Stehule

pá 24. 1. 2020 v 10:43 odesílatel Konstantin Knizhnik <
k.knizh...@postgrespro.ru> napsal:

>
>
> On 24.01.2020 12:09, Pavel Stehule wrote:
>
>
>
> pá 24. 1. 2020 v 9:39 odesílatel Konstantin Knizhnik <
> k.knizh...@postgrespro.ru> napsal:
>
>>
>>
>> On 23.01.2020 23:47, Robert Haas wrote:
>> > On Sat, Jan 11, 2020 at 8:51 PM Tomas Vondra
>> >  wrote:
>> >> 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.
>> > +1.
>> >
>> >> 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.
>> > +1. I can't imagine that's a safe or sane thing to do.
>> >
>>
>> As far as you know there are two versions of GTT implementations now.
>> And we are going to merge them into single patch.
>> But there are some principle question concerning provided functionality
>> which has to be be discussed:
>> should we prohibit DDL on GTT if there are more than one sessions using
>> it. It includes creation/dropping indexes, dropping table, altering
>> table...
>>
>> If the answer is "yes", then the question whether to populate new
>> indexes with data is no relevant at all, because such situation will not
>> be possible.
>> But in this case we will get incompatible behavior with normal
>> (permanent) tables and it seems to be very inconvenient from DBA point
>> of view:
>> it will be necessary to enforce all clients to close their sessions to
>> perform some DDL manipulations with GTT.
>> Some DDLs will be very difficult to implement if GTT is used by more
>> than one backend, for example altering table schema.
>>
>> My current solution is to allow creation/droping index on GTT and
>> dropping table itself, while prohibit alter schema at all for GTT.
>> Wenjing's approach is to prohibit any DDL if GTT is used by more than
>> one backend.
>>
>
> When I create index on GTT in one session, then I don't expect creating
> same index in all other sessions that uses same GTT.
>
> But I can imagine to creating index on GTT enforces index in current
> session, and for other sessions this index will be invalid to end of
> session.
>
>
> So there are three possible alternatives:
>
> 1. Prohibit index creation of GTT when it used by more than once session.
> 2. Create index and populate them with data in all sessions using this GTT.
> 3. Create index only in current session and do not allow to use it in all
> other sessions already using this GTT (but definitely allow to use it in
> new sessions).
>
> 1 is Wenjing's approach, 2 - is my approach, 3 - is your suggestion :)
>
> I can construct the following table with pro/cons of each approach:
>
> Approach
> Compatibility with normal table
> User (DBA) friendly
> Complexity of implementation
> Consistency
> 1
> -
> 1: requires restart of all sessions to perform operation
> 2: requires global cache of GTT
> 3*: *no man, no problem
> 2
> +
> 3: if index is created then it is actually needed, isn't it? 1: use
> existed functionality to create index
> 2: if alter schema is prohibited
> 3
> -
> 2: requires restart of all sessions to use created index
> 3: requires some mechanism for prohibiting index created after first
> session access to GTT
> 1: can perform DDL but do no see effect of it
>
>
You will see a effect of DDL in current session (where you did the change),
all other sessions should to live without any any change do reconnect or to
RESET connect

I don't like 2 - when I do index on global temp table, I don't would to
wait on indexing on all other sessions. These operations should be
maximally independent.

Regards

Pavel


>
>
> --
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>

Re: [Proposal] Global temporary tables

On 24.01.2020 12:09, Pavel Stehule wrote:

pá 24. 1. 2020 v 9:39 odesílatel Konstantin Knizhnik 
mailto:k.knizh...@postgrespro.ru>> napsal:

On 23.01.2020 23:47, Robert Haas wrote:
> On Sat, Jan 11, 2020 at 8:51 PM Tomas Vondra
> mailto:tomas.von...@2ndquadrant.com>> wrote:
>> 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.
> +1.
>
>> 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.
> +1. I can't imagine that's a safe or sane thing to do.
>

As far as you know there are two versions of GTT implementations now.
And we are going to merge them into single patch.
But there are some principle question concerning provided
functionality
which has to be be discussed:
should we prohibit DDL on GTT if there are more than one sessions
using
it. It includes creation/dropping indexes, dropping table,
altering table...

If the answer is "yes", then the question whether to populate new
indexes with data is no relevant at all, because such situation
will not
be possible.
But in this case we will get incompatible behavior with normal
(permanent) tables and it seems to be very inconvenient from DBA
point
of view:
it will be necessary to enforce all clients to close their
sessions to
perform some DDL manipulations with GTT.
Some DDLs will be very difficult to implement if GTT is used by more
than one backend, for example altering table schema.

My current solution is to allow creation/droping index on GTT and
dropping table itself, while prohibit alter schema at all for GTT.
Wenjing's approach is to prohibit any DDL if GTT is used by more than
one backend.

When I create index on GTT in one session, then I don't expect 
creating same index in all other sessions that uses same GTT.

But I can imagine to creating index on GTT enforces index in current 
session, and for other sessions this index will be invalid to end of 
session.

So there are three possible alternatives:

1. Prohibit index creation of GTT when it used by more than once session.
2. Create index and populate them with data in all sessions using this GTT.
3. Create index only in current session and do not allow to use it in 
all other sessions already using this GTT (but definitely allow to use 
it in new sessions).

1 is Wenjing's approach, 2 - is my approach, 3 - is your suggestion :)

I can construct the following table with pro/cons of each approach:

Approach
Compatibility with normal table
User (DBA) friendly
Complexity of implementation
Consistency
1
-
1: requires restart of all sessions to perform operation
2: requires global cache of GTT
3/: /no man, no problem
2
+
	3: if index is created then it is actually needed, isn't it? 	1: use 
existed functionality to create index

2: if alter schema is prohibited
3
-
2: requires restart of all sessions to use created index
	3: requires some mechanism for prohibiting index created after first 
session access to GTT

1: can perform DDL but do no see effect of it

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [Proposal] Global temporary tables

2020-01-24 Thread Pavel Stehule

pá 24. 1. 2020 v 9:39 odesílatel Konstantin Knizhnik <
k.knizh...@postgrespro.ru> napsal:

>
>
> On 23.01.2020 23:47, Robert Haas wrote:
> > On Sat, Jan 11, 2020 at 8:51 PM Tomas Vondra
> >  wrote:
> >> 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.
> > +1.
> >
> >> 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.
> > +1. I can't imagine that's a safe or sane thing to do.
> >
>
> As far as you know there are two versions of GTT implementations now.
> And we are going to merge them into single patch.
> But there are some principle question concerning provided functionality
> which has to be be discussed:
> should we prohibit DDL on GTT if there are more than one sessions using
> it. It includes creation/dropping indexes, dropping table, altering
> table...
>
> If the answer is "yes", then the question whether to populate new
> indexes with data is no relevant at all, because such situation will not
> be possible.
> But in this case we will get incompatible behavior with normal
> (permanent) tables and it seems to be very inconvenient from DBA point
> of view:
> it will be necessary to enforce all clients to close their sessions to
> perform some DDL manipulations with GTT.
> Some DDLs will be very difficult to implement if GTT is used by more
> than one backend, for example altering table schema.
>
> My current solution is to allow creation/droping index on GTT and
> dropping table itself, while prohibit alter schema at all for GTT.
> Wenjing's approach is to prohibit any DDL if GTT is used by more than
> one backend.
>

When I create index on GTT in one session, then I don't expect creating
same index in all other sessions that uses same GTT.

But I can imagine to creating index on GTT enforces index in current
session, and for other sessions this index will be invalid to end of
session.

Regards

Pavel

>
> --
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>

Re: [Proposal] Global temporary tables





On 23.01.2020 23:47, Robert Haas wrote:

On Sat, Jan 11, 2020 at 8:51 PM Tomas Vondra
 wrote:

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.

+1.


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.

+1. I can't imagine that's a safe or sane thing to do.



As far as you know there are two versions of GTT implementations now.
And we are going to merge them into single patch.
But there are some principle question concerning provided functionality 
which has to be be discussed:
should we prohibit DDL on GTT if there are more than one sessions using 
it. It includes creation/dropping indexes, dropping table, altering table...


If the answer is "yes", then the question whether to populate new 
indexes with data is no relevant at all, because such situation will not 
be possible.
But in this case we will get incompatible behavior with normal 
(permanent) tables and it seems to be very inconvenient from DBA point 
of view:
it will be necessary to enforce all clients to close their sessions to 
perform some DDL manipulations with GTT.
Some DDLs will be very difficult to implement if GTT is used by more 
than one backend, for example altering table schema.


My current solution is to allow creation/droping index on GTT and 
dropping table itself, while prohibit alter schema at all for GTT.
Wenjing's approach is to prohibit any DDL if GTT is used by more than 
one backend.


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [Proposal] Global temporary tables




On 23.01.2020 19:28, 曾文旌(义从) wrote:


I'm trying to improve this part of the implementation in 
global_temporary_table_v7-pg13.patch

Please check my patch and give me feedback.


Thanks

Wenjing




Below is my short review of the patch:

+    /*
+     * For global temp table only
+     * use AccessExclusiveLock for ensure safety
+     */
+    {
+        {
+            "on_commit_delete_rows",
+            "global temp table on commit options",
+            RELOPT_KIND_HEAP | RELOPT_KIND_PARTITIONED,
+            ShareUpdateExclusiveLock
+        },
+        true
+    },


The comment seems to be confusing: it says about AccessExclusiveLock but 
actually uses ShareUpdateExclusiveLock.


- Assert(TransactionIdIsNormal(onerel->rd_rel->relfrozenxid));
-    Assert(MultiXactIdIsValid(onerel->rd_rel->relminmxid));
+    Assert((RELATION_IS_GLOBAL_TEMP(onerel) && 
onerel->rd_rel->relfrozenxid == InvalidTransactionId) ||
+        (!RELATION_IS_GLOBAL_TEMP(onerel) && 
TransactionIdIsNormal(onerel->rd_rel->relfrozenxid)));
+    Assert((RELATION_IS_GLOBAL_TEMP(onerel) && 
onerel->rd_rel->relminmxid == InvalidMultiXactId) ||
+        (!RELATION_IS_GLOBAL_TEMP(onerel) && 
MultiXactIdIsValid(onerel->rd_rel->relminmxid)));


It is actually equivalent to:

Assert(RELATION_IS_GLOBAL_TEMP(onerel) ^ 
TransactionIdIsNormal(onerel->rd_rel->relfrozenxid);
Assert(RELATION_IS_GLOBAL_TEMP(onerel) ^ 
MultiXactIdIsValid(onerel->rd_rel->relminmxid));


+    /* clean temp relation files */
+    if (max_active_gtt > 0)
+        RemovePgTempFiles();
+
 /*

I wonder why do we need some special check for GTT here.
From my point of view cleanup at startup of local storage of temp 
tables should be performed in the same way for local and global temp tables.



-    new_rel_reltup->relfrozenxid = relfrozenxid;
-    new_rel_reltup->relminmxid = relminmxid;
+    /* global temp table not remember transaction info in catalog */
+    if (relpersistence == RELPERSISTENCE_GLOBAL_TEMP)
+    {
+        new_rel_reltup->relfrozenxid = InvalidTransactionId;
+        new_rel_reltup->relminmxid = InvalidMultiXactId;
+    }
+    else
+    {
+        new_rel_reltup->relfrozenxid = relfrozenxid;
+        new_rel_reltup->relminmxid = relminmxid;
+    }
+


Why do we need to do it for GTT?
Did you check that there will be no problems with GTT in case of XID 
wraparound?
Right now if you create temp table and keep session open, then it will 
block XID wraparound.


+    /* We allow to drop global temp table only this session use it */
+    if (RELATION_IS_GLOBAL_TEMP(rel))
+    {
+        if (is_other_backend_use_gtt(rel->rd_node))
+            elog(ERROR, "can not drop relation when other backend 
attached this global temp table");

+    }
+

Here we once again introduce incompatibility with normal (permanent) tables.
Assume that DBA or programmer need to change format of GTT. But there 
are some active sessions which have used this GTT sometime in the past.

We will not be able to drop this GTT until all this sessions are terminated.
I do not think that it is acceptable behaviour.

+        LOCKMODE    lockmode = AccessExclusiveLock;
+
+        /* truncate global temp table only need RowExclusiveLock */
+        if (get_rel_persistence(rid) == RELPERSISTENCE_GLOBAL_TEMP)
+            lockmode = RowExclusiveLock;


What are the reasons of using RowExclusiveLock for GTT instead of 
AccessExclusiveLock?
Yes, GTT data is access only by one backend so no locking here seems to 
be needed at all.
But I wonder what are the motivations/benefits of using weaker lock 
level here?

There should be no conflicts in any case...

+        /* We allow to create index on global temp table only this 
session use it */

+        if (is_other_backend_use_gtt(heapRelation->rd_node))
+            elog(ERROR, "can not create index when have other backend 
attached this global temp table");

+

The same argument as in case of dropping GTT: I do not think that 
prohibiting DLL operations on GTT used by more than one backend is bad idea.


+    /* global temp table not support foreign key constraint yet */
+    if (RELATION_IS_GLOBAL_TEMP(pkrel))
+        ereport(ERROR,
+                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                 errmsg("referenced relation \"%s\" is not a global 
temp table",

+                        RelationGetRelationName(pkrel;
+

Why do we need to prohibit foreign key constraint on GTT?

+    /*
+     * Global temp table get frozenxid from MyProc
+     * to avoid the vacuum truncate clog that gtt need.
+     */
+    if (max_active_gtt > 0)
+    {
+        TransactionId oldest_gtt_frozenxid =
+            list_all_session_gtt_frozenxids(0, NULL, NULL, NULL);
+
+        if (TransactionIdIsNormal(oldest_gtt_frozenxid) &&
+            TransactionIdPrecedes(oldest_gtt_frozenxid, newFrozenXid))
+        {
+            ereport(WARNING,
+                (errmsg("global temp table oldest FrozenXid is far in 
the past"),
+

Re: [Proposal] Global temporary tables

2020-01-23 Thread Robert Haas

On Sat, Jan 11, 2020 at 8:51 PM Tomas Vondra
 wrote:
> 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.

+1.

> 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.

+1. I can't imagine that's a safe or sane thing to do.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [Proposal] Global temporary tables

2020-01-23 Thread Pavel Stehule

čt 23. 1. 2020 v 17:28 odesílatel 曾文旌(义从) 
napsal:

>
>
> 2020年1月22日 下午1:29，曾文旌(义从)  写道：
>
>
>
> 2020年1月21日 下午1:43，Pavel Stehule  写道：
>
> Hi
>
> I have a free time this evening, so I will check this patch
>
> I have a one question
>
> + /* global temp table get relstats from localhash */
> + if (RELATION_IS_GLOBAL_TEMP(rel))
> + {
> + get_gtt_relstats(RelationGetRelid(rel),
> + , , ,
> + NULL, NULL);
> + }
> + else
> + {
> + /* coerce values in pg_class to more desirable types */
> + relpages = (BlockNumber) rel->rd_rel->relpages;
> + reltuples = (double) rel->rd_rel->reltuples;
> + relallvisible = (BlockNumber) rel->rd_rel->relallvisible;
> + }
>
> Isbn't possible to fill the rd_rel structure too, so this branching can be
> reduced?
>
> I'll make some improvements to optimize this part of the code.
>
> I'm trying to improve this part of the implementation in
> global_temporary_table_v7-pg13.patch
> Please check my patch and give me feedback.
>
>
It is looking better, still there are some strange things (I didn't tested
functionality yet)

  elog(ERROR, "invalid relpersistence: %c",
  relation->rd_rel->relpersistence);
@@ -3313,6 +3336,10 @@ RelationBuildLocalRelation(const char *relname,
  rel->rd_backend = BackendIdForTempRelations();
  rel->rd_islocaltemp = true;
  break;
+ case RELPERSISTENCE_GLOBAL_TEMP:
+ rel->rd_backend = BackendIdForTempRelations();
+ rel->rd_islocaltemp = true;
+ break;
  default:

+ rel->rd_islocaltemp = true;  <<< if this is valid, then the name of
field "rd_islocaltemp" is not probably best



regards

Pavel




>
> Thanks
>
> Wenjing
>
>
>
>
>
> Regards
>
> Pavel
>
> po 20. 1. 2020 v 17:27 odesílatel 曾文旌(义从) 
> napsal:
>
>>
>>
>> > 2020年1月20日 上午1:32，Erik Rijkers  写道：
>> >
>> > On 2020-01-19 18:04, 曾文旌(义从) wrote:
>> >>> 2020年1月14日 下午9:20，Pavel Stehule  写道：
>> >>> út 14. 1. 2020 v 14:09 odesílatel 曾文旌(义从) <
>> wenjing@alibaba-inc.com > napsal:
>> >
>> >>> [global_temporary_table_v4-pg13.patch ]
>> >
>> > Hi,
>> >
>> > This patch doesn't quiet apply for me:
>> >
>> > patching file src/backend/access/common/reloptions.c
>> > patching file src/backend/access/gist/gistutil.c
>> > patching file src/backend/access/hash/hash.c
>> > Hunk #1 succeeded at 149 (offset 3 lines).
>> > patching file src/backend/access/heap/heapam_handler.c
>> > patching file src/backend/access/heap/vacuumlazy.c
>> > patching file src/backend/access/nbtree/nbtpage.c
>> > patching file src/backend/access/table/tableam.c
>> > patching file src/backend/access/transam/xlog.c
>> > patching file src/backend/catalog/Makefile
>> > Hunk #1 FAILED at 44.
>> > 1 out of 1 hunk FAILED -- saving rejects to file
>> src/backend/catalog/Makefile.rej
>> > [...]
>> >   (The rest applies without errors)
>> >
>> > src/backend/catalog/Makefile.rej contains:
>> >
>> > 
>> > --- src/backend/catalog/Makefile
>> > +++ src/backend/catalog/Makefile
>> > @@ -44,6 +44,8 @@ OBJS = \
>> >   storage.o \
>> >   toasting.o
>> >
>> > +OBJS += storage_gtt.o
>> > +
>> > BKIFILES = postgres.bki postgres.description postgres.shdescription
>> >
>> > include $(top_srcdir)/src/backend/common.mk
>> > 
>> >
>> > Can you have a look?
>> I updated the code and remade the patch.
>> Please give me feedback if you have any more questions.
>>
>>
>>
>>
>> >
>> >
>> > thanks,
>> >
>> > Erik Rijkers
>> >
>> >
>> >
>> >
>> >
>>
>>
>
>

Re: [Proposal] Global temporary tables

2020-01-21 Thread Pavel Stehule

st 22. 1. 2020 v 7:16 odesílatel 曾文旌(义从) 
napsal:

>
>
> 2020年1月22日 上午2:51，Pavel Stehule  写道：
>
>
>
> út 21. 1. 2020 v 9:46 odesílatel 曾文旌(义从) 
> napsal:
>
>>
>>
>> 2020年1月12日 上午4:27，Pavel Stehule  写道：
>>
>> Hi
>>
>> so 11. 1. 2020 v 15:00 odesílatel 曾文旌(义从) 
>> napsal:
>>
>>> Hi all
>>>
>>> This is the latest patch
>>>
>>> The updates are as follows:
>>> 1. Support global temp Inherit table global temp partition table
>>> 2. Support serial column in GTT
>>> 3. Provide views pg_gtt_relstats pg_gtt_stats for GTT’s statistics
>>> 4. Provide view pg_gtt_attached_pids to manage GTT
>>> 5. Provide function pg_list_gtt_relfrozenxids() to manage GTT
>>> 6. Alter GTT or rename GTT is allowed under some conditions
>>>
>>>
>>> Please give me feedback.
>>>
>>
>> I tested the functionality
>>
>> 1. i think so "ON COMMIT PRESERVE ROWS" should be default mode (like
>> local temp tables).
>>
>>
>> ON COMMIT PRESERVE ROWS is default mode now.
>>
>
> Thank you
>
> * I tried to create global temp table with index. When I tried to drop
> this table (and this table was used by second instance), then I got error
> message
>
> postgres=# drop table foo;
> ERROR:  can not drop index when other backend attached this global temp
> table
>
> It is expected, but it is not too much user friendly. Is better to check
> if you can drop table, then lock it, and then drop all objects.
>
> I don't understand what needs to be improved. Could you describe it in
> detail?
>

the error messages should be some like

can not drop table when other backend attached this global temp table.

It is little bit messy, when you try to drop table and you got message
about index


>
> * tab complete can be nice for CREATE GLOBAL TEMP table
>
> Yes, I will improve it.
>
>
> \dt+ \di+ doesn't work correctly, or maybe I don't understand to the
> implementation.
>
>
> postgres=# create table t(a int primary key);
> CREATE TABLE
> postgres=# create global temp table gt(a int primary key);
> CREATE TABLE
> postgres=# insert into t values(generate_series(1,1));
> INSERT 0 1
> postgres=# insert into gt values(generate_series(1,1));
> INSERT 0 1
>
> postgres=# \dt+
> List of relations
>  Schema | Name | Type  |Owner| Persistence |  Size  | Description
> +--+---+-+-++-
>  public | gt   | table | wenjing.zwj | session | 384 kB |
>  public | t| table | wenjing.zwj | permanent   | 384 kB |
> (2 rows)
>
> postgres=# \di+
>   List of relations
>  Schema |  Name   | Type  |Owner| Table | Persistence |  Size  |
> Description
>
> +-+---+-+---+-++-
>  public | gt_pkey | index | wenjing.zwj | gt| session | 240 kB |
>  public | t_pkey  | index | wenjing.zwj | t | permanent   | 240 kB |
> (2 rows)
>
>
> I see same size in all sessions. Global temp tables shares same files?
>
> No, they use their own files.
> But \dt+ \di+ counts the total file sizes in all sessions for each GTT.
>

I think so it is wrong. The data are independent and the sizes should be
independent too


>
>
> Wenjing
>
>
> Regards
>
> Pavel
>
>
>
>
>>
>> Wenjing
>>
>>
>>
>>
>> I tested some simple scripts
>>
>> test01.sql
>>
>> CREATE TEMP TABLE foo(a int, b int);
>> INSERT INTO foo SELECT random()*100, random()*1000 FROM
>> generate_series(1,1000);
>> ANALYZE foo;
>> SELECT sum(a), sum(b) FROM foo;
>> DROP TABLE foo; -- simulate disconnect
>>
>>
>> after 100 sec, the table pg_attribute has 3.2MB
>> and 64 tps, 6446 transaction
>>
>> test02.sql
>>
>> INSERT INTO foo SELECT random()*100, random()*1000 FROM
>> generate_series(1,1000);
>> ANALYZE foo;
>> SELECT sum(a), sum(b) FROM foo;
>> DELETE FROM foo; -- simulate disconnect
>>
>>
>> after 100 sec, 1688 tps, 168830 transactions
>>
>> So performance is absolutely different as we expected.
>>
>> From my perspective, this functionality is great.
>>
>> Todo:
>>
>> pg_table_size function doesn't work
>>
>> Regards
>>
>> Pavel
>>
>>
>>> Wenjing
>>>
>>>
>>>
>>>
>>>
>>> 2020年1月6日 上午4:06，Tomas Vondra  写道：
>>&g

Re: [Proposal] Global temporary tables

2020-01-21 Thread 曾文旌(义从)



> 2020年1月22日 上午2:51，Pavel Stehule  写道：
> 
> 
> 
> út 21. 1. 2020 v 9:46 odesílatel 曾文旌(义从)  <mailto:wenjing@alibaba-inc.com>> napsal:
> 
> 
>> 2020年1月12日 上午4:27，Pavel Stehule > <mailto:pavel.steh...@gmail.com>> 写道：
>> 
>> Hi
>> 
>> so 11. 1. 2020 v 15:00 odesílatel 曾文旌(义从) > <mailto:wenjing@alibaba-inc.com>> napsal:
>> Hi all
>> 
>> This is the latest patch
>> 
>> The updates are as follows:
>> 1. Support global temp Inherit table global temp partition table
>> 2. Support serial column in GTT
>> 3. Provide views pg_gtt_relstats pg_gtt_stats for GTT’s statistics
>> 4. Provide view pg_gtt_attached_pids to manage GTT
>> 5. Provide function pg_list_gtt_relfrozenxids() to manage GTT
>> 6. Alter GTT or rename GTT is allowed under some conditions
>> 
>> 
>> Please give me feedback.
>> 
>> I tested the functionality
>> 
>> 1. i think so "ON COMMIT PRESERVE ROWS" should be default mode (like local 
>> temp tables).
> 
> ON COMMIT PRESERVE ROWS is default mode now.
> 
> Thank you
> 
> * I tried to create global temp table with index. When I tried to drop this 
> table (and this table was used by second instance), then I got error message
> 
> postgres=# drop table foo;
> ERROR:  can not drop index when other backend attached this global temp table
> 
> It is expected, but it is not too much user friendly. Is better to check if 
> you can drop table, then lock it, and then drop all objects.
I don't understand what needs to be improved. Could you describe it in detail?

> 
> * tab complete can be nice for CREATE GLOBAL TEMP table
Yes, I will improve it.
> 
> \dt+ \di+ doesn't work correctly, or maybe I don't understand to the 
> implementation.
> 

postgres=# create table t(a int primary key);
CREATE TABLE
postgres=# create global temp table gt(a int primary key);
CREATE TABLE
postgres=# insert into t values(generate_series(1,1));
INSERT 0 1
postgres=# insert into gt values(generate_series(1,1));
INSERT 0 1

postgres=# \dt+
List of relations
 Schema | Name | Type  |Owner| Persistence |  Size  | Description 
+--+---+-+-++-
 public | gt   | table | wenjing.zwj | session | 384 kB | 
 public | t| table | wenjing.zwj | permanent   | 384 kB | 
(2 rows)

postgres=# \di+
  List of relations
 Schema |  Name   | Type  |Owner| Table | Persistence |  Size  | 
Description 
+-+---+-+---+-++-
 public | gt_pkey | index | wenjing.zwj | gt| session | 240 kB | 
 public | t_pkey  | index | wenjing.zwj | t | permanent   | 240 kB | 
(2 rows)


> I see same size in all sessions. Global temp tables shares same files?
No, they use their own files.
But \dt+ \di+ counts the total file sizes in all sessions for each GTT.



Wenjing

> 
> Regards
> 
> Pavel
> 
> 
> 
> 
> 
> Wenjing
> 
> 
> 
>> 
>> I tested some simple scripts 
>> 
>> test01.sql
>> 
>> CREATE TEMP TABLE foo(a int, b int);
>> INSERT INTO foo SELECT random()*100, random()*1000 FROM 
>> generate_series(1,1000);
>> ANALYZE foo;
>> SELECT sum(a), sum(b) FROM foo;
>> DROP TABLE foo; -- simulate disconnect
>> 
>> 
>> after 100 sec, the table pg_attribute has 3.2MB
>> and 64 tps, 6446 transaction
>> 
>> test02.sql
>> 
>> INSERT INTO foo SELECT random()*100, random()*1000 FROM 
>> generate_series(1,1000);
>> ANALYZE foo;
>> SELECT sum(a), sum(b) FROM foo;
>> DELETE FROM foo; -- simulate disconnect
>> 
>> 
>> after 100 sec, 1688 tps, 168830 transactions
>> 
>> So performance is absolutely different as we expected.
>> 
>> From my perspective, this functionality is great.
>> 
>> Todo:
>> 
>> pg_table_size function doesn't work
>> 
>> Regards
>> 
>> Pavel
>> 
>> 
>> Wenjing
>> 
>> 
>> 
>> 
>> 
>>> 2020年1月6日 上午4:06，Tomas Vondra >> <mailto:tomas.von...@2ndquadrant.com>> 写道：
>>> 
>>> Hi,
>>> 
>>> I think we need to do something with having two patches aiming to add
>>> global temporary tables:
>>> 
>>> [1] https://commitfest.postgresql.org/26/2349/ 
>>> <https://commitfest.postgresql.org/26/2349/>
>>> 
>>> [2] https://commitfest.postgresql.org/26/2233/ 
>>> <https://commitfest.postgresql.org/26/2233/>
>>> 
>>> As a reviewer I ha

Re: [Proposal] Global temporary tables

2020-01-21 Thread 曾文旌(义从)



> 2020年1月21日 下午1:43，Pavel Stehule  写道：
> 
> Hi
> 
> I have a free time this evening, so I will check this patch
> 
> I have a one question
> 
> + /* global temp table get relstats from localhash */
> + if (RELATION_IS_GLOBAL_TEMP(rel))
> + {
> + get_gtt_relstats(RelationGetRelid(rel),
> + , , ,
> + NULL, NULL);
> + }
> + else
> + {
> + /* coerce values in pg_class to more desirable types */
> + relpages = (BlockNumber) rel->rd_rel->relpages;
> + reltuples = (double) rel->rd_rel->reltuples;
> + relallvisible = (BlockNumber) rel->rd_rel->relallvisible;
> + }
> 
> Isbn't possible to fill the rd_rel structure too, so this branching can be 
> reduced?
I'll make some improvements to optimize this part of the code.

> 
> Regards
> 
> Pavel
> 
> po 20. 1. 2020 v 17:27 odesílatel 曾文旌(义从)  > napsal:
> 
> 
> > 2020年1月20日 上午1:32，Erik Rijkers mailto:e...@xs4all.nl>> 写道：
> > 
> > On 2020-01-19 18:04, 曾文旌(义从) wrote:
> >>> 2020年1月14日 下午9:20，Pavel Stehule  >>> > 写道：
> >>> út 14. 1. 2020 v 14:09 odesílatel 曾文旌(义从)  >>>   >>> >> napsal:
> > 
> >>> [global_temporary_table_v4-pg13.patch ]
> > 
> > Hi,
> > 
> > This patch doesn't quiet apply for me:
> > 
> > patching file src/backend/access/common/reloptions.c
> > patching file src/backend/access/gist/gistutil.c
> > patching file src/backend/access/hash/hash.c
> > Hunk #1 succeeded at 149 (offset 3 lines).
> > patching file src/backend/access/heap/heapam_handler.c
> > patching file src/backend/access/heap/vacuumlazy.c
> > patching file src/backend/access/nbtree/nbtpage.c
> > patching file src/backend/access/table/tableam.c
> > patching file src/backend/access/transam/xlog.c
> > patching file src/backend/catalog/Makefile
> > Hunk #1 FAILED at 44.
> > 1 out of 1 hunk FAILED -- saving rejects to file 
> > src/backend/catalog/Makefile.rej
> > [...]
> >   (The rest applies without errors)
> > 
> > src/backend/catalog/Makefile.rej contains:
> > 
> > 
> > --- src/backend/catalog/Makefile
> > +++ src/backend/catalog/Makefile
> > @@ -44,6 +44,8 @@ OBJS = \
> >   storage.o \
> >   toasting.o
> > 
> > +OBJS += storage_gtt.o
> > +
> > BKIFILES = postgres.bki postgres.description postgres.shdescription
> > 
> > include $(top_srcdir)/src/backend/common.mk 
> > 
> > 
> > Can you have a look?
> I updated the code and remade the patch.
> Please give me feedback if you have any more questions.
> 
> 
> 
> 
> > 
> > 
> > thanks,
> > 
> > Erik Rijkers
> > 
> > 
> > 
> > 
> > 
>

Re: [Proposal] Global temporary tables

2020-01-21 Thread Pavel Stehule

út 21. 1. 2020 v 9:46 odesílatel 曾文旌(义从) 
napsal:

>
>
> 2020年1月12日 上午4:27，Pavel Stehule  写道：
>
> Hi
>
> so 11. 1. 2020 v 15:00 odesílatel 曾文旌(义从) 
> napsal:
>
>> Hi all
>>
>> This is the latest patch
>>
>> The updates are as follows:
>> 1. Support global temp Inherit table global temp partition table
>> 2. Support serial column in GTT
>> 3. Provide views pg_gtt_relstats pg_gtt_stats for GTT’s statistics
>> 4. Provide view pg_gtt_attached_pids to manage GTT
>> 5. Provide function pg_list_gtt_relfrozenxids() to manage GTT
>> 6. Alter GTT or rename GTT is allowed under some conditions
>>
>>
>> Please give me feedback.
>>
>
> I tested the functionality
>
> 1. i think so "ON COMMIT PRESERVE ROWS" should be default mode (like local
> temp tables).
>
>
> ON COMMIT PRESERVE ROWS is default mode now.
>

Thank you

* I tried to create global temp table with index. When I tried to drop this
table (and this table was used by second instance), then I got error message

postgres=# drop table foo;
ERROR:  can not drop index when other backend attached this global temp
table

It is expected, but it is not too much user friendly. Is better to check if
you can drop table, then lock it, and then drop all objects.

* tab complete can be nice for CREATE GLOBAL TEMP table

\dt+ \di+ doesn't work correctly, or maybe I don't understand to the
implementation.

I see same size in all sessions. Global temp tables shares same files?

Regards

Pavel




>
> Wenjing
>
>
>
>
> I tested some simple scripts
>
> test01.sql
>
> CREATE TEMP TABLE foo(a int, b int);
> INSERT INTO foo SELECT random()*100, random()*1000 FROM
> generate_series(1,1000);
> ANALYZE foo;
> SELECT sum(a), sum(b) FROM foo;
> DROP TABLE foo; -- simulate disconnect
>
>
> after 100 sec, the table pg_attribute has 3.2MB
> and 64 tps, 6446 transaction
>
> test02.sql
>
> INSERT INTO foo SELECT random()*100, random()*1000 FROM
> generate_series(1,1000);
> ANALYZE foo;
> SELECT sum(a), sum(b) FROM foo;
> DELETE FROM foo; -- simulate disconnect
>
>
> after 100 sec, 1688 tps, 168830 transactions
>
> So performance is absolutely different as we expected.
>
> From my perspective, this functionality is great.
>
> Todo:
>
> pg_table_size function doesn't work
>
> Regards
>
> Pavel
>
>
>> Wenjing
>>
>>
>>
>>
>>
>> 2020年1月6日 上午4:06，Tomas Vondra  写道：
>>
>> Hi,
>>
>> I think we need to do something with having two patches aiming to add
>> global temporary tables:
>>
>> [1] https://commitfest.postgresql.org/26/2349/
>>
>> [2] https://commitfest.postgresql.org/26/2233/
>>
>> As a reviewer I have no idea which of the threads to look at - certainly
>> not without reading both threads, which I doubt anyone will really do.
>> The reviews and discussions are somewhat intermixed between those two
>> threads, which makes it even more confusing.
>>
>> I think we should agree on a minimal patch combining the necessary/good
>> bits from the various patches, and terminate one of the threads (i.e.
>> mark it as rejected or RWF). And we need to do that now, otherwise
>> there's about 0% chance of getting this into v13.
>>
>> In general, I agree with the sentiment Rober expressed in [1] - the
>> patch needs to be as small as possible, not adding "nice to have"
>> features (like support for parallel queries - I very much doubt just
>> using shared instead of local buffers is enough to make it work.)
>>
>> regards
>>
>> --
>> Tomas Vondra  http://www.2ndQuadrant.com
>> <http://www.2ndquadrant.com/>
>> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>
>>
>>
>

Re: [Proposal] Global temporary tables

2020-01-20 Thread Pavel Stehule

Hi

I have a free time this evening, so I will check this patch

I have a one question

+ /* global temp table get relstats from localhash */
+ if (RELATION_IS_GLOBAL_TEMP(rel))
+ {
+ get_gtt_relstats(RelationGetRelid(rel),
+ , , ,
+ NULL, NULL);
+ }
+ else
+ {
+ /* coerce values in pg_class to more desirable types */
+ relpages = (BlockNumber) rel->rd_rel->relpages;
+ reltuples = (double) rel->rd_rel->reltuples;
+ relallvisible = (BlockNumber) rel->rd_rel->relallvisible;
+ }

Isbn't possible to fill the rd_rel structure too, so this branching can be
reduced?

Regards

Pavel

po 20. 1. 2020 v 17:27 odesílatel 曾文旌(义从) 
napsal:

>
>
> > 2020年1月20日 上午1:32，Erik Rijkers  写道：
> >
> > On 2020-01-19 18:04, 曾文旌(义从) wrote:
> >>> 2020年1月14日 下午9:20，Pavel Stehule  写道：
> >>> út 14. 1. 2020 v 14:09 odesílatel 曾文旌(义从)  > napsal:
> >
> >>> [global_temporary_table_v4-pg13.patch ]
> >
> > Hi,
> >
> > This patch doesn't quiet apply for me:
> >
> > patching file src/backend/access/common/reloptions.c
> > patching file src/backend/access/gist/gistutil.c
> > patching file src/backend/access/hash/hash.c
> > Hunk #1 succeeded at 149 (offset 3 lines).
> > patching file src/backend/access/heap/heapam_handler.c
> > patching file src/backend/access/heap/vacuumlazy.c
> > patching file src/backend/access/nbtree/nbtpage.c
> > patching file src/backend/access/table/tableam.c
> > patching file src/backend/access/transam/xlog.c
> > patching file src/backend/catalog/Makefile
> > Hunk #1 FAILED at 44.
> > 1 out of 1 hunk FAILED -- saving rejects to file
> src/backend/catalog/Makefile.rej
> > [...]
> >   (The rest applies without errors)
> >
> > src/backend/catalog/Makefile.rej contains:
> >
> > 
> > --- src/backend/catalog/Makefile
> > +++ src/backend/catalog/Makefile
> > @@ -44,6 +44,8 @@ OBJS = \
> >   storage.o \
> >   toasting.o
> >
> > +OBJS += storage_gtt.o
> > +
> > BKIFILES = postgres.bki postgres.description postgres.shdescription
> >
> > include $(top_srcdir)/src/backend/common.mk
> > 
> >
> > Can you have a look?
> I updated the code and remade the patch.
> Please give me feedback if you have any more questions.
>
>
>
>
> >
> >
> > thanks,
> >
> > Erik Rijkers
> >
> >
> >
> >
> >
>
>

Re: [Proposal] Global temporary tables

2020-01-19 Thread Erik Rijkers


On 2020-01-19 18:04, 曾文旌(义从) wrote:

2020年1月14日 下午9:20，Pavel Stehule  写道：
út 14. 1. 2020 v 14:09 odesílatel 曾文旌(义从) > napsal:



[global_temporary_table_v4-pg13.patch ]


Hi,

This patch doesn't quiet apply for me:

patching file src/backend/access/common/reloptions.c
patching file src/backend/access/gist/gistutil.c
patching file src/backend/access/hash/hash.c
Hunk #1 succeeded at 149 (offset 3 lines).
patching file src/backend/access/heap/heapam_handler.c
patching file src/backend/access/heap/vacuumlazy.c
patching file src/backend/access/nbtree/nbtpage.c
patching file src/backend/access/table/tableam.c
patching file src/backend/access/transam/xlog.c
patching file src/backend/catalog/Makefile
Hunk #1 FAILED at 44.
1 out of 1 hunk FAILED -- saving rejects to file 
src/backend/catalog/Makefile.rej

[...]
   (The rest applies without errors)

src/backend/catalog/Makefile.rej contains:


--- src/backend/catalog/Makefile
+++ src/backend/catalog/Makefile
@@ -44,6 +44,8 @@ OBJS = \
storage.o \
toasting.o

+OBJS += storage_gtt.o
+
 BKIFILES = postgres.bki postgres.description postgres.shdescription

 include $(top_srcdir)/src/backend/common.mk


Can you have a look?


thanks,

Erik Rijkers

Re: [Proposal] Global temporary tables

2020-01-15 Thread Konstantin Knizhnik





On 15.01.2020 16:10, 曾文旌(义从) wrote:



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 think the problem is that when one session completes the creation of the 
index on GTT,
it will trigger the other sessions build own local index of GTT in a 
centralized time.
This will consume a lot of hardware resources (cpu io memory) in a short time,
and even the database service becomes slow, because 50 sessions are building 
index.
I think this is not what we expected.



First of all creating index for GTT ni one session doesn't immediately 
initiate building indexes in all other sessions.
Indexes are built on demand. If session is not using this GTT any more, 
then index for it will not build at all.
And if GTT is really are actively used by all sessions, then building 
index and using it for constructing optimal execution plan is better,

then continue to  use sequential scan and read all GTT data from the disk.

And as I already mentioned I do not see some principle difference in 
aspect of resource consumptions comparing with current usage of local 
temp tables.
If we have have many sessions, each creating temp table, populating it 
with data and building index for it, then we will
observe the same CPU utilization and memory resource consumption as in 
case of using GTT and creating index for it.


Sorry, but I still not convinced by your and Tomas arguments.
Yes, building GTT index may cause high memory consumption 
(maintenance_work_mem * n_backends).
But such consumption can be  observed also without GTT and it has to be 
taken in account when choosing value for maintenance_work_mem.
But from my point of view it is much more important to make behavior of 
GTT as much compatible with normal tables as possible.
Also from database administration point of view, necessity to restart 
sessions to make then use new indexes seems to be very strange and 
inconvenient.
Alternatively DBA can address the problem with high memory consumption 
by adjusting maintenance_work_mem, so this solution is more flexible.




--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [Proposal] Global temporary tables

2020-01-15 Thread 曾文旌(义从)




> 2020年1月13日 下午4:08，Konstantin Knizhnik  写道：
> 
> 
> 
> 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.
> 
>> 
>> 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.
>> 
>> 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

Re: [Proposal] Global temporary tables

2020-01-14 Thread 曾文旌(义从)



> 2020年1月14日 下午9:20，Pavel Stehule  写道：
> 
> 
> 
> út 14. 1. 2020 v 14:09 odesílatel 曾文旌(义从)  <mailto:wenjing@alibaba-inc.com>> napsal:
> Thank you for review my patch.
> 
> 
>> 2020年1月12日 上午4:27，Pavel Stehule > <mailto:pavel.steh...@gmail.com>> 写道：
>> 
>> Hi
>> 
>> so 11. 1. 2020 v 15:00 odesílatel 曾文旌(义从) > <mailto:wenjing@alibaba-inc.com>> napsal:
>> Hi all
>> 
>> This is the latest patch
>> 
>> The updates are as follows:
>> 1. Support global temp Inherit table global temp partition table
>> 2. Support serial column in GTT
>> 3. Provide views pg_gtt_relstats pg_gtt_stats for GTT’s statistics
>> 4. Provide view pg_gtt_attached_pids to manage GTT
>> 5. Provide function pg_list_gtt_relfrozenxids() to manage GTT
>> 6. Alter GTT or rename GTT is allowed under some conditions
>> 
>> 
>> Please give me feedback.
>> 
>> I tested the functionality
>> 
>> 1. i think so "ON COMMIT PRESERVE ROWS" should be default mode (like local 
>> temp tables).
> makes sense, I will fix it.
> 
>> 
>> I tested some simple scripts 
>> 
>> test01.sql
>> 
>> CREATE TEMP TABLE foo(a int, b int);
>> INSERT INTO foo SELECT random()*100, random()*1000 FROM 
>> generate_series(1,1000);
>> ANALYZE foo;
>> SELECT sum(a), sum(b) FROM foo;
>> DROP TABLE foo; -- simulate disconnect
>> 
>> 
>> after 100 sec, the table pg_attribute has 3.2MB
>> and 64 tps, 6446 transaction
>> 
>> test02.sql
>> 
>> INSERT INTO foo SELECT random()*100, random()*1000 FROM 
>> generate_series(1,1000);
>> ANALYZE foo;
>> SELECT sum(a), sum(b) FROM foo;
>> DELETE FROM foo; -- simulate disconnect
>> 
>> 
>> after 100 sec, 1688 tps, 168830 transactions
>> 
>> So performance is absolutely different as we expected.
>> 
>> From my perspective, this functionality is great.
> Yes, frequent ddl causes catalog bloat, GTT avoids this problem.
> 
>> 
>> Todo:
>> 
>> pg_table_size function doesn't work
> Do you mean that function pg_table_size() need get the storage space used by 
> the one GTT in the entire db(include all session) .
> 
> It's question how much GTT tables should be similar to classic tables. But 
> the reporting in psql should to work \dt+, \l+, \di+
Get it, I will fix it.
> 
> 
> 
>> 
>> Regards
>> 
>> Pavel
>> 
>> 
>> Wenjing
>> 
>> 
>> 
>> 
>> 
>>> 2020年1月6日 上午4:06，Tomas Vondra >> <mailto:tomas.von...@2ndquadrant.com>> 写道：
>>> 
>>> Hi,
>>> 
>>> I think we need to do something with having two patches aiming to add
>>> global temporary tables:
>>> 
>>> [1] https://commitfest.postgresql.org/26/2349/ 
>>> <https://commitfest.postgresql.org/26/2349/>
>>> 
>>> [2] https://commitfest.postgresql.org/26/2233/ 
>>> <https://commitfest.postgresql.org/26/2233/>
>>> 
>>> As a reviewer I have no idea which of the threads to look at - certainly
>>> not without reading both threads, which I doubt anyone will really do.
>>> The reviews and discussions are somewhat intermixed between those two
>>> threads, which makes it even more confusing.
>>> 
>>> I think we should agree on a minimal patch combining the necessary/good
>>> bits from the various patches, and terminate one of the threads (i.e.
>>> mark it as rejected or RWF). And we need to do that now, otherwise
>>> there's about 0% chance of getting this into v13.
>>> 
>>> In general, I agree with the sentiment Rober expressed in [1] - the
>>> patch needs to be as small as possible, not adding "nice to have"
>>> features (like support for parallel queries - I very much doubt just
>>> using shared instead of local buffers is enough to make it work.)
>>> 
>>> regards
>>> 
>>> -- 
>>> Tomas Vondra  http://www.2ndQuadrant.com 
>>> <http://www.2ndquadrant.com/>
>>> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>> 
>

Re: [Proposal] Global temporary tables

2020-01-14 Thread 曾文旌(义从)

> 2020年1月12日 上午9:14，Tomas Vondra  写道：
> 
> On Fri, Jan 10, 2020 at 03:24:34PM +0300, Konstantin Knizhnik wrote:
>> 
>> 
>> On 09.01.2020 19:30, Tomas Vondra wrote:
>> 
>> 
>>> 

> 
>> 3 Still no one commented on GTT's transaction information processing, 
>> they include
>> 3.1 Should gtt's frozenxid need to be care?
>> 3.2 gtt’s clog clean
>> 3.3 How to deal with "too old" gtt data
>> 
> 
> No idea what to do about this.
> 

 I wonder what is the specific of GTT here?
 The same problem takes place for normal (local) temp tables, doesn't it?

>>> 
>>> Not sure. TBH I'm not sure I understand what the issue actually is.
>> 
>> Just open session, create temporary table and insert some data in it.
>> Then in other session run 2^31 transactions (at my desktop it takes about 2 
>> hours).
>> As far as temp tables are not proceeded by vacuum, database is stalled:
>> 
>>  ERROR:  database is not accepting commands to avoid wraparound data loss in 
>> database "postgres"
>> 
>> It seems to be quite dubious behavior and it is strange to me that nobody 
>> complains about it.
>> We discuss  many issues related with temp tables (statistic, parallel 
>> queries,...) which seems to be less critical.
>> 
>> But this problem is not specific to GTT - it can be reproduced with normal 
>> (local) temp tables.
>> This is why I wonder why do we need to solve it in GTT patch.
>> 
> 
> Yeah, I think that's out of scope for GTT patch. Once we solve it for
> plain temporary tables, we'll solve it for GTT too.
1. The core problem is that the data contains transaction information (xid), 
which needs to be vacuum(freeze) regularly to avoid running out of xid.
The autovacuum supports vacuum regular table but local temp does not. 
autovacuum also does not support GTT.

2. However, the difference between the local temp table and the global temp 
table(GTT) is that
a) For local temp table: one table hava one piece of data. the frozenxid of one 
local temp table is store in the catalog(pg_class). 
b) For global temp table: each session has a separate copy of data, one GTT may 
contain maxbackend frozenxid.
and I don't think it's a good idea to keep frozenxid of GTT in the 
catalog(pg_class). 
It becomes a question: how to handle GTT transaction information?

I agree that problem 1 should be completely solved by a some feature, such as 
local transactions. It is definitely not included in the GTT patch.

But, I think we need to ensure the durability of GTT data. For example, data in 
GTT cannot be lost due to the clog being cleaned up. It belongs to problem 2.

Wenjing

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

Re: [Proposal] Global temporary tables

2020-01-14 Thread Pavel Stehule

út 14. 1. 2020 v 14:09 odesílatel 曾文旌(义从) 
napsal:

> Thank you for review my patch.
>
>
> 2020年1月12日 上午4:27，Pavel Stehule  写道：
>
> Hi
>
> so 11. 1. 2020 v 15:00 odesílatel 曾文旌(义从) 
> napsal:
>
>> Hi all
>>
>> This is the latest patch
>>
>> The updates are as follows:
>> 1. Support global temp Inherit table global temp partition table
>> 2. Support serial column in GTT
>> 3. Provide views pg_gtt_relstats pg_gtt_stats for GTT’s statistics
>> 4. Provide view pg_gtt_attached_pids to manage GTT
>> 5. Provide function pg_list_gtt_relfrozenxids() to manage GTT
>> 6. Alter GTT or rename GTT is allowed under some conditions
>>
>>
>> Please give me feedback.
>>
>
> I tested the functionality
>
> 1. i think so "ON COMMIT PRESERVE ROWS" should be default mode (like local
> temp tables).
>
> makes sense, I will fix it.
>
>
> I tested some simple scripts
>
> test01.sql
>
> CREATE TEMP TABLE foo(a int, b int);
> INSERT INTO foo SELECT random()*100, random()*1000 FROM
> generate_series(1,1000);
> ANALYZE foo;
> SELECT sum(a), sum(b) FROM foo;
> DROP TABLE foo; -- simulate disconnect
>
>
> after 100 sec, the table pg_attribute has 3.2MB
> and 64 tps, 6446 transaction
>
> test02.sql
>
> INSERT INTO foo SELECT random()*100, random()*1000 FROM
> generate_series(1,1000);
> ANALYZE foo;
> SELECT sum(a), sum(b) FROM foo;
> DELETE FROM foo; -- simulate disconnect
>
>
> after 100 sec, 1688 tps, 168830 transactions
>
> So performance is absolutely different as we expected.
>
> From my perspective, this functionality is great.
>
> Yes, frequent ddl causes catalog bloat, GTT avoids this problem.
>
>
> Todo:
>
> pg_table_size function doesn't work
>
> Do you mean that function pg_table_size() need get the storage space used
> by the one GTT in the entire db(include all session) .
>

It's question how much GTT tables should be similar to classic tables. But
the reporting in psql should to work \dt+, \l+, \di+



>
> Regards
>
> Pavel
>
>
>> Wenjing
>>
>>
>>
>>
>>
>> 2020年1月6日 上午4:06，Tomas Vondra  写道：
>>
>> Hi,
>>
>> I think we need to do something with having two patches aiming to add
>> global temporary tables:
>>
>> [1] https://commitfest.postgresql.org/26/2349/
>>
>> [2] https://commitfest.postgresql.org/26/2233/
>>
>> As a reviewer I have no idea which of the threads to look at - certainly
>> not without reading both threads, which I doubt anyone will really do.
>> The reviews and discussions are somewhat intermixed between those two
>> threads, which makes it even more confusing.
>>
>> I think we should agree on a minimal patch combining the necessary/good
>> bits from the various patches, and terminate one of the threads (i.e.
>> mark it as rejected or RWF). And we need to do that now, otherwise
>> there's about 0% chance of getting this into v13.
>>
>> In general, I agree with the sentiment Rober expressed in [1] - the
>> patch needs to be as small as possible, not adding "nice to have"
>> features (like support for parallel queries - I very much doubt just
>> using shared instead of local buffers is enough to make it work.)
>>
>> regards
>>
>> --
>> Tomas Vondra  http://www.2ndQuadrant.com
>> <http://www.2ndquadrant.com/>
>> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>
>>
>>
>

Re: [Proposal] Global temporary tables

2020-01-14 Thread 曾文旌(义从)

Thank you for review my patch.


> 2020年1月12日 上午4:27，Pavel Stehule  写道：
> 
> Hi
> 
> so 11. 1. 2020 v 15:00 odesílatel 曾文旌(义从)  <mailto:wenjing@alibaba-inc.com>> napsal:
> Hi all
> 
> This is the latest patch
> 
> The updates are as follows:
> 1. Support global temp Inherit table global temp partition table
> 2. Support serial column in GTT
> 3. Provide views pg_gtt_relstats pg_gtt_stats for GTT’s statistics
> 4. Provide view pg_gtt_attached_pids to manage GTT
> 5. Provide function pg_list_gtt_relfrozenxids() to manage GTT
> 6. Alter GTT or rename GTT is allowed under some conditions
> 
> 
> Please give me feedback.
> 
> I tested the functionality
> 
> 1. i think so "ON COMMIT PRESERVE ROWS" should be default mode (like local 
> temp tables).
makes sense, I will fix it.

> 
> I tested some simple scripts 
> 
> test01.sql
> 
> CREATE TEMP TABLE foo(a int, b int);
> INSERT INTO foo SELECT random()*100, random()*1000 FROM 
> generate_series(1,1000);
> ANALYZE foo;
> SELECT sum(a), sum(b) FROM foo;
> DROP TABLE foo; -- simulate disconnect
> 
> 
> after 100 sec, the table pg_attribute has 3.2MB
> and 64 tps, 6446 transaction
> 
> test02.sql
> 
> INSERT INTO foo SELECT random()*100, random()*1000 FROM 
> generate_series(1,1000);
> ANALYZE foo;
> SELECT sum(a), sum(b) FROM foo;
> DELETE FROM foo; -- simulate disconnect
> 
> 
> after 100 sec, 1688 tps, 168830 transactions
> 
> So performance is absolutely different as we expected.
> 
> From my perspective, this functionality is great.
Yes, frequent ddl causes catalog bloat, GTT avoids this problem.

> 
> Todo:
> 
> pg_table_size function doesn't work
Do you mean that function pg_table_size() need get the storage space used by 
the one GTT in the entire db(include all session) .

> 
> Regards
> 
> Pavel
> 
> 
> Wenjing
> 
> 
> 
> 
> 
>> 2020年1月6日 上午4:06，Tomas Vondra > <mailto:tomas.von...@2ndquadrant.com>> 写道：
>> 
>> Hi,
>> 
>> I think we need to do something with having two patches aiming to add
>> global temporary tables:
>> 
>> [1] https://commitfest.postgresql.org/26/2349/ 
>> <https://commitfest.postgresql.org/26/2349/>
>> 
>> [2] https://commitfest.postgresql.org/26/2233/ 
>> <https://commitfest.postgresql.org/26/2233/>
>> 
>> As a reviewer I have no idea which of the threads to look at - certainly
>> not without reading both threads, which I doubt anyone will really do.
>> The reviews and discussions are somewhat intermixed between those two
>> threads, which makes it even more confusing.
>> 
>> I think we should agree on a minimal patch combining the necessary/good
>> bits from the various patches, and terminate one of the threads (i.e.
>> mark it as rejected or RWF). And we need to do that now, otherwise
>> there's about 0% chance of getting this into v13.
>> 
>> In general, I agree with the sentiment Rober expressed in [1] - the
>> patch needs to be as small as possible, not adding "nice to have"
>> features (like support for parallel queries - I very much doubt just
>> using shared instead of local buffers is enough to make it work.)
>> 
>> regards
>> 
>> -- 
>> Tomas Vondra  http://www.2ndQuadrant.com 
>> <http://www.2ndquadrant.com/>
>> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [Proposal] Global temporary tables

2020-01-13 Thread Tomas Vondra

On Mon, Jan 13, 2020 at 09:12:38PM +0100, Julien Rouhaud wrote:

On Mon, Jan 13, 2020 at 05:32:53PM +0100, Tomas Vondra wrote:

On Mon, Jan 13, 2020 at 11:08:40AM +0300, Konstantin Knizhnik wrote:
>
>"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.

AFAIR, hypopg only opens pg_statistic to use its tupledesc when creating
statistics on hypothetical partitions, but it should otherwise never reads or
need plain pg_statistic rows.

Ah, OK! Thanks for the clarification. I knew it does something with the
catalog, didn't realize it only gets the descriptor.

regards

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

Re: [Proposal] Global temporary tables

2020-01-13 Thread Julien Rouhaud

On Mon, Jan 13, 2020 at 05:32:53PM +0100, Tomas Vondra wrote:
> On Mon, Jan 13, 2020 at 11:08:40AM +0300, Konstantin Knizhnik wrote:
> >
> >"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.

AFAIR, hypopg only opens pg_statistic to use its tupledesc when creating
statistics on hypothetical partitions, but it should otherwise never reads or
need plain pg_statistic rows.

Re: [Proposal] Global temporary tables

2020-01-13 Thread Tomas Vondra


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

Re: [Proposal] Global temporary tables

2020-01-13 Thread Konstantin Knizhnik





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.




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.


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

Re: [Proposal] Global temporary tables

2020-01-11 Thread Tomas Vondra

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.

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.

* 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?

... 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.

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.

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).

Can't we track which indexes a particular session sees, somehow?

Statistic is another important case.
But once again I do not completely understand why we want to
address all this issues with statistic in first version of the
patch?

I think the question is which "issues with statistic" you mean. I'm sure
we can ignore some of them, e.g. the one with parallel workers not
having any stats (assuming we consider functions using GTT to be
parallel restricted).

If we do not use shared buffers for GTT then parallel processing of
GTT is not possible at all, so there is no problem with statistic for
parallel workers.

Right.

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

regards

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

Re: [Proposal] Global temporary tables

2020-01-11 Thread Tomas Vondra


On Fri, Jan 10, 2020 at 03:24:34PM +0300, Konstantin Knizhnik wrote:



On 09.01.2020 19:30, Tomas Vondra wrote:








3 Still no one commented on GTT's transaction information 
processing, they include

3.1 Should gtt's frozenxid need to be care?
3.2 gtt’s clog clean
3.3 How to deal with "too old" gtt data



No idea what to do about this.



I wonder what is the specific of GTT here?
The same problem takes place for normal (local) temp tables, doesn't it?



Not sure. TBH I'm not sure I understand what the issue actually is.


Just open session, create temporary table and insert some data in it.
Then in other session run 2^31 transactions (at my desktop it takes 
about 2 hours).

As far as temp tables are not proceeded by vacuum, database is stalled:

 ERROR:  database is not accepting commands to avoid wraparound data 
loss in database "postgres"


It seems to be quite dubious behavior and it is strange to me that 
nobody complains about it.
We discuss  many issues related with temp tables (statistic, parallel 
queries,...) which seems to be less critical.


But this problem is not specific to GTT - it can be reproduced with 
normal (local) temp tables.

This is why I wonder why do we need to solve it in GTT patch.



Yeah, I think that's out of scope for GTT patch. Once we solve it for
plain temporary tables, we'll solve it for GTT too.

regards

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

Re: [Proposal] Global temporary tables

2020-01-11 Thread Pavel Stehule

Hi

so 11. 1. 2020 v 15:00 odesílatel 曾文旌(义从) 
napsal:

> Hi all
>
> This is the latest patch
>
> The updates are as follows:
> 1. Support global temp Inherit table global temp partition table
> 2. Support serial column in GTT
> 3. Provide views pg_gtt_relstats pg_gtt_stats for GTT’s statistics
> 4. Provide view pg_gtt_attached_pids to manage GTT
> 5. Provide function pg_list_gtt_relfrozenxids() to manage GTT
> 6. Alter GTT or rename GTT is allowed under some conditions
>
>
> Please give me feedback.
>

I tested the functionality

1. i think so "ON COMMIT PRESERVE ROWS" should be default mode (like local
temp tables).

I tested some simple scripts

test01.sql

CREATE TEMP TABLE foo(a int, b int);
INSERT INTO foo SELECT random()*100, random()*1000 FROM
generate_series(1,1000);
ANALYZE foo;
SELECT sum(a), sum(b) FROM foo;
DROP TABLE foo; -- simulate disconnect


after 100 sec, the table pg_attribute has 3.2MB
and 64 tps, 6446 transaction

test02.sql

INSERT INTO foo SELECT random()*100, random()*1000 FROM
generate_series(1,1000);
ANALYZE foo;
SELECT sum(a), sum(b) FROM foo;
DELETE FROM foo; -- simulate disconnect


after 100 sec, 1688 tps, 168830 transactions

So performance is absolutely different as we expected.

>From my perspective, this functionality is great.

Todo:

pg_table_size function doesn't work

Regards

Pavel


> Wenjing
>
>
>
>
>
> 2020年1月6日 上午4:06，Tomas Vondra  写道：
>
> Hi,
>
> I think we need to do something with having two patches aiming to add
> global temporary tables:
>
> [1] https://commitfest.postgresql.org/26/2349/
>
> [2] https://commitfest.postgresql.org/26/2233/
>
> As a reviewer I have no idea which of the threads to look at - certainly
> not without reading both threads, which I doubt anyone will really do.
> The reviews and discussions are somewhat intermixed between those two
> threads, which makes it even more confusing.
>
> I think we should agree on a minimal patch combining the necessary/good
> bits from the various patches, and terminate one of the threads (i.e.
> mark it as rejected or RWF). And we need to do that now, otherwise
> there's about 0% chance of getting this into v13.
>
> In general, I agree with the sentiment Rober expressed in [1] - the
> patch needs to be as small as possible, not adding "nice to have"
> features (like support for parallel queries - I very much doubt just
> using shared instead of local buffers is enough to make it work.)
>
> regards
>
> --
> Tomas Vondra  http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>
>

Re: [Proposal] Global temporary tables

2020-01-10 Thread Konstantin Knizhnik




On 09.01.2020 19:30, Tomas Vondra wrote:








3 Still no one commented on GTT's transaction information 
processing, they include

3.1 Should gtt's frozenxid need to be care?
3.2 gtt’s clog clean
3.3 How to deal with "too old" gtt data



No idea what to do about this.



I wonder what is the specific of GTT here?
The same problem takes place for normal (local) temp tables, doesn't it?



Not sure. TBH I'm not sure I understand what the issue actually is. 


Just open session, create temporary table and insert some data in it.
Then in other session run 2^31 transactions (at my desktop it takes 
about 2 hours).

As far as temp tables are not proceeded by vacuum, database is stalled:

 ERROR:  database is not accepting commands to avoid wraparound data 
loss in database "postgres"


It seems to be quite dubious behavior and it is strange to me that 
nobody complains about it.
We discuss  many issues related with temp tables (statistic, parallel 
queries,...) which seems to be less critical.


But this problem is not specific to GTT - it can be reproduced with 
normal (local) temp tables.

This is why I wonder why do we need to solve it in GTT patch.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [Proposal] Global temporary tables

2020-01-10 Thread Konstantin Knizhnik





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.


Can't we track which indexes a particular session sees, somehow?


Statistic is another important case.
But once again I do not completely understand why we want to address 
all this issues with statistic in first version of the patch?


I think the question is which "issues with statistic" you mean. I'm sure
we can ignore some of them, e.g. the one with parallel workers not
having any stats (assuming we consider functions using GTT to be
parallel restricted).


If we do not use shared buffers for GTT then parallel processing of GTT 
is not possible at all, so there is no problem with statistic for 
parallel workers.




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.



--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [Proposal] Global temporary tables

2020-01-09 Thread Tomas Vondra


On Thu, Jan 09, 2020 at 02:17:08PM +0300, Konstantin Knizhnik wrote:



On 06.01.2020 8:04, 曾文旌(义从) wrote:

In the previous communication

1 we agreed on the general direction
1.1 gtt use local (private) buffer
1.2 no replica access in first version

2 We feel that gtt needs to maintain statistics, but there is no agreement on 
what it will be done.

3 Still no one commented on GTT's transaction information processing, they 
include
3.1 Should gtt's frozenxid need to be care?
3.2 gtt’s clog clean
3.3 How to deal with "too old" gtt data

I suggest we discuss further, reach an agreement, and merge the two patches to 
one.



I also hope that we should come to the common solution for GTT.
If we do not try to address parallel execution issues and access to 
temp tables at replicas (and I agreed
that it should be avoided in first version of the patch), then GTT 
patch becomes quite small.




Well, that was kinda my goal - making the patch as small as possible by
eliminating bits that are contentious or where we don't know the
solution (like planning for parallel queries).

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.

Can't we track which indexes a particular session sees, somehow?


Statistic is another important case.
But once again I do not completely understand why we want to address 
all this issues with statistic in first version of the patch?


I think the question is which "issues with statistic" you mean. I'm sure
we can ignore some of them, e.g. the one with parallel workers not
having any stats (assuming we consider functions using GTT to be
parallel restricted).


It contradicts to the idea to make this patch as small as possible.


Well, there's "making patch as small as possible" vs. "patch behaving
correctly" trade-off ;-)

Also it seems to me that everybody agreed that users very rarely 
create indexes for temp tables and explicitly analyze them.


I certainly *disagree* with this.

We often see temporary tables as a fix or misestimates in complex
queries, and/or as a replacement for CTEs with statistics/indexes. In
fact it's a pretty valuable tool when helping customers with complex
queries affected by poor estimates.

So I think GTT will be useful even with limited support of statistic. 
In my version statistics for GTT is provided by pushing correspondent 
information to backend's cache for pg_statistic table.


I think someone pointed out pushing stuff directly into the cache is
rather problematic, but I don't recall the details.

Also I provided pg_temp_statistic view for inspecting it by users. The 
idea to make pg_statistic a view which combines statistic of normal 
and temporary tables is overkill from my point of view.


I do not understand why do we need to maintain hash with some extra 
information for GTT in backends memory (as it was done in Wenjing 
patch).
Also idea to use create extension for accessing this information seems 
to be dubious.




I think the extension was more a PoC rather than a final solution.


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

Re: [Proposal] Global temporary tables

2020-01-09 Thread Tomas Vondra


On Thu, Jan 09, 2020 at 06:07:46PM +0300, Konstantin Knizhnik wrote:



On 06.01.2020 14:01, Tomas Vondra wrote:

On Mon, Jan 06, 2020 at 01:04:15PM +0800, 曾文旌(义从) wrote:

In the previous communication

1 we agreed on the general direction 1.1 gtt use local (private)
buffer 1.2 no replica access in first version



OK, good.


2 We feel that gtt needs to maintain statistics, but there is no
agreement on what it will be done.



I certainly agree GTT needs to maintain statistics, otherwise it'll
lead to poor query plans. AFAIK the current patch stores the info in a
hash table in a backend private memory, and I don't see how else to do
that (e.g. storing it in a catalog would cause catalog bloat).

FWIW this is a reasons why I think just using shared buffers (instead
of local ones) is not sufficient to support parallel queriesl as
proposed by Alexander. The workers would not know the stats, breaking
planning of queries in PARALLEL SAFE plpgsql functions etc.



I do not think that "all or nothing" approach is so good for software
development as for database transactions.


Well, sure. I'm not saying we need to have a perfect solution in v1. I'm
saying if we have two choices:

(1) Use shared buffers even if it means the parallel query plan may be
arbitrarily bad.

(2) Use private buffers, even if it means no parallel queries with temp
tables.

Then I'm voting for (2) because it's less likely to break down. I can
imagine allowing parallel queries with GTT when there's no risk of
having to plan in the worker, but that's not there yet.

If we can come up with a reasonable solution for the parallel case, we
can enable it later.

Yes, if we have function in PL/pgSQL which performs queries om 
temporary tables, then
parallel workers may build inefficient plan for this queries due to 
lack of statistics.


IMHO that's a pretty awful deficiency, because it essentially means
users may need to disable parallelism for such queries. Which means
we'll get complaints from users, and we'll have to come up with some
sort of solution. I'd rather not be in that position.

From my point of view this is not a pitfall of GTT but result of lack 
of global plan cache in Postgres. And it should be fixed not at GTT 
level.




That doesn't give us free pass to just ignore the issue. Even if it
really was due to a lack of global plan cache, the fact is we don't have
that feature, so we have a problem. I mean, if you need infrastructure
that is not available, you either have to implement that infrastructure
or make it work properly without it.

Also I never see real use cases with such functions, even in the 
systems which using hard temporary tables and stored procedures.
But there are many other real problems with temp tables  (except 
already mentioned in this thread).


Oh, I'm sure there are pretty large plpgsql applications, and I'd be
surprised if at least some of those were not affected. And I'm sure
there are apps using UDF to do all sorts of stuff (e.g. I wonder if
PostGIS would have this issue - IIRC it's using SPI etc.).

The question is whether we should consider existing apps affected,
because they are using the regular temporary tables and not GTT. So
unless they switch to GTT there is no regression ...

But even in that case I don't think it's a good idea to accept this as
an acceptable limitation. I admit one of the reasons why I think that
may be that statistics and planning are my areas of interest, so I'm not
quite willing to accept incomplete stuff as OK.


In PgPro/EE we have fixes for some of them, for example:

1. Do not reserve space in the file for temp relations. Right now 
append of relation cause writing zero page to the disk by mdextend.
It cause useless disk IO for temp tables which in most cases fit in 
memory and should not be written at disk.


2. Implicitly perform analyze of temp table intermediately after 
storing data in it. Usually tables are analyzed by autovacuum in 
background.
But it doesn't work for temp tables which are not processes by 
autovacuum and are accessed immediately after filling them with data 
and lack of statistic  may cause
building very inefficient plan. We have online_analyze extension which 
force analyze of the table after appending some bulk of data to it.
It can be used for normal table but most of all it is useful for temp 
relations.


Unlike hypothetical example with parallel safe function working with 
temp tables,

this are real problems observed by some of our customers.
Them are applicable both to local and global temp tables and this is 
why I do not want to discuss them in context of GTT.




I think those are both interesting issues worth fixing, but I don't
think it makes the issue discussed here less important.





3 Still no one commented on GTT's transaction information 
processing, they include

3.1 Should gtt's frozenxid need to be care?
3.2 gtt’s clog clean
3.3 How to deal with "too old" gtt data



No idea what to do about this.



I wonder

Re: [Proposal] Global temporary tables

2020-01-09 Thread Konstantin Knizhnik





On 06.01.2020 14:01, Tomas Vondra wrote:

On Mon, Jan 06, 2020 at 01:04:15PM +0800, 曾文旌(义从) wrote:

In the previous communication

1 we agreed on the general direction
1.1 gtt use local (private) buffer
1.2 no replica access in first version



OK, good.


2 We feel that gtt needs to maintain statistics, but there is no
agreement on what it will be done.



I certainly agree GTT needs to maintain statistics, otherwise it'll lead
to poor query plans. AFAIK the current patch stores the info in a hash
table in a backend private memory, and I don't see how else to do that
(e.g. storing it in a catalog would cause catalog bloat).

FWIW this is a reasons why I think just using shared buffers (instead of
local ones) is not sufficient to support parallel queriesl as proposed
by Alexander. The workers would not know the stats, breaking planning of
queries in PARALLEL SAFE plpgsql functions etc.



I do not think that "all or nothing" approach is so good for software 
development as for database transactions.
Yes, if we have function in PL/pgSQL which performs queries om temporary 
tables, then
parallel workers may build inefficient plan for this queries due to lack 
of statistics.
From my point of view this is not a pitfall of GTT but result of lack 
of global plan cache in Postgres. And it should be fixed not at GTT level.


Also I never see real use cases with such functions, even in the systems 
which using hard temporary tables and stored procedures.
But there are many other real problems with temp tables  (except already 
mentioned in this thread).

In PgPro/EE we have fixes for some of them, for example:

1. Do not reserve space in the file for temp relations. Right now append 
of relation cause writing zero page to the disk by mdextend.
It cause useless disk IO for temp tables which in most cases fit in 
memory and should not be written at disk.


2. Implicitly perform analyze of temp table intermediately after storing 
data in it. Usually tables are analyzed by autovacuum in background.
But it doesn't work for temp tables which are not processes by 
autovacuum and are accessed immediately after filling them with data and 
lack of statistic  may cause
building very inefficient plan. We have online_analyze extension which 
force analyze of the table after appending some bulk of data to it.
It can be used for normal table but most of all it is useful for temp 
relations.


Unlike hypothetical example with parallel safe function working with 
temp tables,

this are real problems observed by some of our customers.
Them are applicable both to local and global temp tables and this is why 
I do not want to discuss them in context of GTT.





3 Still no one commented on GTT's transaction information processing, 
they include

3.1 Should gtt's frozenxid need to be care?
3.2 gtt’s clog clean
3.3 How to deal with "too old" gtt data



No idea what to do about this.



I wonder what is the specific of GTT here?
The same problem takes place for normal (local) temp tables, doesn't it?


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [Proposal] Global temporary tables

2020-01-09 Thread Konstantin Knizhnik





On 06.01.2020 8:04, 曾文旌(义从) wrote:

In the previous communication

1 we agreed on the general direction
1.1 gtt use local (private) buffer
1.2 no replica access in first version

2 We feel that gtt needs to maintain statistics, but there is no agreement on 
what it will be done.

3 Still no one commented on GTT's transaction information processing, they 
include
3.1 Should gtt's frozenxid need to be care?
3.2 gtt’s clog clean
3.3 How to deal with "too old" gtt data

I suggest we discuss further, reach an agreement, and merge the two patches to 
one.



I also hope that we should come to the common solution for GTT.
If we do not try to address parallel execution issues and access to temp 
tables at replicas (and I agreed
that it should be avoided in first version of the patch), then GTT patch 
becomes quite small.


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.


Statistic is another important case.
But once again I do not completely understand why we want to address all 
this issues with statistic in first version of the patch? It contradicts 
to the idea to make this patch as small as possible.
Also it seems to me that everybody agreed that users very rarely create 
indexes for temp tables and explicitly analyze them.
So I think GTT will be useful even with limited support of statistic. In 
my version statistics for GTT is provided by pushing correspondent 
information to backend's cache for pg_statistic table.
Also I provided pg_temp_statistic view for inspecting it by users. The 
idea to make pg_statistic a view which combines statistic of normal and 
temporary tables is overkill from my point of view.


I do not understand why do we need to maintain hash with some extra 
information for GTT in backends memory (as it was done in Wenjing patch).
Also idea to use create extension for accessing this information seems 
to be dubious.


--
Konstantin Knizhnik
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

Re: [Proposal] Global temporary tables

2020-01-07 Thread 曾文旌(义从)



> 2020年1月6日 下午8:17，Dean Rasheed  写道：
> 
> On Mon, 6 Jan 2020 at 11:01, Tomas Vondra  
> wrote:
>> 
>> On Mon, Jan 06, 2020 at 01:04:15PM +0800, 曾文旌(义从) wrote:
>> 
>>> 2 We feel that gtt needs to maintain statistics, but there is no
>>> agreement on what it will be done.
>>> 
>> 
>> I certainly agree GTT needs to maintain statistics, otherwise it'll lead
>> to poor query plans.
> 
> +1
> 
>> AFAIK the current patch stores the info in a hash
>> table in a backend private memory, and I don't see how else to do that
>> (e.g. storing it in a catalog would cause catalog bloat).
>> 
> 
> It sounds like it needs a pair of system GTTs to hold the table and
> column statistics for other GTTs. One would probably have the same
> columns as pg_statistic, and the other just the relevant columns from
> pg_class. I can see it being useful for the user to be able to see
> these stats, so perhaps they could be UNIONed into the existing stats
> view.
The current patch provides several functions as extension(pg_gtt) for read gtt 
statistics. 
Next I can move them to the kernel and let the view pg_stats can see gtt’s 
statistics.


> Regards,
> Dean

Re: [Proposal] Global temporary tables

2020-01-06 Thread Pavel Stehule

po 6. 1. 2020 v 13:17 odesílatel Dean Rasheed 
napsal:

> On Mon, 6 Jan 2020 at 11:01, Tomas Vondra 
> wrote:
> >
> > On Mon, Jan 06, 2020 at 01:04:15PM +0800, 曾文旌(义从) wrote:
> >
> > >2 We feel that gtt needs to maintain statistics, but there is no
> > >agreement on what it will be done.
> > >
> >
> > I certainly agree GTT needs to maintain statistics, otherwise it'll lead
> > to poor query plans.
>
> +1
>
> > AFAIK the current patch stores the info in a hash
> > table in a backend private memory, and I don't see how else to do that
> > (e.g. storing it in a catalog would cause catalog bloat).
> >
>
> It sounds like it needs a pair of system GTTs to hold the table and
> column statistics for other GTTs. One would probably have the same
> columns as pg_statistic, and the other just the relevant columns from
> pg_class. I can see it being useful for the user to be able to see
> these stats, so perhaps they could be UNIONed into the existing stats
> view.
>

+1

Pavel


> Regards,
> Dean
>

Re: [Proposal] Global temporary tables

2020-01-06 Thread Tomas Vondra

On Mon, Jan 06, 2020 at 12:17:43PM +, Dean Rasheed wrote:

On Mon, 6 Jan 2020 at 11:01, Tomas Vondra  wrote:

On Mon, Jan 06, 2020 at 01:04:15PM +0800, 曾文旌(义从) wrote:

>2 We feel that gtt needs to maintain statistics, but there is no
>agreement on what it will be done.
>

I certainly agree GTT needs to maintain statistics, otherwise it'll lead
to poor query plans.

+1

AFAIK the current patch stores the info in a hash
table in a backend private memory, and I don't see how else to do that
(e.g. storing it in a catalog would cause catalog bloat).

It sounds like it needs a pair of system GTTs to hold the table and
column statistics for other GTTs. One would probably have the same
columns as pg_statistic, and the other just the relevant columns from
pg_class. I can see it being useful for the user to be able to see
these stats, so perhaps they could be UNIONed into the existing stats
view.

Hmmm, yeah. A "temporary catalog" (not sure if it can work exactly the
same as GTT) storing pg_statistics data for GTTs might work, I think. It
would not have the catalog bloat issue, which is good.

I still think we'd need to integrate this with the regular pg_statistic
catalogs somehow, so that people don't have to care about two things. I
mean, extensions like hypopg do use pg_statistic data to propose indexes
etc. and it would be nice if we don't make them more complicated.

Not sure why we'd need a temporary version of pg_class, though?

regards

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

Re: [Proposal] Global temporary tables

2020-01-06 Thread Dean Rasheed

On Mon, 6 Jan 2020 at 11:01, Tomas Vondra  wrote:
>
> On Mon, Jan 06, 2020 at 01:04:15PM +0800, 曾文旌(义从) wrote:
>
> >2 We feel that gtt needs to maintain statistics, but there is no
> >agreement on what it will be done.
> >
>
> I certainly agree GTT needs to maintain statistics, otherwise it'll lead
> to poor query plans.

+1

> AFAIK the current patch stores the info in a hash
> table in a backend private memory, and I don't see how else to do that
> (e.g. storing it in a catalog would cause catalog bloat).
>

It sounds like it needs a pair of system GTTs to hold the table and
column statistics for other GTTs. One would probably have the same
columns as pg_statistic, and the other just the relevant columns from
pg_class. I can see it being useful for the user to be able to see
these stats, so perhaps they could be UNIONed into the existing stats
view.

Regards,
Dean

Re: [Proposal] Global temporary tables

2020-01-06 Thread Tomas Vondra


On Mon, Jan 06, 2020 at 01:04:15PM +0800, 曾文旌(义从) wrote:

In the previous communication

1 we agreed on the general direction
1.1 gtt use local (private) buffer
1.2 no replica access in first version



OK, good.


2 We feel that gtt needs to maintain statistics, but there is no
agreement on what it will be done.



I certainly agree GTT needs to maintain statistics, otherwise it'll lead
to poor query plans. AFAIK the current patch stores the info in a hash
table in a backend private memory, and I don't see how else to do that
(e.g. storing it in a catalog would cause catalog bloat).

FWIW this is a reasons why I think just using shared buffers (instead of
local ones) is not sufficient to support parallel queriesl as proposed
by Alexander. The workers would not know the stats, breaking planning of
queries in PARALLEL SAFE plpgsql functions etc.


3 Still no one commented on GTT's transaction information processing, they 
include
3.1 Should gtt's frozenxid need to be care?
3.2 gtt’s clog clean
3.3 How to deal with "too old" gtt data



No idea what to do about this.


I suggest we discuss further, reach an agreement, and merge the two patches to 
one.



OK, cool. Thanks for the clarification.


regards

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

Re: [Proposal] Global temporary tables

2020-01-05 Thread 曾文旌(义从)

In the previous communication

1 we agreed on the general direction
1.1 gtt use local (private) buffer
1.2 no replica access in first version

2 We feel that gtt needs to maintain statistics, but there is no agreement on 
what it will be done.

3 Still no one commented on GTT's transaction information processing, they 
include
3.1 Should gtt's frozenxid need to be care?
3.2 gtt’s clog clean
3.3 How to deal with "too old" gtt data

I suggest we discuss further, reach an agreement, and merge the two patches to 
one.


Wenjing


> 2020年1月6日 上午4:06，Tomas Vondra  写道：
> 
> Hi,
> 
> I think we need to do something with having two patches aiming to add
> global temporary tables:
> 
> [1] https://commitfest.postgresql.org/26/2349/
> 
> [2] https://commitfest.postgresql.org/26/2233/
> 
> As a reviewer I have no idea which of the threads to look at - certainly
> not without reading both threads, which I doubt anyone will really do.
> The reviews and discussions are somewhat intermixed between those two
> threads, which makes it even more confusing.
> 
> I think we should agree on a minimal patch combining the necessary/good
> bits from the various patches, and terminate one of the threads (i.e.
> mark it as rejected or RWF). And we need to do that now, otherwise
> there's about 0% chance of getting this into v13.
> 
> In general, I agree with the sentiment Rober expressed in [1] - the
> patch needs to be as small as possible, not adding "nice to have"
> features (like support for parallel queries - I very much doubt just
> using shared instead of local buffers is enough to make it work.)
> 
> regards
> 
> -- 
> Tomas Vondra  http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [Proposal] Global temporary tables

2020-01-05 Thread Tomas Vondra


Hi,

I think we need to do something with having two patches aiming to add
global temporary tables:

[1] https://commitfest.postgresql.org/26/2349/

[2] https://commitfest.postgresql.org/26/2233/

As a reviewer I have no idea which of the threads to look at - certainly
not without reading both threads, which I doubt anyone will really do.
The reviews and discussions are somewhat intermixed between those two
threads, which makes it even more confusing.

I think we should agree on a minimal patch combining the necessary/good
bits from the various patches, and terminate one of the threads (i.e.
mark it as rejected or RWF). And we need to do that now, otherwise
there's about 0% chance of getting this into v13.

In general, I agree with the sentiment Rober expressed in [1] - the
patch needs to be as small as possible, not adding "nice to have"
features (like support for parallel queries - I very much doubt just
using shared instead of local buffers is enough to make it work.)

regards

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

Re: Global temporary tables

2019-12-22 Thread Philippe BEAUDOIN

Hi all,

I am not aware enough in the Postgres internals to give advice about the 
implementation.

But my feeling is that there is another big interest for this feature: simplify 
the Oracle to PostgreSQL migration of applications that use global termporary 
tables. And this is quite common when stored procedures are used. In such a 
case, we currently need to modify the logic of the code, always implementing an 
ugly solution (either add CREATE TEMP TABLE statements in the code everywhere 
it is needed, or use a regular table with additional TRUNCATE statements if we 
can ensure that only a single connection uses the table at a time).

So, Konstantin and all, Thanks by advance for all that could be done on this 
feature :-)

Best regards.

Re: Global temporary tables

2019-12-02 Thread Konstantin Knizhnik

+21,7 @@
 #include "access/spgist_private.h"
 #include "access/transam.h"
 #include "access/xact.h"
+#include "catalog/index.h"
 #include "catalog/pg_amop.h"
 #include "storage/bufmgr.h"
 #include "storage/indexfsm.h"
@@ -106,6 +107,7 @@ spgGetCache(Relation index)
 		spgConfigIn in;
 		FmgrInfo   *procinfo;
 		Buffer		metabuffer;
+		Pagemetapage;
 		SpGistMetaPageData *metadata;
 
 		cache = MemoryContextAllocZero(index->rd_indexcxt,
@@ -155,12 +157,21 @@ spgGetCache(Relation index)
 		metabuffer = ReadBuffer(index, SPGIST_METAPAGE_BLKNO);
 		LockBuffer(metabuffer, BUFFER_LOCK_SHARE);
 
-		metadata = SpGistPageGetMeta(BufferGetPage(metabuffer));
+		metapage = BufferGetPage(metabuffer);
+		metadata = SpGistPageGetMeta(metapage);
 
 		if (metadata->magicNumber != SPGIST_MAGIC_NUMBER)
-			elog(ERROR, "index \"%s\" is not an SP-GiST index",
- RelationGetRelationName(index));
-
+		{
+			if (GlobalTempRelationPageIsNotInitialized(index, metapage))
+			{
+Relation heap = RelationIdGetRelation(index->rd_index->indrelid);
+spgbuild(heap, index, BuildIndexInfo(index));
+RelationClose(heap);
+			}
+			else
+elog(ERROR, "index \"%s\" is not an SP-GiST index",
+	 RelationGetRelationName(index));
+		}
 		cache->lastUsedPages = metadata->lastUsedPages;
 
 		UnlockReleaseBuffer(metabuffer);
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 6b10469..88fca55 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -401,6 +401,9 @@ GetNewRelFileNode(Oid reltablespace, Relation pg_class, char relpersistence)
 		case RELPERSISTENCE_TEMP:
 			backend = BackendIdForTempRelations();
 			break;
+		case RELPERSISTENCE_SESSION:
+			backend = BackendIdForSessionRelations();
+			break;
 		case RELPERSISTENCE_UNLOGGED:
 		case RELPERSISTENCE_PERMANENT:
 			backend = InvalidBackendId;
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index e995570..1522109 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3699,7 +3699,7 @@ reindex_relation(Oid relid, int flags, int options)
 		if (flags & REINDEX_REL_FORCE_INDEXES_UNLOGGED)
 			persistence = RELPERSISTENCE_UNLOGGED;
 		else if (flags & REINDEX_REL_FORCE_INDEXES_PERMANENT)
-			persistence = RELPERSISTENCE_PERMANENT;
+			persistence = rel->rd_rel->relpersistence == RELPERSISTENCE_SESSION ? RELPERSISTENCE_SESSION : RELPERSISTENCE_PERMANENT;
 		else
 			persistence = rel->rd_rel->relpersistence;
 
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 056ea3d..317574a 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -92,6 +92,10 @@ RelationCreateStorage(RelFileNode rnode, char relpersistence)
 			backend = InvalidBackendId;
 			needs_wal = false;
 			break;
+		case RELPERSISTENCE_SESSION:
+			backend = BackendIdForSessionRelations();
+			needs_wal = false;
+			break;
 		case RELPERSISTENCE_PERMANENT:
 			backend = InvalidBackendId;
 			needs_wal = true;
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index f7800f0..23ab456 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1332,7 +1332,15 @@ LANGUAGE INTERNAL
 STRICT STABLE PARALLEL SAFE
 AS 'jsonb_path_query_first_tz';
 
+
+--
+-- Statistic for global temporary tables
 --
+
+CREATE VIEW pg_gtt_statistic AS
+   SELECT s.* from pg_class c,pg_gtt_statistic_for_relation(c.oid) s where c.relpersistence='s';
+
+
 -- The default permissions for functions mean that anyone can execute them.
 -- A number of functions shouldn't be executable by just anyone, but rather
 -- than use explicit 'superuser()' checks in those functions, we use the GRANT
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 71372ce..324a249 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -39,6 +39,7 @@
 #include "commands/vacuum.h"
 #include "executor/executor.h"
 #include "foreign/fdwapi.h"
+#include "funcapi.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
 #include "parser/parse_oper.h"
@@ -102,7 +103,7 @@ static int	acquire_inherited_sample_rows(Relation onerel, int elevel,
 		  HeapTuple *rows, int targrows,
 		  double *totalrows, double *totaldeadrows);
 static void update_attstats(Oid relid, bool inh,
-			int natts, VacAttrStats **vacattrstats);
+			int natts, VacAttrStats **vacattrstats, bool is_global_temp);
 static Datum std_fetch_func(VacAttrStatsP stats, int rownum, bool *isNull);
 static Datum ind_fetch_func(VacAttrStatsP stats, int rownum, bool *isNull);
 
@@ -318,6 +319,7 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 	Oid			save_userid;
 	int			save_sec_contex

Re: Global temporary tables

2019-11-30 Thread Michael Paquier

On Wed, Nov 20, 2019 at 07:32:14PM +0300, Konstantin Knizhnik wrote:
> Now pg_gtt_statistic view is provided for global temp tables.

Latest patch fails to apply, per Mr Robot's report.  Could you please
rebase and send an updated version?  For now I have moved the patch to
next CF, waiting on author.
--
Michael


signature.asc
Description: PGP signature

Re: Global temporary tables

2019-11-20 Thread Konstantin Knizhnik

n SP-GiST index",
- RelationGetRelationName(index));
-
+		{
+			if (GlobalTempRelationPageIsNotInitialized(index, metapage))
+			{
+Relation heap = RelationIdGetRelation(index->rd_index->indrelid);
+spgbuild(heap, index, BuildIndexInfo(index));
+RelationClose(heap);
+			}
+			else
+elog(ERROR, "index \"%s\" is not an SP-GiST index",
+	 RelationGetRelationName(index));
+		}
 		cache->lastUsedPages = metadata->lastUsedPages;
 
 		UnlockReleaseBuffer(metabuffer);
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 1af31c2..e60bdb7 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -402,6 +402,9 @@ GetNewRelFileNode(Oid reltablespace, Relation pg_class, char relpersistence)
 		case RELPERSISTENCE_TEMP:
 			backend = BackendIdForTempRelations();
 			break;
+		case RELPERSISTENCE_SESSION:
+			backend = BackendIdForSessionRelations();
+			break;
 		case RELPERSISTENCE_UNLOGGED:
 		case RELPERSISTENCE_PERMANENT:
 			backend = InvalidBackendId;
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index f6c31cc..d943b57 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3652,7 +3652,7 @@ reindex_relation(Oid relid, int flags, int options)
 		if (flags & REINDEX_REL_FORCE_INDEXES_UNLOGGED)
 			persistence = RELPERSISTENCE_UNLOGGED;
 		else if (flags & REINDEX_REL_FORCE_INDEXES_PERMANENT)
-			persistence = RELPERSISTENCE_PERMANENT;
+			persistence = rel->rd_rel->relpersistence == RELPERSISTENCE_SESSION ? RELPERSISTENCE_SESSION : RELPERSISTENCE_PERMANENT;
 		else
 			persistence = rel->rd_rel->relpersistence;
 
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 625af8d..1e192fa 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -93,6 +93,10 @@ RelationCreateStorage(RelFileNode rnode, char relpersistence)
 			backend = InvalidBackendId;
 			needs_wal = false;
 			break;
+		case RELPERSISTENCE_SESSION:
+			backend = BackendIdForSessionRelations();
+			needs_wal = false;
+			break;
 		case RELPERSISTENCE_PERMANENT:
 			backend = InvalidBackendId;
 			needs_wal = true;
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 9fe4a47..46b07c4 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1327,7 +1327,17 @@ LANGUAGE INTERNAL
 STRICT STABLE PARALLEL SAFE
 AS 'jsonb_path_query_first_tz';
 
+
+--
+-- Statistic for global temporary tables
 --
+
+CREATE VIEW pg_sequence_params AS select s.* from pg_class c,pg_sequence_parameters(c.oid) s where c.relkind='S';
+
+CREATE VIEW pg_gtt_statistic AS
+   SELECT s.* from pg_class c,pg_gtt_statistic_for_relation(c.oid) s where c.relpersistence='s';
+
+
 -- The default permissions for functions mean that anyone can execute them.
 -- A number of functions shouldn't be executable by just anyone, but rather
 -- than use explicit 'superuser()' checks in those functions, we use the GRANT
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 7accb95..fb11f26 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -39,6 +39,7 @@
 #include "commands/vacuum.h"
 #include "executor/executor.h"
 #include "foreign/fdwapi.h"
+#include "funcapi.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
 #include "parser/parse_oper.h"
@@ -102,7 +103,7 @@ static int	acquire_inherited_sample_rows(Relation onerel, int elevel,
 		  HeapTuple *rows, int targrows,
 		  double *totalrows, double *totaldeadrows);
 static void update_attstats(Oid relid, bool inh,
-			int natts, VacAttrStats **vacattrstats);
+			int natts, VacAttrStats **vacattrstats, bool is_global_temp);
 static Datum std_fetch_func(VacAttrStatsP stats, int rownum, bool *isNull);
 static Datum ind_fetch_func(VacAttrStatsP stats, int rownum, bool *isNull);
 
@@ -318,6 +319,7 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 	Oid			save_userid;
 	int			save_sec_context;
 	int			save_nestlevel;
+	boolis_global_temp = onerel->rd_rel->relpersistence == RELPERSISTENCE_SESSION;
 
 	if (inh)
 		ereport(elevel,
@@ -575,14 +577,14 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 		 * pg_statistic for columns we didn't process, we leave them alone.)
 		 */
 		update_attstats(RelationGetRelid(onerel), inh,
-		attr_cnt, vacattrstats);
+		attr_cnt, vacattrstats, is_global_temp);
 
 		for (ind = 0; ind < nindexes; ind++)
 		{
 			AnlIndexData *thisdata = [ind];
 
 			update_attstats(RelationGetRelid(Irel[ind]), false,
-			thisdata->attr_cnt, thisdata->vacattrstats);
+			thisdata->attr_cnt, thisdata->vacattrstats, is_global_temp);
 		}
 
 		/*
@@ -1425,7 +1427,7 @@ acquire_inherited_sample_rows(Relation onerel, int elevel,
  *		by ta

Re: [Proposal] Global temporary tables

2019-11-11 Thread Konstantin Knizhnik




On 08.11.2019 18:06, 曾文旌(义从) wrote:

My comments for global_private_temp-4.patch


Thank you very much for inspecting my patch.


good side:
1 Lots of  index type on GTT. I think we need support for all kinds of 
indexes.

2 serial column on GTT.
3 INHERITS GTT.
4 PARTITION GTT.

I didn't choose to support them in the first release, but you did.

Other side:
1 case: create global temp table gtt2(a int primary key, b text) on 
commit delete rows;

I think you've lost the meaning of the on commit delete rows clause.
After the GTT is created, the other sessions feel that this is an on 
commit PRESERVE rows GTT.




Yes, there was bug in my implementation of ON COMMIT DELETE ROWS for GTT.
It is fixed in global_private_temp-6.patch


2 truncate gtt, mybe this is a bug in DropRelFileNodeBuffers.
GTT's local buffer is not released.
Case:
postgres=# insert into gtt2 values(1,'xx');
INSERT 0 1
postgres=# truncate gtt2;
TRUNCATE TABLE
postgres=# insert into gtt2 values(1,'xx');
ERROR:  unexpected data beyond EOF in block 0 of relation 
base/13579/t3_16384
HINT:  This has been seen to occur with buggy kernels; consider 
updating your system.




Yes another bug, also fixed in new version of the patch.


3  lock type of truncate GTT.
I don't think it's a good idea to hold a big lock with truncate GTT, 
because it only needs to process private data.


Sorry, I do not understand which lock you are talking about.
I have not introduced any special locks for GTT.


4 GTT's ddl Those ddl that need to rewrite data files may need attention.
We have discussed in the previous email. This is why I used shared 
hash to track the GTT file.




You are right.
But instead of prohibiting ALTER TABLE at all for GTT, we can check
that there are no other backends using it.
I do not think that we should maintain some hash in shared memory to 
check it.
As far as ALTER TABLE is rare and slow operation in any case, we can 
just check presence of GTT files

created by other backends.
I have implemented this check in global_private_temp-6.patch




5 There will be problems with DDL that will change relfilenode. Such 
as cluster GTT ,vacuum full GTT.
A session completes vacuum full gtt(a), and other sessions will 
immediately start reading and writing new storage files and existing 
data is also lost.

I disable them in my current version.


Thank you for noticing it.
Autovacuum full should really be prohibited for GTT.



6 drop GTT
I think drop GTT should clean up all storage files and definitions. 
How do you think?



Storage files will be cleaned in any case on backend termination.
Certainly if backend creates  and deletes huge number of GTT in the 
loop, it can cause space exhaustion.

But it seems to be very strange pattern of GTT usage.




7 MVCC visibility clog clean
GTT data visibility rules, like regular tables, so GTT also need clog.
We need to avoid the clog that GTT needs to be cleaned up.
At the same time, GTT does not do autovacuum, and retaining "too old 
data" will cause wraparound data loss.

I have given a solution in my design.

But why do we need some special handling of visibility rules for GTT 
comparing with normal (local) temp tables?

Them are also not proceeded by autovacuum?

In principle, I have also implemented special visibility rules for GTT, 
but only for the case when them
are accessed at replica. And it is not included in this patch, because 
everybody think that access to GTT

replica should be considered in separate patch.


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Global temporary tables

2019-11-11 Thread Konstantin Knizhnik

(sd), values, nulls);
+CatalogTupleInsert(sd, stup);
+			}
+		}
 		heap_freetuple(stup);
 	}
 
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index a23128d..5d131a7 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1400,7 +1400,7 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
 	 */
 	if (newrelpersistence == RELPERSISTENCE_UNLOGGED)
 		reindex_flags |= REINDEX_REL_FORCE_INDEXES_UNLOGGED;
-	else if (newrelpersistence == RELPERSISTENCE_PERMANENT)
+	else if (newrelpersistence != RELPERSISTENCE_TEMP)
 		reindex_flags |= REINDEX_REL_FORCE_INDEXES_PERMANENT;
 
 	/* Report that we are now reindexing relations */
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index a13322b..be661a4 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -94,7 +94,7 @@ static HTAB *seqhashtab = NULL; /* hash table for SeqTable items */
  */
 static SeqTableData *last_used_seq = NULL;
 
-static void fill_seq_with_data(Relation rel, HeapTuple tuple);
+static void fill_seq_with_data(Relation rel, HeapTuple tuple, Buffer buf);
 static Relation lock_and_open_sequence(SeqTable seq);
 static void create_seq_hashtable(void);
 static void init_sequence(Oid relid, SeqTable *p_elm, Relation *p_rel);
@@ -222,7 +222,7 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
 
 	/* now initialize the sequence's data */
 	tuple = heap_form_tuple(tupDesc, value, null);
-	fill_seq_with_data(rel, tuple);
+	fill_seq_with_data(rel, tuple, InvalidBuffer);
 
 	/* process OWNED BY if given */
 	if (owned_by)
@@ -327,7 +327,7 @@ ResetSequence(Oid seq_relid)
 	/*
 	 * Insert the modified tuple into the new storage file.
 	 */
-	fill_seq_with_data(seq_rel, tuple);
+	fill_seq_with_data(seq_rel, tuple, InvalidBuffer);
 
 	/* Clear local cache so that we don't think we have cached numbers */
 	/* Note that we do not change the currval() state */
@@ -340,18 +340,21 @@ ResetSequence(Oid seq_relid)
  * Initialize a sequence's relation with the specified tuple as content
  */
 static void
-fill_seq_with_data(Relation rel, HeapTuple tuple)
+fill_seq_with_data(Relation rel, HeapTuple tuple, Buffer buf)
 {
-	Buffer		buf;
 	Page		page;
 	sequence_magic *sm;
 	OffsetNumber offnum;
+	bool lockBuffer = false;
 
 	/* Initialize first page of relation with special magic number */
 
-	buf = ReadBuffer(rel, P_NEW);
-	Assert(BufferGetBlockNumber(buf) == 0);
-
+	if (buf == InvalidBuffer)
+	{
+		buf = ReadBuffer(rel, P_NEW);
+		Assert(BufferGetBlockNumber(buf) == 0);
+		lockBuffer = true;
+	}
 	page = BufferGetPage(buf);
 
 	PageInit(page, BufferGetPageSize(buf), sizeof(sequence_magic));
@@ -360,7 +363,8 @@ fill_seq_with_data(Relation rel, HeapTuple tuple)
 
 	/* Now insert sequence tuple */
 
-	LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
+	if (lockBuffer)
+		LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
 
 	/*
 	 * Since VACUUM does not process sequences, we have to force the tuple to
@@ -410,7 +414,8 @@ fill_seq_with_data(Relation rel, HeapTuple tuple)
 
 	END_CRIT_SECTION();
 
-	UnlockReleaseBuffer(buf);
+	if (lockBuffer)
+		UnlockReleaseBuffer(buf);
 }
 
 /*
@@ -502,7 +507,7 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
 		/*
 		 * Insert the modified tuple into the new storage file.
 		 */
-		fill_seq_with_data(seqrel, newdatatuple);
+		fill_seq_with_data(seqrel, newdatatuple, InvalidBuffer);
 	}
 
 	/* process OWNED BY if given */
@@ -1178,6 +1183,17 @@ read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple)
 	LockBuffer(*buf, BUFFER_LOCK_EXCLUSIVE);
 
 	page = BufferGetPage(*buf);
+	if (GlobalTempRelationPageIsNotInitialized(rel, page))
+	{
+		/* Initialize sequence for global temporary tables */
+		Datum		value[SEQ_COL_LASTCOL] = {0};
+		bool		null[SEQ_COL_LASTCOL] = {false};
+		HeapTuple tuple;
+		value[SEQ_COL_LASTVAL-1] = Int64GetDatumFast(1); /* start sequence with 1 */
+		tuple = heap_form_tuple(RelationGetDescr(rel), value, null);
+		fill_seq_with_data(rel, tuple, *buf);
+	}
+
 	sm = (sequence_magic *) PageGetSpecialPointer(page);
 
 	if (sm->magic != SEQ_MAGIC)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8d25d14..21d5a30 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -12,6 +12,9 @@
  *
  *-
  */
+#include 
+#include 
+
 #include "postgres.h"
 
 #include "access/genam.h"
@@ -533,6 +536,23 @@ static List *GetParentedForeignKeyRefs(Relation partition);
 static void ATDetachCheckNoForeignKeyRefs(Relation partition);
 
 
+static bool
+has_oncommit_option(List *options)
+{
+	ListCell   *listptr;
+
+	foreach(listptr, options)
+	{
+		DefElem*def = (DefElem *) lfirst(listptr);
+
+		if (pg_strcasecmp(def->defname, "on_commit_delete_rows") == 0)
+			return true;
+	}
+
+	return false;
+}
+
+
 /* --

Re: [Proposal] Global temporary tables

2019-11-08 Thread 曾文旌(义从)

My comments for global_private_temp-4.patch

good side:
1 Lots of  index type on GTT. I think we need support for all kinds of indexes.
2 serial column on GTT.
3 INHERITS GTT.
4 PARTITION GTT.

I didn't choose to support them in the first release, but you did.

Other side:
1 case: create global temp table gtt2(a int primary key, b text) on commit 
delete rows;
I think you've lost the meaning of the on commit delete rows clause.
After the GTT is created, the other sessions feel that this is an on commit 
PRESERVE rows GTT.

2 truncate gtt, mybe this is a bug in DropRelFileNodeBuffers.
GTT's local buffer is not released.
Case:
postgres=# insert into gtt2 values(1,'xx');
INSERT 0 1
postgres=# truncate gtt2;
TRUNCATE TABLE
postgres=# insert into gtt2 values(1,'xx');
ERROR:  unexpected data beyond EOF in block 0 of relation base/13579/t3_16384
HINT:  This has been seen to occur with buggy kernels; consider updating your 
system.

3  lock type of truncate GTT.
I don't think it's a good idea to hold a big lock with truncate GTT, because it 
only needs to process private data.

4 GTT's ddl Those ddl that need to rewrite data files may need attention.
We have discussed in the previous email. This is why I used shared hash to 
track the GTT file.

5 There will be problems with DDL that will change relfilenode. Such as cluster 
GTT ,vacuum full GTT.
A session completes vacuum full gtt(a), and other sessions will immediately 
start reading and writing new storage files and existing data is also lost.
I disable them in my current version.

6 drop GTT
I think drop GTT should clean up all storage files and definitions. How do you 
think?

7 MVCC visibility clog clean
GTT data visibility rules, like regular tables, so GTT also need clog.
We need to avoid the clog that GTT needs to be cleaned up. 
At the same time, GTT does not do autovacuum, and retaining "too old data" will 
cause wraparound data loss.
I have given a solution in my design.

Zeng Wenjing

> 2019年11月1日 下午11:15，Konstantin Knizhnik  写道：
> 
> 
> 
> On 25.10.2019 20:00, Pavel Stehule wrote:
>> 
>> >
>> >> So except the limitation mentioned above (which I do not consider as 
>> >> critical) there is only one problem which was not addressed: maintaining 
>> >> statistics for GTT.
>> >> If all of the following conditions are true:
>> >>
>> >> 1) GTT are used in joins
>> >> 2) There are indexes defined for GTT
>> >> 3) Size and histogram of GTT in different backends can significantly vary.
>> >> 4) ANALYZE was explicitly called for GTT
>> >>
>> >> then query execution plan built in one backend will be also used for 
>> >> other backends where it can be inefficient.
>> >> I also do not consider this problem as "show stopper" for adding GTT to 
>> >> Postgres.
>> > I think that's *definitely* a show stopper.
>> Well, if both you and Pavel think that it is really "show stopper", then 
>> this problem really has to be addressed.
>> I slightly confused about this opinion, because Pavel has told me 
>> himself that 99% of users never create indexes for temp tables
>> or run "analyze" for them. And without it, this problem is not a problem 
>> at all.
>> 
>> 
>> Users doesn't do ANALYZE on temp tables in 99%. It's true. But second fact 
>> is so users has lot of problems. It's very similar to wrong statistics on 
>> persistent tables. When data are small, then it is not problem for users, 
>> although from my perspective it's not optimal. When data are not small, then 
>> the problem can be brutal. Temporary tables are not a exception. And users 
>> and developers are people - we know only about fatal problems. There are lot 
>> of unoptimized queries, but because the problem is not fatal, then it is not 
>> reason for report it. And lot of people has not any idea how fast the 
>> databases can be. The knowledges of  users and app developers are sad book.
>> 
>> Pavel
> 
> It seems to me that I have found quite elegant solution for per-backend 
> statistic for GTT: I just inserting it in backend's catalog cache, but not in 
> pg_statistic table itself.
> To do it I have to add InsertSysCache/InsertCatCache functions which insert 
> pinned entry in the correspondent cache.
> I wonder if there are some pitfalls of such approach?
> 
> New patch for GTT is attached.
> -- 
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com 
> 
> The Russian Postgres Company 
>

Re: [Proposal] Global temporary tables

2019-11-08 Thread Konstantin Knizhnik





On 08.11.2019 10:50, 曾文旌(义从) wrote:

In my opinion, it is not a good idea to trigger a btbuild with a select or DML, 
the cost of which depends on the amount of data in the GTT.

IMHO it is better than returning error.
Also index will be used only if cost of plan with index will be 
considered better than cost of plan without index. If you do not have 
index, then you have to scan the whole table.

Time of such scan is comparable with time of building index.

Yes, I agree that indexes for GTT are used to be created together with 
table itself before it is used by any application.
But if later DBA recognized that efficient execution of queries requires 
some more indexes,
it will be strange and dangerous to prevent him from adding such index 
until all clients which have accessed this table will drop their 
connections.
Also maintaining in shared memory information about attached backends 
seems to be overkill.




This code initializes B-Tree and load data in it when GTT index is access and 
is not initialized yet.
It looks a little bit hacker but it works.

I also wonder why you are keeping information about GTT in shared memory. Looks 
like the only information we really need to share is table's metadata.
But it is already shared though catalog. All other GTT related information is 
private to backend so I do not see reasons to place it in shared memory.

The shared hash structure tracks which backend has initialized the GTT storage 
in order to implement the DDL of the GTT.

Sorry, I do not understand this argument.
DDL is performed on shared metadata present in global catalog.
Standard postgres invalidation mechanism is used to notify all backends 
about schema changes.

Why do we need to maintain some extra information in shared memory.
Can you give me example of DLL which does't work without such shared hash?


As for GTT, there is only one definition(include index on GTT), but each 
backend may have one data.
For the implementation of drop GTT, I assume that all data and definitions need 
to be deleted.


Data of dropped GTT is removed on normal backend termination or cleaned 
up at server restart in case of abnormal shutdown (as it is done for 
local temp tables).
I have not used any shared control structures for GTT in my 
implementation and that is why I wonder why do you need it and what are 
the expected problems with my

implementation?

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Global temporary tables

2019-11-08 Thread Konstantin Knizhnik

_flags |= REINDEX_REL_FORCE_INDEXES_PERMANENT;
 
 	/* Report that we are now reindexing relations */
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index a13322b..be661a4 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -94,7 +94,7 @@ static HTAB *seqhashtab = NULL; /* hash table for SeqTable items */
  */
 static SeqTableData *last_used_seq = NULL;
 
-static void fill_seq_with_data(Relation rel, HeapTuple tuple);
+static void fill_seq_with_data(Relation rel, HeapTuple tuple, Buffer buf);
 static Relation lock_and_open_sequence(SeqTable seq);
 static void create_seq_hashtable(void);
 static void init_sequence(Oid relid, SeqTable *p_elm, Relation *p_rel);
@@ -222,7 +222,7 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
 
 	/* now initialize the sequence's data */
 	tuple = heap_form_tuple(tupDesc, value, null);
-	fill_seq_with_data(rel, tuple);
+	fill_seq_with_data(rel, tuple, InvalidBuffer);
 
 	/* process OWNED BY if given */
 	if (owned_by)
@@ -327,7 +327,7 @@ ResetSequence(Oid seq_relid)
 	/*
 	 * Insert the modified tuple into the new storage file.
 	 */
-	fill_seq_with_data(seq_rel, tuple);
+	fill_seq_with_data(seq_rel, tuple, InvalidBuffer);
 
 	/* Clear local cache so that we don't think we have cached numbers */
 	/* Note that we do not change the currval() state */
@@ -340,18 +340,21 @@ ResetSequence(Oid seq_relid)
  * Initialize a sequence's relation with the specified tuple as content
  */
 static void
-fill_seq_with_data(Relation rel, HeapTuple tuple)
+fill_seq_with_data(Relation rel, HeapTuple tuple, Buffer buf)
 {
-	Buffer		buf;
 	Page		page;
 	sequence_magic *sm;
 	OffsetNumber offnum;
+	bool lockBuffer = false;
 
 	/* Initialize first page of relation with special magic number */
 
-	buf = ReadBuffer(rel, P_NEW);
-	Assert(BufferGetBlockNumber(buf) == 0);
-
+	if (buf == InvalidBuffer)
+	{
+		buf = ReadBuffer(rel, P_NEW);
+		Assert(BufferGetBlockNumber(buf) == 0);
+		lockBuffer = true;
+	}
 	page = BufferGetPage(buf);
 
 	PageInit(page, BufferGetPageSize(buf), sizeof(sequence_magic));
@@ -360,7 +363,8 @@ fill_seq_with_data(Relation rel, HeapTuple tuple)
 
 	/* Now insert sequence tuple */
 
-	LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
+	if (lockBuffer)
+		LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
 
 	/*
 	 * Since VACUUM does not process sequences, we have to force the tuple to
@@ -410,7 +414,8 @@ fill_seq_with_data(Relation rel, HeapTuple tuple)
 
 	END_CRIT_SECTION();
 
-	UnlockReleaseBuffer(buf);
+	if (lockBuffer)
+		UnlockReleaseBuffer(buf);
 }
 
 /*
@@ -502,7 +507,7 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
 		/*
 		 * Insert the modified tuple into the new storage file.
 		 */
-		fill_seq_with_data(seqrel, newdatatuple);
+		fill_seq_with_data(seqrel, newdatatuple, InvalidBuffer);
 	}
 
 	/* process OWNED BY if given */
@@ -1178,6 +1183,17 @@ read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple)
 	LockBuffer(*buf, BUFFER_LOCK_EXCLUSIVE);
 
 	page = BufferGetPage(*buf);
+	if (GlobalTempRelationPageIsNotInitialized(rel, page))
+	{
+		/* Initialize sequence for global temporary tables */
+		Datum		value[SEQ_COL_LASTCOL] = {0};
+		bool		null[SEQ_COL_LASTCOL] = {false};
+		HeapTuple tuple;
+		value[SEQ_COL_LASTVAL-1] = Int64GetDatumFast(1); /* start sequence with 1 */
+		tuple = heap_form_tuple(RelationGetDescr(rel), value, null);
+		fill_seq_with_data(rel, tuple, *buf);
+	}
+
 	sm = (sequence_magic *) PageGetSpecialPointer(page);
 
 	if (sm->magic != SEQ_MAGIC)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8d25d14..50d0402 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -587,7 +587,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 	 * Check consistency of arguments
 	 */
 	if (stmt->oncommit != ONCOMMIT_NOOP
-		&& stmt->relation->relpersistence != RELPERSISTENCE_TEMP)
+		&& !IsLocalRelpersistence(stmt->relation->relpersistence))
 		ereport(ERROR,
 (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
  errmsg("ON COMMIT can only be used on temporary tables")));
@@ -1772,7 +1772,8 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged,
 		 * table or the current physical file to be thrown away anyway.
 		 */
 		if (rel->rd_createSubid == mySubid ||
-			rel->rd_newRelfilenodeSubid == mySubid)
+			rel->rd_newRelfilenodeSubid == mySubid ||
+			rel->rd_rel->relpersistence == RELPERSISTENCE_SESSION)
 		{
 			/* Immediate, non-rollbackable truncation is OK */
 			heap_truncate_one_rel(rel);
@@ -7708,6 +7709,12 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
 		 errmsg("constraints on unlogged tables may reference only permanent or unlogged tables")));
 			break;
+		case RELPERSISTENCE_SESSION:
+			if (pkrel->rd_rel->relpersistenc

Re: [Proposal] Global temporary tables

2019-11-07 Thread Konstantin Knizhnik





On 07.11.2019 12:30, 曾文旌(义从) wrote:



May be the assumption is that all indexes has to be created before GTT start to 
be used.

Yes, Currently, GTT's index is only supported and created in an empty table 
state, and other sessions are not using it.
There has two improvements pointer:
1 Index can create on GTT(A) when the GTT(A)  in the current session is not 
empty, requiring the GTT table to be empty in the other session.
Index_build needs to be done in the current session just like a normal table. 
This improvement is relatively easy.

2 Index can create on GTT(A)  when more than one session are using this GTT(A).
Because when I'm done creating an index of the GTT in this session and setting 
it to be an valid index, it's not true for the GTT in other sessions.
Indexes on gtt in other sessions require "rebuild_index" before using it.
I don't have a better solution right now, maybe you have some suggestions.

It is possible to create index on demand:

Buffer
_bt_getbuf(Relation rel, BlockNumber blkno, int access)
{
    Buffer        buf;

    if (blkno != P_NEW)
    {
        /* Read an existing block of the relation */
        buf = ReadBuffer(rel, blkno);
        /* Session temporary relation may be not yet initialized for 
this backend. */
        if (blkno == BTREE_METAPAGE && 
GlobalTempRelationPageIsNotInitialized(rel, BufferGetPage(buf)))

        {
            Relation heap = RelationIdGetRelation(rel->rd_index->indrelid);
            ReleaseBuffer(buf);
            DropRelFileNodeLocalBuffers(rel->rd_node, MAIN_FORKNUM, blkno);
            btbuild(heap, rel, BuildIndexInfo(rel));
            RelationClose(heap);
            buf = ReadBuffer(rel, blkno);
            LockBuffer(buf, access);
        }
        else
        {
            LockBuffer(buf, access);
            _bt_checkpage(rel, buf);
        }
    }
    ...


This code initializes B-Tree and load data in it when GTT index is 
access and is not initialized yet.

It looks a little bit hacker but it works.

I also wonder why you are keeping information about GTT in shared 
memory. Looks like the only information we really need to share is 
table's metadata.
But it is already shared though catalog. All other GTT related 
information is private to backend so I do not see reasons to place it in 
shared memory.


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [Proposal] Global temporary tables

2019-11-07 Thread Pavel Stehule

čt 7. 11. 2019 v 13:17 odesílatel 曾文旌(义从) 
napsal:

>
>
> 2019年11月7日 下午5:40，Pavel Stehule  写道：
>
>
>
> čt 7. 11. 2019 v 10:30 odesílatel 曾文旌(义从) 
> napsal:
>
>>
>>
>> > 2019年11月7日 上午12:08，Konstantin Knizhnik  写道：
>> >
>> >
>> >
>> > On 06.11.2019 16:24, 曾文旌(义从) wrote:
>> >> Dear Hackers
>> >>
>> >>
>> >> I attached the patch of GTT implementationI base on PG12.
>> >> The GTT design came from my first email.
>> >> Some limitations in patch will be eliminated in later versions.
>> >>
>> >> Later, I will comment on Konstantin's patch and make some proposals
>> for cooperation.
>> >> Looking forward to your feedback.
>> >>
>> >> Thanks.
>> >>
>> >> Zeng Wenjing
>> >>
>> >
>> > Thank you for this patch.
>> > My first comments:
>> >
>> > 1.  I have ported you patch to the latest Postgres version (my patch is
>> attached).
>> > 2. You patch is supporting only B-Tree index for GTT. All other indexes
>> (hash, gin, gist, brin,...) are not currently supported.
>> Currently I only support btree index.
>> I noticed that your patch supports more index types, which is where I'd
>> like to work with you.
>>
>> > 3. I do not understand the reason for the following limitation:
>> > "We allow to create index on global temp table only this session use it"
>> >
>> > First of all it seems to significantly reduce usage of global temp
>> tables.
>> > Why do we need GTT at all? Mostly because we need to access temporary
>> data in more than one backend. Otherwise we can just use normal table.
>> > If temp table is expected to be larger enough, so that we need to
>> create index for it, then it is hard to believe that it will be needed only
>> in one backend.
>> >
>> > May be the assumption is that all indexes has to be created before GTT
>> start to be used.
>> Yes, Currently, GTT's index is only supported and created in an empty
>> table state, and other sessions are not using it.
>> There has two improvements pointer:
>> 1 Index can create on GTT(A) when the GTT(A)  in the current session is
>> not empty, requiring the GTT table to be empty in the other session.
>> Index_build needs to be done in the current session just like a normal
>> table. This improvement is relatively easy.
>>
>> 2 Index can create on GTT(A)  when more than one session are using this
>> GTT(A).
>> Because when I'm done creating an index of the GTT in this session and
>> setting it to be an valid index, it's not true for the GTT in other
>> sessions.
>> Indexes on gtt in other sessions require "rebuild_index" before using it.
>> I don't have a better solution right now, maybe you have some suggestions.
>>
>
> I think so DDL operations can be implemented in some reduced form - so DDL
> are active only for one session, and for other sessions are invisible.
> Important is state of GTT object on session start.
>
> For example ALTER TABLE DROP COLUMN can has very fatal impact on other
> sessions. So I think the best of GTT can be pattern - the structure of GTT
> table is immutable for any session that doesn't do DDL operations.
>
> Yes, Those ddl that need to rewrite data files will have this problem.
> This is why I disabled alter GTT in the current version.
> It can be improved, such as Alter GTT can also be allowed when only the
> current session is in use.
>

I think so it is acceptable solution for some first steps, but I cannot to
imagine so this behave can be good for production usage. But can be good
enough for some time.

Regards

Pavel

Users can also choose to kick off other sessions that are using gtt, then
> do alter GTT.
> I provide a function(pg_gtt_attached_pid(relation, schema)) to query which
> session a GTT is being used by.
>
>
>
>>
>> > But right now this check is not working correctly in any case - if you
>> insert some data into the table, then
>> > you can not create index any more:
>> >
>> > postgres=# create global temp table gtt(x integer primary key, y
>> integer);
>> > CREATE TABLE
>> > postgres=# insert into gtt values (generate_series(1,10),
>> generate_series(1,10));
>> > INSERT 0 10
>> > postgres=# create index on gtt(y);
>> > ERROR:  can not create index when have one or more backend attached
>> this global temp table
>> >
>> > I wonder why do you need such restriction?
>> >
>> >
>> > --
>> > Konstantin Knizhnik
>> > Postgres Professional: http://www.postgrespro.com
>> > The Russian Postgres Company
>> >
>> > 
>>
>>
>

Re: [Proposal] Global temporary tables

2019-11-07 Thread 曾文旌(义从)



> 2019年11月7日 下午5:40，Pavel Stehule  写道：
> 
> 
> 
> čt 7. 11. 2019 v 10:30 odesílatel 曾文旌(义从)  > napsal:
> 
> 
> > 2019年11月7日 上午12:08，Konstantin Knizhnik  > > 写道：
> > 
> > 
> > 
> > On 06.11.2019 16:24, 曾文旌(义从) wrote:
> >> Dear Hackers
> >> 
> >> 
> >> I attached the patch of GTT implementationI base on PG12.
> >> The GTT design came from my first email.
> >> Some limitations in patch will be eliminated in later versions.
> >> 
> >> Later, I will comment on Konstantin's patch and make some proposals for 
> >> cooperation.
> >> Looking forward to your feedback.
> >> 
> >> Thanks.
> >> 
> >> Zeng Wenjing
> >> 
> > 
> > Thank you for this patch.
> > My first comments:
> > 
> > 1.  I have ported you patch to the latest Postgres version (my patch is 
> > attached).
> > 2. You patch is supporting only B-Tree index for GTT. All other indexes 
> > (hash, gin, gist, brin,...) are not currently supported.
> Currently I only support btree index.
> I noticed that your patch supports more index types, which is where I'd like 
> to work with you.
> 
> > 3. I do not understand the reason for the following limitation:
> > "We allow to create index on global temp table only this session use it"
> > 
> > First of all it seems to significantly reduce usage of global temp tables.
> > Why do we need GTT at all? Mostly because we need to access temporary data 
> > in more than one backend. Otherwise we can just use normal table.
> > If temp table is expected to be larger enough, so that we need to create 
> > index for it, then it is hard to believe that it will be needed only in one 
> > backend.
> > 
> > May be the assumption is that all indexes has to be created before GTT 
> > start to be used.
> Yes, Currently, GTT's index is only supported and created in an empty table 
> state, and other sessions are not using it.
> There has two improvements pointer:
> 1 Index can create on GTT(A) when the GTT(A)  in the current session is not 
> empty, requiring the GTT table to be empty in the other session.
> Index_build needs to be done in the current session just like a normal table. 
> This improvement is relatively easy.
> 
> 2 Index can create on GTT(A)  when more than one session are using this 
> GTT(A).
> Because when I'm done creating an index of the GTT in this session and 
> setting it to be an valid index, it's not true for the GTT in other sessions.
> Indexes on gtt in other sessions require "rebuild_index" before using it. 
> I don't have a better solution right now, maybe you have some suggestions.
> 
> I think so DDL operations can be implemented in some reduced form - so DDL 
> are active only for one session, and for other sessions are invisible. 
> Important is state of GTT object on session start. 
> 
> For example ALTER TABLE DROP COLUMN can has very fatal impact on other 
> sessions. So I think the best of GTT can be pattern - the structure of GTT 
> table is immutable for any session that doesn't do DDL operations.
Yes, Those ddl that need to rewrite data files will have this problem.
This is why I disabled alter GTT in the current version.
It can be improved, such as Alter GTT can also be allowed when only the current 
session is in use.
Users can also choose to kick off other sessions that are using gtt, then do 
alter GTT.
I provide a function(pg_gtt_attached_pid(relation, schema)) to query which 
session a GTT is being used by.

> 
> 
> 
> > But right now this check is not working correctly in any case - if you 
> > insert some data into the table, then
> > you can not create index any more:
> > 
> > postgres=# create global temp table gtt(x integer primary key, y integer);
> > CREATE TABLE
> > postgres=# insert into gtt values (generate_series(1,10), 
> > generate_series(1,10));
> > INSERT 0 10
> > postgres=# create index on gtt(y);
> > ERROR:  can not create index when have one or more backend attached this 
> > global temp table
> > 
> > I wonder why do you need such restriction?
> > 
> > 
> > -- 
> > Konstantin Knizhnik
> > Postgres Professional: http://www.postgrespro.com 
> > 
> > The Russian Postgres Company
> > 
> > 
>

Re: [Proposal] Global temporary tables

2019-11-07 Thread Pavel Stehule

čt 7. 11. 2019 v 10:30 odesílatel 曾文旌(义从) 
napsal:

>
>
> > 2019年11月7日 上午12:08，Konstantin Knizhnik  写道：
> >
> >
> >
> > On 06.11.2019 16:24, 曾文旌(义从) wrote:
> >> Dear Hackers
> >>
> >>
> >> I attached the patch of GTT implementationI base on PG12.
> >> The GTT design came from my first email.
> >> Some limitations in patch will be eliminated in later versions.
> >>
> >> Later, I will comment on Konstantin's patch and make some proposals for
> cooperation.
> >> Looking forward to your feedback.
> >>
> >> Thanks.
> >>
> >> Zeng Wenjing
> >>
> >
> > Thank you for this patch.
> > My first comments:
> >
> > 1.  I have ported you patch to the latest Postgres version (my patch is
> attached).
> > 2. You patch is supporting only B-Tree index for GTT. All other indexes
> (hash, gin, gist, brin,...) are not currently supported.
> Currently I only support btree index.
> I noticed that your patch supports more index types, which is where I'd
> like to work with you.
>
> > 3. I do not understand the reason for the following limitation:
> > "We allow to create index on global temp table only this session use it"
> >
> > First of all it seems to significantly reduce usage of global temp
> tables.
> > Why do we need GTT at all? Mostly because we need to access temporary
> data in more than one backend. Otherwise we can just use normal table.
> > If temp table is expected to be larger enough, so that we need to create
> index for it, then it is hard to believe that it will be needed only in one
> backend.
> >
> > May be the assumption is that all indexes has to be created before GTT
> start to be used.
> Yes, Currently, GTT's index is only supported and created in an empty
> table state, and other sessions are not using it.
> There has two improvements pointer:
> 1 Index can create on GTT(A) when the GTT(A)  in the current session is
> not empty, requiring the GTT table to be empty in the other session.
> Index_build needs to be done in the current session just like a normal
> table. This improvement is relatively easy.
>
> 2 Index can create on GTT(A)  when more than one session are using this
> GTT(A).
> Because when I'm done creating an index of the GTT in this session and
> setting it to be an valid index, it's not true for the GTT in other
> sessions.
> Indexes on gtt in other sessions require "rebuild_index" before using it.
> I don't have a better solution right now, maybe you have some suggestions.
>

I think so DDL operations can be implemented in some reduced form - so DDL
are active only for one session, and for other sessions are invisible.
Important is state of GTT object on session start.

For example ALTER TABLE DROP COLUMN can has very fatal impact on other
sessions. So I think the best of GTT can be pattern - the structure of GTT
table is immutable for any session that doesn't do DDL operations.


>
> > But right now this check is not working correctly in any case - if you
> insert some data into the table, then
> > you can not create index any more:
> >
> > postgres=# create global temp table gtt(x integer primary key, y
> integer);
> > CREATE TABLE
> > postgres=# insert into gtt values (generate_series(1,10),
> generate_series(1,10));
> > INSERT 0 10
> > postgres=# create index on gtt(y);
> > ERROR:  can not create index when have one or more backend attached this
> global temp table
> >
> > I wonder why do you need such restriction?
> >
> >
> > --
> > Konstantin Knizhnik
> > Postgres Professional: http://www.postgrespro.com
> > The Russian Postgres Company
> >
> > 
>
>

Re: [Proposal] Global temporary tables

2019-11-07 Thread 曾文旌(义从)




> 2019年11月7日 上午12:08，Konstantin Knizhnik  写道：
> 
> 
> 
> On 06.11.2019 16:24, 曾文旌(义从) wrote:
>> Dear Hackers
>> 
>> 
>> I attached the patch of GTT implementationI base on PG12.
>> The GTT design came from my first email.
>> Some limitations in patch will be eliminated in later versions.
>> 
>> Later, I will comment on Konstantin's patch and make some proposals for 
>> cooperation.
>> Looking forward to your feedback.
>> 
>> Thanks.
>> 
>> Zeng Wenjing
>> 
> 
> Thank you for this patch.
> My first comments:
> 
> 1.  I have ported you patch to the latest Postgres version (my patch is 
> attached).
> 2. You patch is supporting only B-Tree index for GTT. All other indexes 
> (hash, gin, gist, brin,...) are not currently supported.
Currently I only support btree index.
I noticed that your patch supports more index types, which is where I'd like to 
work with you.

> 3. I do not understand the reason for the following limitation:
> "We allow to create index on global temp table only this session use it"
> 
> First of all it seems to significantly reduce usage of global temp tables.
> Why do we need GTT at all? Mostly because we need to access temporary data in 
> more than one backend. Otherwise we can just use normal table.
> If temp table is expected to be larger enough, so that we need to create 
> index for it, then it is hard to believe that it will be needed only in one 
> backend.
> 
> May be the assumption is that all indexes has to be created before GTT start 
> to be used.
Yes, Currently, GTT's index is only supported and created in an empty table 
state, and other sessions are not using it.
There has two improvements pointer:
1 Index can create on GTT(A) when the GTT(A)  in the current session is not 
empty, requiring the GTT table to be empty in the other session.
Index_build needs to be done in the current session just like a normal table. 
This improvement is relatively easy.

2 Index can create on GTT(A)  when more than one session are using this GTT(A).
Because when I'm done creating an index of the GTT in this session and setting 
it to be an valid index, it's not true for the GTT in other sessions.
Indexes on gtt in other sessions require "rebuild_index" before using it. 
I don't have a better solution right now, maybe you have some suggestions.


> But right now this check is not working correctly in any case - if you insert 
> some data into the table, then
> you can not create index any more:
> 
> postgres=# create global temp table gtt(x integer primary key, y integer);
> CREATE TABLE
> postgres=# insert into gtt values (generate_series(1,10), 
> generate_series(1,10));
> INSERT 0 10
> postgres=# create index on gtt(y);
> ERROR:  can not create index when have one or more backend attached this 
> global temp table
> 
> I wonder why do you need such restriction?
> 
> 
> -- 
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
> 
>

Re: [Proposal] Global temporary tables

2019-11-06 Thread Konstantin Knizhnik

use it */
+	if (RELATION_IS_GLOBAL_TEMP(rel))
+	{
+		if (is_other_backend_use_gtt(rel->rd_node))
+			elog(ERROR, "can not drop relation when other backend attached this global temp table");
+	}
+
 	/*
 	 * Schedule unlinking of the relation's physical files at commit.
 	 */
@@ -3132,9 +3161,10 @@ RemoveStatistics(Oid relid, AttrNumber attnum)
  *
  * The routine will truncate and then reconstruct the indexes on
  * the specified relation.  Caller must hold exclusive lock on rel.
+ *
  */
 static void
-RelationTruncateIndexes(Relation heapRelation)
+RelationTruncateIndexes(Relation heapRelation, LOCKMODE lockmode)
 {
 	ListCell   *indlist;
 
@@ -3146,7 +3176,7 @@ RelationTruncateIndexes(Relation heapRelation)
 		IndexInfo  *indexInfo;
 
 		/* Open the index relation; use exclusive lock, just to be sure */
-		currentIndex = index_open(indexId, AccessExclusiveLock);
+		currentIndex = index_open(indexId, lockmode);
 
 		/* Fetch info needed for index_build */
 		indexInfo = BuildIndexInfo(currentIndex);
@@ -3185,8 +3215,13 @@ heap_truncate(List *relids)
 	{
 		Oid			rid = lfirst_oid(cell);
 		Relation	rel;
+		LOCKMODE	lockmode = AccessExclusiveLock;
 
-		rel = table_open(rid, AccessExclusiveLock);
+		/* truncate global temp table only need RowExclusiveLock */
+		if (get_rel_persistence(rid) == RELPERSISTENCE_GLOBAL_TEMP)
+			lockmode = RowExclusiveLock;
+
+		rel = table_open(rid, lockmode);
 		relations = lappend(relations, rel);
 	}
 
@@ -3219,6 +3254,8 @@ void
 heap_truncate_one_rel(Relation rel)
 {
 	Oid			toastrelid;
+	LOCKMODE	lockmode = AccessExclusiveLock;
+	bool		truncate_toastrel = false;
 
 	/*
 	 * Truncate the relation.  Partitioned tables have no storage, so there is
@@ -3227,23 +3264,40 @@ heap_truncate_one_rel(Relation rel)
 	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 		return;
 
+	toastrelid = rel->rd_rel->reltoastrelid;
+
+	/*
+	 * Truncate global temp table only need RowExclusiveLock
+	 */
+	if (RELATION_IS_GLOBAL_TEMP(rel))
+	{
+		lockmode = RowExclusiveLock;
+		if (OidIsValid(toastrelid) &&
+			gtt_storage_attached(toastrelid))
+			truncate_toastrel = true;
+	}
+	else if (OidIsValid(toastrelid))
+		truncate_toastrel = true;
+
 	/* Truncate the underlying relation */
 	table_relation_nontransactional_truncate(rel);
 
 	/* If the relation has indexes, truncate the indexes too */
-	RelationTruncateIndexes(rel);
+	RelationTruncateIndexes(rel, lockmode);
 
 	/* If there is a toast table, truncate that too */
-	toastrelid = rel->rd_rel->reltoastrelid;
-	if (OidIsValid(toastrelid))
+	if (truncate_toastrel)
 	{
-		Relation	toastrel = table_open(toastrelid, AccessExclusiveLock);
+		Relation	toastrel = table_open(toastrelid, lockmode);
 
 		table_relation_nontransactional_truncate(toastrel);
-		RelationTruncateIndexes(toastrel);
+		RelationTruncateIndexes(toastrel, lockmode);
 		/* keep the lock... */
 		table_close(toastrel, NoLock);
 	}
+
+	if (RELATION_IS_GLOBAL_TEMP(rel))
+		up_gtt_relstats(rel, 0, 0, 0, RecentXmin, InvalidMultiXactId);
 }
 
 /*
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 7c34509..f623626 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -52,6 +52,7 @@
 #include "catalog/pg_trigger.h"
 #include "catalog/pg_type.h"
 #include "catalog/storage.h"
+#include "catalog/storage_gtt.h"
 #include "commands/event_trigger.h"
 #include "commands/progress.h"
 #include "commands/tablecmds.h"
@@ -879,6 +880,26 @@ index_create(Relation heapRelation,
 		indexRelationName, RelationGetRelationName(heapRelation;
 	}
 
+	if (RELATION_IS_GLOBAL_TEMP(heapRelation))
+	{
+		if (accessMethodObjectId != BTREE_AM_OID)
+			elog(ERROR, "only support btree index on global temp table");
+
+		/* We allow to create index on global temp table only this session use it */
+		if (is_other_backend_use_gtt(heapRelation->rd_node) ||
+			gtt_storage_attached(heapRelation->rd_node.relNode))
+			elog(ERROR, "can not create index when have one or more backend attached this global temp table");
+
+		/* No support create index on global temp table use concurrent mode yet */
+		if (concurrent)
+			ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("cannot create indexes on global temporary tables using concurrent mode")));
+
+		/* if global temp table not init storage, then skip build index */
+		flags |= INDEX_CREATE_SKIP_BUILD;
+	}
+
 	/*
 	 * construct tuple descriptor for index tuples
 	 */
@@ -2047,6 +2068,13 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode)
 	 */
 	CheckTableNotInUse(userIndexRelation, "DROP INDEX");
 
+	/* We allow to drop index on global temp table only this session use it */
+	if (RELATION_IS_GLOBAL_TEMP(userHeapRelation))
+	{
+		if (is_other_backend_use_gtt(userHeapRelation->rd_node))
+			elog(ERROR, &q

Re: [Proposal] Global temporary tables

2019-11-02 Thread Pavel Stehule

And pg_catalog.pg_statistics_gtt() is set returning functions?
>

yes

I afraid that it is not acceptable solution from performance point of view:
> pg_statictic table is accessed by keys (,,)
>

I don't think so it is problem. The any component, that needs to use fast
access can use some special function that check index or check some memory
buffers.


If it can not be done using index scan, then it can cause significant
> performance slow down.
>

where you need fast access when you use SQL access? Inside postgres
optimizer is caches everywhere. And statistics cache should to know so have
to check index and some memory buffers.

The proposed view will not be used by optimizer, but it can be used by some
higher layers. I think so there is a agreement so GTT metadata should not
be stored in system catalogue. If are stored in some syscache or somewhere
else is not important in this moment. But can be nice if for user the GTT
metadata should not be black hole. I think so is better to change some
current tables to views, than use some special function just specialized
for GTT (these functions should to exists in both variants). When I think
about it - this is important not just for functionality that we expect from
GTT. It is important for consistency of Postgres catalog - how much
different should be GTT than other types of tables in system catalogue from
user's perspective.

Re: [Proposal] Global temporary tables

2019-11-02 Thread Julien Rouhaud

On Sat, Nov 2, 2019 at 4:09 PM Konstantin Knizhnik
 wrote:
>
> On 02.11.2019 10:19, Julien Rouhaud wrote:
> > On Sat, Nov 2, 2019 at 6:31 AM Pavel Stehule  
> > wrote:
> >> pá 1. 11. 2019 v 17:09 odesílatel Konstantin Knizhnik 
> >>  napsal:
> >>> On 01.11.2019 18:26, Robert Haas wrote:
>  On Fri, Nov 1, 2019 at 11:15 AM Konstantin Knizhnik
>   wrote:
> > It seems to me that I have found quite elegant solution for per-backend 
> > statistic for GTT: I just inserting it in backend's catalog cache, but 
> > not in pg_statistic table itself.
> > To do it I have to add InsertSysCache/InsertCatCache functions which 
> > insert pinned entry in the correspondent cache.
> > I wonder if there are some pitfalls of such approach?
>  That sounds pretty hackish. You'd have to be very careful, for
>  example, that if the tables were dropped or re-analyzed, all of the
>  old entries got removed --
> >>> I have checked it:
> >>> - when table is reanalyzed, then cache entries are replaced.
> >>> - when table is dropped, then cache entries are removed.
> >>>
>  and then it would still fail if any code
>  tried to access the statistics directly from the table, rather than
>  via the caches. My assumption is that the statistics ought to be
>  stored in some backend-private data structure designed for that
>  purpose, and that the code that needs the data should be taught to
>  look for it there when the table is a GTT.
> >>> Yes, if you do "select * from pg_statistic" then you will not see
> >>> statistic for GTT in this case.
> >>> But I do not think that it is so critical. I do not believe that anybody
> >>> is trying to manually interpret values in this table.
> >>> And optimizer is retrieving statistic through sys-cache mechanism and so
> >>> is able to build correct plan in this case.
> >>
> >> Years ago, when I though about it, I wrote patch with similar design. It's 
> >> working, but surely it's ugly.
> >>
> >> I have another idea. Can be pg_statistics view instead a table?
> >>
> >> Some like
> >>
> >> SELECT * FROM pg_catalog.pg_statistics_rel
> >> UNION ALL
> >> SELECT * FROM pg_catalog.pg_statistics_gtt();
> >>
> >> Internally - when stat cache is filled, then there can be used 
> >> pg_statistics_rel and pg_statistics_gtt() directly. What I remember, there 
> >> was not possibility to work with queries, only with just relations.
> > It'd be a loss if you lose the ability to see the statistics, as there
> > are valid use cases where you need to see the stats, eg. understanding
> > why you don't get the plan you wanted.  There's also at least one
> > extension [1] that allows you to backup and use restored statistics,
> > so there are definitely people interested in it.
> >
> > [1]: https://github.com/ossc-db/pg_dbms_stats
> It seems to have completely no sense to backup and restore statistic for
> temporary tables which life time is limited to life time of backend,
> doesn't it?

In general yes I agree, but it doesn't if the goal is to understand
why even after an analyze on the temporary table your query is still
behaving poorly.  It can be useful to allow reproduction or just give
someone else the statistics to see what's going on.

Re: [Proposal] Global temporary tables

2019-11-02 Thread Konstantin Knizhnik

On 02.11.2019 8:30, Pavel Stehule wrote:

pá 1. 11. 2019 v 17:09 odesílatel Konstantin Knizhnik 
mailto:k.knizh...@postgrespro.ru>> napsal:

On 01.11.2019 18:26, Robert Haas wrote:
> On Fri, Nov 1, 2019 at 11:15 AM Konstantin Knizhnik
> mailto:k.knizh...@postgrespro.ru>>
wrote:
>> It seems to me that I have found quite elegant solution for
per-backend statistic for GTT: I just inserting it in backend's
catalog cache, but not in pg_statistic table itself.
>> To do it I have to add InsertSysCache/InsertCatCache functions
which insert pinned entry in the correspondent cache.
>> I wonder if there are some pitfalls of such approach?
> That sounds pretty hackish. You'd have to be very careful, for
> example, that if the tables were dropped or re-analyzed, all of the
> old entries got removed --

I have checked it:
- when table is reanalyzed, then cache entries are replaced.
- when table is dropped, then cache entries are removed.

> and then it would still fail if any code
> tried to access the statistics directly from the table, rather than
> via the caches. My assumption is that the statistics ought to be
> stored in some backend-private data structure designed for that
> purpose, and that the code that needs the data should be taught to
> look for it there when the table is a GTT.

Yes, if you do "select * from pg_statistic" then you will not see
statistic for GTT in this case.
But I do not think that it is so critical. I do not believe that
anybody
is trying to manually interpret values in this table.
And optimizer is retrieving statistic through sys-cache mechanism
and so
is able to build correct plan in this case.

Years ago, when I though about it, I wrote patch with similar design. 
It's working, but surely it's ugly.

I have another idea. Can be pg_statistics view instead a table?

Some like

SELECT * FROM pg_catalog.pg_statistics_rel
UNION ALL
SELECT * FROM pg_catalog.pg_statistics_gtt();

And pg_catalog.pg_statistics_gtt() is set returning functions?
I afraid that it is not acceptable solution from performance point of 
view: pg_statictic table is accessed by keys (,,)
If it can not be done using index scan, then it can cause significant 
performance slow down.

Internally - when stat cache is filled, then there can be used 
pg_statistics_rel and pg_statistics_gtt() directly. What I remember, 
there was not possibility to work with queries, only with just relations.

Or crazy idea - today we can implement own types of heaps. Is possible 
to create engine where result can be combination of some shared data 
and local data. So union will be implemented on heap level.
This implementation can be simple, just scanning pages from shared 
buffers and from local buffers. For these tables we don't need complex 
metadata. It's crazy idea, and I think so union with table function 
should be best.

Frankly speaking, implementing special heap access method for 
pg_statistic just to handle case of global temp tables seems to be overkill
from my point of view. It requires a lot coding (or at least copying a 
lot of code from heapam). Also, as I wrote above, we need also index for 
efficient lookup of statistic.

Re: [Proposal] Global temporary tables

2019-11-02 Thread Konstantin Knizhnik





On 02.11.2019 10:19, Julien Rouhaud wrote:

On Sat, Nov 2, 2019 at 6:31 AM Pavel Stehule  wrote:

pá 1. 11. 2019 v 17:09 odesílatel Konstantin Knizhnik 
 napsal:

On 01.11.2019 18:26, Robert Haas wrote:

On Fri, Nov 1, 2019 at 11:15 AM Konstantin Knizhnik
 wrote:

It seems to me that I have found quite elegant solution for per-backend 
statistic for GTT: I just inserting it in backend's catalog cache, but not in 
pg_statistic table itself.
To do it I have to add InsertSysCache/InsertCatCache functions which insert 
pinned entry in the correspondent cache.
I wonder if there are some pitfalls of such approach?

That sounds pretty hackish. You'd have to be very careful, for
example, that if the tables were dropped or re-analyzed, all of the
old entries got removed --

I have checked it:
- when table is reanalyzed, then cache entries are replaced.
- when table is dropped, then cache entries are removed.


and then it would still fail if any code
tried to access the statistics directly from the table, rather than
via the caches. My assumption is that the statistics ought to be
stored in some backend-private data structure designed for that
purpose, and that the code that needs the data should be taught to
look for it there when the table is a GTT.

Yes, if you do "select * from pg_statistic" then you will not see
statistic for GTT in this case.
But I do not think that it is so critical. I do not believe that anybody
is trying to manually interpret values in this table.
And optimizer is retrieving statistic through sys-cache mechanism and so
is able to build correct plan in this case.


Years ago, when I though about it, I wrote patch with similar design. It's 
working, but surely it's ugly.

I have another idea. Can be pg_statistics view instead a table?

Some like

SELECT * FROM pg_catalog.pg_statistics_rel
UNION ALL
SELECT * FROM pg_catalog.pg_statistics_gtt();

Internally - when stat cache is filled, then there can be used 
pg_statistics_rel and pg_statistics_gtt() directly. What I remember, there was 
not possibility to work with queries, only with just relations.

It'd be a loss if you lose the ability to see the statistics, as there
are valid use cases where you need to see the stats, eg. understanding
why you don't get the plan you wanted.  There's also at least one
extension [1] that allows you to backup and use restored statistics,
so there are definitely people interested in it.

[1]: https://github.com/ossc-db/pg_dbms_stats
It seems to have completely no sense to backup and restore statistic for 
temporary tables which life time is limited to life time of backend,

doesn't it?

Re: [Proposal] Global temporary tables

2019-11-02 Thread Julien Rouhaud

On Sat, Nov 2, 2019 at 8:23 AM Pavel Stehule  wrote:
>
> so 2. 11. 2019 v 8:18 odesílatel Julien Rouhaud  napsal:
>>
>> On Sat, Nov 2, 2019 at 6:31 AM Pavel Stehule  wrote:
>> >
>> > pá 1. 11. 2019 v 17:09 odesílatel Konstantin Knizhnik 
>> >  napsal:
>> >>
>> >> On 01.11.2019 18:26, Robert Haas wrote:
>> >> > On Fri, Nov 1, 2019 at 11:15 AM Konstantin Knizhnik
>> >> >  wrote:
>> >> >> It seems to me that I have found quite elegant solution for 
>> >> >> per-backend statistic for GTT: I just inserting it in backend's 
>> >> >> catalog cache, but not in pg_statistic table itself.
>> >> >> To do it I have to add InsertSysCache/InsertCatCache functions which 
>> >> >> insert pinned entry in the correspondent cache.
>> >> >> I wonder if there are some pitfalls of such approach?
>> >> > That sounds pretty hackish. You'd have to be very careful, for
>> >> > example, that if the tables were dropped or re-analyzed, all of the
>> >> > old entries got removed --
>> >>
>> >> I have checked it:
>> >> - when table is reanalyzed, then cache entries are replaced.
>> >> - when table is dropped, then cache entries are removed.
>> >>
>> >> > and then it would still fail if any code
>> >> > tried to access the statistics directly from the table, rather than
>> >> > via the caches. My assumption is that the statistics ought to be
>> >> > stored in some backend-private data structure designed for that
>> >> > purpose, and that the code that needs the data should be taught to
>> >> > look for it there when the table is a GTT.
>> >>
>> >> Yes, if you do "select * from pg_statistic" then you will not see
>> >> statistic for GTT in this case.
>> >> But I do not think that it is so critical. I do not believe that anybody
>> >> is trying to manually interpret values in this table.
>> >> And optimizer is retrieving statistic through sys-cache mechanism and so
>> >> is able to build correct plan in this case.
>> >
>> >
>> > Years ago, when I though about it, I wrote patch with similar design. It's 
>> > working, but surely it's ugly.
>> >
>> > I have another idea. Can be pg_statistics view instead a table?
>> >
>> > Some like
>> >
>> > SELECT * FROM pg_catalog.pg_statistics_rel
>> > UNION ALL
>> > SELECT * FROM pg_catalog.pg_statistics_gtt();
>> >
>> > Internally - when stat cache is filled, then there can be used 
>> > pg_statistics_rel and pg_statistics_gtt() directly. What I remember, there 
>> > was not possibility to work with queries, only with just relations.
>>
>> It'd be a loss if you lose the ability to see the statistics, as there
>> are valid use cases where you need to see the stats, eg. understanding
>> why you don't get the plan you wanted.  There's also at least one
>> extension [1] that allows you to backup and use restored statistics,
>> so there are definitely people interested in it.
>>
>> [1]: https://github.com/ossc-db/pg_dbms_stats
>
>
> I don't think - the extensions can use UNION and the content will be same as 
> caches used by planner.

Yes, I agree that changing pg_statistics to be a view as you showed
would fix the problem.  I was answering Konstantin's point:

>> >> But I do not think that it is so critical. I do not believe that anybody
>> >> is trying to manually interpret values in this table.
>> >> And optimizer is retrieving statistic through sys-cache mechanism and so
>> >> is able to build correct plan in this case.

which is IMHO a wrong assumption.

Re: [Proposal] Global temporary tables

2019-11-02 Thread Pavel Stehule

so 2. 11. 2019 v 8:23 odesílatel Pavel Stehule 
napsal:

>
>
> so 2. 11. 2019 v 8:18 odesílatel Julien Rouhaud 
> napsal:
>
>> On Sat, Nov 2, 2019 at 6:31 AM Pavel Stehule 
>> wrote:
>> >
>> > pá 1. 11. 2019 v 17:09 odesílatel Konstantin Knizhnik <
>> k.knizh...@postgrespro.ru> napsal:
>> >>
>> >> On 01.11.2019 18:26, Robert Haas wrote:
>> >> > On Fri, Nov 1, 2019 at 11:15 AM Konstantin Knizhnik
>> >> >  wrote:
>> >> >> It seems to me that I have found quite elegant solution for
>> per-backend statistic for GTT: I just inserting it in backend's catalog
>> cache, but not in pg_statistic table itself.
>> >> >> To do it I have to add InsertSysCache/InsertCatCache functions
>> which insert pinned entry in the correspondent cache.
>> >> >> I wonder if there are some pitfalls of such approach?
>> >> > That sounds pretty hackish. You'd have to be very careful, for
>> >> > example, that if the tables were dropped or re-analyzed, all of the
>> >> > old entries got removed --
>> >>
>> >> I have checked it:
>> >> - when table is reanalyzed, then cache entries are replaced.
>> >> - when table is dropped, then cache entries are removed.
>> >>
>> >> > and then it would still fail if any code
>> >> > tried to access the statistics directly from the table, rather than
>> >> > via the caches. My assumption is that the statistics ought to be
>> >> > stored in some backend-private data structure designed for that
>> >> > purpose, and that the code that needs the data should be taught to
>> >> > look for it there when the table is a GTT.
>> >>
>> >> Yes, if you do "select * from pg_statistic" then you will not see
>> >> statistic for GTT in this case.
>> >> But I do not think that it is so critical. I do not believe that
>> anybody
>> >> is trying to manually interpret values in this table.
>> >> And optimizer is retrieving statistic through sys-cache mechanism and
>> so
>> >> is able to build correct plan in this case.
>> >
>> >
>> > Years ago, when I though about it, I wrote patch with similar design.
>> It's working, but surely it's ugly.
>> >
>> > I have another idea. Can be pg_statistics view instead a table?
>> >
>> > Some like
>> >
>> > SELECT * FROM pg_catalog.pg_statistics_rel
>> > UNION ALL
>> > SELECT * FROM pg_catalog.pg_statistics_gtt();
>> >
>> > Internally - when stat cache is filled, then there can be used
>> pg_statistics_rel and pg_statistics_gtt() directly. What I remember, there
>> was not possibility to work with queries, only with just relations.
>>
>> It'd be a loss if you lose the ability to see the statistics, as there
>> are valid use cases where you need to see the stats, eg. understanding
>> why you don't get the plan you wanted.  There's also at least one
>> extension [1] that allows you to backup and use restored statistics,
>> so there are definitely people interested in it.
>>
>> [1]: https://github.com/ossc-db/pg_dbms_stats
>
>
> I don't think - the extensions can use UNION and the content will be same
> as caches used by planner.
>

sure, if some one try to modify directly system tables, then it should be
fixed.

Re: [Proposal] Global temporary tables

2019-11-02 Thread Pavel Stehule

so 2. 11. 2019 v 8:18 odesílatel Julien Rouhaud  napsal:

> On Sat, Nov 2, 2019 at 6:31 AM Pavel Stehule 
> wrote:
> >
> > pá 1. 11. 2019 v 17:09 odesílatel Konstantin Knizhnik <
> k.knizh...@postgrespro.ru> napsal:
> >>
> >> On 01.11.2019 18:26, Robert Haas wrote:
> >> > On Fri, Nov 1, 2019 at 11:15 AM Konstantin Knizhnik
> >> >  wrote:
> >> >> It seems to me that I have found quite elegant solution for
> per-backend statistic for GTT: I just inserting it in backend's catalog
> cache, but not in pg_statistic table itself.
> >> >> To do it I have to add InsertSysCache/InsertCatCache functions which
> insert pinned entry in the correspondent cache.
> >> >> I wonder if there are some pitfalls of such approach?
> >> > That sounds pretty hackish. You'd have to be very careful, for
> >> > example, that if the tables were dropped or re-analyzed, all of the
> >> > old entries got removed --
> >>
> >> I have checked it:
> >> - when table is reanalyzed, then cache entries are replaced.
> >> - when table is dropped, then cache entries are removed.
> >>
> >> > and then it would still fail if any code
> >> > tried to access the statistics directly from the table, rather than
> >> > via the caches. My assumption is that the statistics ought to be
> >> > stored in some backend-private data structure designed for that
> >> > purpose, and that the code that needs the data should be taught to
> >> > look for it there when the table is a GTT.
> >>
> >> Yes, if you do "select * from pg_statistic" then you will not see
> >> statistic for GTT in this case.
> >> But I do not think that it is so critical. I do not believe that anybody
> >> is trying to manually interpret values in this table.
> >> And optimizer is retrieving statistic through sys-cache mechanism and so
> >> is able to build correct plan in this case.
> >
> >
> > Years ago, when I though about it, I wrote patch with similar design.
> It's working, but surely it's ugly.
> >
> > I have another idea. Can be pg_statistics view instead a table?
> >
> > Some like
> >
> > SELECT * FROM pg_catalog.pg_statistics_rel
> > UNION ALL
> > SELECT * FROM pg_catalog.pg_statistics_gtt();
> >
> > Internally - when stat cache is filled, then there can be used
> pg_statistics_rel and pg_statistics_gtt() directly. What I remember, there
> was not possibility to work with queries, only with just relations.
>
> It'd be a loss if you lose the ability to see the statistics, as there
> are valid use cases where you need to see the stats, eg. understanding
> why you don't get the plan you wanted.  There's also at least one
> extension [1] that allows you to backup and use restored statistics,
> so there are definitely people interested in it.
>
> [1]: https://github.com/ossc-db/pg_dbms_stats


I don't think - the extensions can use UNION and the content will be same
as caches used by planner.

Re: [Proposal] Global temporary tables

2019-11-02 Thread Julien Rouhaud

On Sat, Nov 2, 2019 at 6:31 AM Pavel Stehule  wrote:
>
> pá 1. 11. 2019 v 17:09 odesílatel Konstantin Knizhnik 
>  napsal:
>>
>> On 01.11.2019 18:26, Robert Haas wrote:
>> > On Fri, Nov 1, 2019 at 11:15 AM Konstantin Knizhnik
>> >  wrote:
>> >> It seems to me that I have found quite elegant solution for per-backend 
>> >> statistic for GTT: I just inserting it in backend's catalog cache, but 
>> >> not in pg_statistic table itself.
>> >> To do it I have to add InsertSysCache/InsertCatCache functions which 
>> >> insert pinned entry in the correspondent cache.
>> >> I wonder if there are some pitfalls of such approach?
>> > That sounds pretty hackish. You'd have to be very careful, for
>> > example, that if the tables were dropped or re-analyzed, all of the
>> > old entries got removed --
>>
>> I have checked it:
>> - when table is reanalyzed, then cache entries are replaced.
>> - when table is dropped, then cache entries are removed.
>>
>> > and then it would still fail if any code
>> > tried to access the statistics directly from the table, rather than
>> > via the caches. My assumption is that the statistics ought to be
>> > stored in some backend-private data structure designed for that
>> > purpose, and that the code that needs the data should be taught to
>> > look for it there when the table is a GTT.
>>
>> Yes, if you do "select * from pg_statistic" then you will not see
>> statistic for GTT in this case.
>> But I do not think that it is so critical. I do not believe that anybody
>> is trying to manually interpret values in this table.
>> And optimizer is retrieving statistic through sys-cache mechanism and so
>> is able to build correct plan in this case.
>
>
> Years ago, when I though about it, I wrote patch with similar design. It's 
> working, but surely it's ugly.
>
> I have another idea. Can be pg_statistics view instead a table?
>
> Some like
>
> SELECT * FROM pg_catalog.pg_statistics_rel
> UNION ALL
> SELECT * FROM pg_catalog.pg_statistics_gtt();
>
> Internally - when stat cache is filled, then there can be used 
> pg_statistics_rel and pg_statistics_gtt() directly. What I remember, there 
> was not possibility to work with queries, only with just relations.

It'd be a loss if you lose the ability to see the statistics, as there
are valid use cases where you need to see the stats, eg. understanding
why you don't get the plan you wanted.  There's also at least one
extension [1] that allows you to backup and use restored statistics,
so there are definitely people interested in it.

[1]: https://github.com/ossc-db/pg_dbms_stats

Re: [Proposal] Global temporary tables

2019-11-01 Thread Pavel Stehule

pá 1. 11. 2019 v 17:09 odesílatel Konstantin Knizhnik <
k.knizh...@postgrespro.ru> napsal:

>
>
> On 01.11.2019 18:26, Robert Haas wrote:
> > On Fri, Nov 1, 2019 at 11:15 AM Konstantin Knizhnik
> >  wrote:
> >> It seems to me that I have found quite elegant solution for per-backend
> statistic for GTT: I just inserting it in backend's catalog cache, but not
> in pg_statistic table itself.
> >> To do it I have to add InsertSysCache/InsertCatCache functions which
> insert pinned entry in the correspondent cache.
> >> I wonder if there are some pitfalls of such approach?
> > That sounds pretty hackish. You'd have to be very careful, for
> > example, that if the tables were dropped or re-analyzed, all of the
> > old entries got removed --
>
> I have checked it:
> - when table is reanalyzed, then cache entries are replaced.
> - when table is dropped, then cache entries are removed.
>
> > and then it would still fail if any code
> > tried to access the statistics directly from the table, rather than
> > via the caches. My assumption is that the statistics ought to be
> > stored in some backend-private data structure designed for that
> > purpose, and that the code that needs the data should be taught to
> > look for it there when the table is a GTT.
>
> Yes, if you do "select * from pg_statistic" then you will not see
> statistic for GTT in this case.
> But I do not think that it is so critical. I do not believe that anybody
> is trying to manually interpret values in this table.
> And optimizer is retrieving statistic through sys-cache mechanism and so
> is able to build correct plan in this case.
>

Years ago, when I though about it, I wrote patch with similar design. It's
working, but surely it's ugly.

I have another idea. Can be pg_statistics view instead a table?

Some like

SELECT * FROM pg_catalog.pg_statistics_rel
UNION ALL
SELECT * FROM pg_catalog.pg_statistics_gtt();

Internally - when stat cache is filled, then there can be used
pg_statistics_rel and pg_statistics_gtt() directly. What I remember, there
was not possibility to work with queries, only with just relations.

Or crazy idea - today we can implement own types of heaps. Is possible to
create engine where result can be combination of some shared data and local
data. So union will be implemented on heap level.
This implementation can be simple, just scanning pages from shared buffers
and from local buffers. For these tables we don't need complex metadata.
It's crazy idea, and I think so union with table function should be best.

Regards

Pavel





> --
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>

Re: [Proposal] Global temporary tables

2019-11-01 Thread Konstantin Knizhnik





On 01.11.2019 18:26, Robert Haas wrote:

On Fri, Nov 1, 2019 at 11:15 AM Konstantin Knizhnik
 wrote:

It seems to me that I have found quite elegant solution for per-backend 
statistic for GTT: I just inserting it in backend's catalog cache, but not in 
pg_statistic table itself.
To do it I have to add InsertSysCache/InsertCatCache functions which insert 
pinned entry in the correspondent cache.
I wonder if there are some pitfalls of such approach?

That sounds pretty hackish. You'd have to be very careful, for
example, that if the tables were dropped or re-analyzed, all of the
old entries got removed --


I have checked it:
- when table is reanalyzed, then cache entries are replaced.
- when table is dropped, then cache entries are removed.


and then it would still fail if any code
tried to access the statistics directly from the table, rather than
via the caches. My assumption is that the statistics ought to be
stored in some backend-private data structure designed for that
purpose, and that the code that needs the data should be taught to
look for it there when the table is a GTT.


Yes, if you do "select * from pg_statistic" then you will not see 
statistic for GTT in this case.
But I do not think that it is so critical. I do not believe that anybody 
is trying to manually interpret values in this table.
And optimizer is retrieving statistic through sys-cache mechanism and so 
is able to build correct plan in this case.


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [Proposal] Global temporary tables

2019-11-01 Thread Robert Haas

On Fri, Nov 1, 2019 at 11:15 AM Konstantin Knizhnik
 wrote:
> It seems to me that I have found quite elegant solution for per-backend 
> statistic for GTT: I just inserting it in backend's catalog cache, but not in 
> pg_statistic table itself.
> To do it I have to add InsertSysCache/InsertCatCache functions which insert 
> pinned entry in the correspondent cache.
> I wonder if there are some pitfalls of such approach?

That sounds pretty hackish. You'd have to be very careful, for
example, that if the tables were dropped or re-analyzed, all of the
old entries got removed -- and then it would still fail if any code
tried to access the statistics directly from the table, rather than
via the caches. My assumption is that the statistics ought to be
stored in some backend-private data structure designed for that
purpose, and that the code that needs the data should be taught to
look for it there when the table is a GTT.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [Proposal] Global temporary tables

2019-11-01 Thread Konstantin Knizhnik

etBlockNumber(buf) == 0);
-
+	if (buf == InvalidBuffer)
+	{
+		buf = ReadBuffer(rel, P_NEW);
+		Assert(BufferGetBlockNumber(buf) == 0);
+		lockBuffer = true;
+	}
 	page = BufferGetPage(buf);
 
 	PageInit(page, BufferGetPageSize(buf), sizeof(sequence_magic));
@@ -360,7 +363,8 @@ fill_seq_with_data(Relation rel, HeapTuple tuple)
 
 	/* Now insert sequence tuple */
 
-	LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
+	if (lockBuffer)
+		LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
 
 	/*
 	 * Since VACUUM does not process sequences, we have to force the tuple to
@@ -410,7 +414,8 @@ fill_seq_with_data(Relation rel, HeapTuple tuple)
 
 	END_CRIT_SECTION();
 
-	UnlockReleaseBuffer(buf);
+	if (lockBuffer)
+		UnlockReleaseBuffer(buf);
 }
 
 /*
@@ -502,7 +507,7 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
 		/*
 		 * Insert the modified tuple into the new storage file.
 		 */
-		fill_seq_with_data(seqrel, newdatatuple);
+		fill_seq_with_data(seqrel, newdatatuple, InvalidBuffer);
 	}
 
 	/* process OWNED BY if given */
@@ -1178,6 +1183,17 @@ read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple)
 	LockBuffer(*buf, BUFFER_LOCK_EXCLUSIVE);
 
 	page = BufferGetPage(*buf);
+	if (GlobalTempRelationPageIsNotInitialized(rel, page))
+	{
+		/* Initialize sequence for global temporary tables */
+		Datum		value[SEQ_COL_LASTCOL] = {0};
+		bool		null[SEQ_COL_LASTCOL] = {false};
+		HeapTuple tuple;
+		value[SEQ_COL_LASTVAL-1] = Int64GetDatumFast(1); /* start sequence with 1 */
+		tuple = heap_form_tuple(RelationGetDescr(rel), value, null);
+		fill_seq_with_data(rel, tuple, *buf);
+	}
+
 	sm = (sequence_magic *) PageGetSpecialPointer(page);
 
 	if (sm->magic != SEQ_MAGIC)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8d25d14..50d0402 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -587,7 +587,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 	 * Check consistency of arguments
 	 */
 	if (stmt->oncommit != ONCOMMIT_NOOP
-		&& stmt->relation->relpersistence != RELPERSISTENCE_TEMP)
+		&& !IsLocalRelpersistence(stmt->relation->relpersistence))
 		ereport(ERROR,
 (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
  errmsg("ON COMMIT can only be used on temporary tables")));
@@ -1772,7 +1772,8 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged,
 		 * table or the current physical file to be thrown away anyway.
 		 */
 		if (rel->rd_createSubid == mySubid ||
-			rel->rd_newRelfilenodeSubid == mySubid)
+			rel->rd_newRelfilenodeSubid == mySubid ||
+			rel->rd_rel->relpersistence == RELPERSISTENCE_SESSION)
 		{
 			/* Immediate, non-rollbackable truncation is OK */
 			heap_truncate_one_rel(rel);
@@ -7708,6 +7709,12 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
 		 errmsg("constraints on unlogged tables may reference only permanent or unlogged tables")));
 			break;
+		case RELPERSISTENCE_SESSION:
+			if (pkrel->rd_rel->relpersistence != RELPERSISTENCE_SESSION)
+ereport(ERROR,
+		(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+		 errmsg("constraints on session tables may reference only session tables")));
+			break;
 		case RELPERSISTENCE_TEMP:
 			if (pkrel->rd_rel->relpersistence != RELPERSISTENCE_TEMP)
 ereport(ERROR,
@@ -14140,6 +14147,13 @@ ATPrepChangePersistence(Relation rel, bool toLogged)
 			RelationGetRelationName(rel)),
 	 errtable(rel)));
 			break;
+		case RELPERSISTENCE_SESSION:
+			ereport(ERROR,
+	(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+	 errmsg("cannot change logged status of session table \"%s\"",
+			RelationGetRelationName(rel)),
+	 errtable(rel)));
+			break;
 		case RELPERSISTENCE_PERMANENT:
 			if (toLogged)
 /* nothing to do */
@@ -14627,14 +14641,7 @@ PreCommit_on_commit_actions(void)
 /* Do nothing (there shouldn't be such entries, actually) */
 break;
 			case ONCOMMIT_DELETE_ROWS:
-
-/*
- * If this transaction hasn't accessed any temporary
- * relations, we can skip truncating ON COMMIT DELETE ROWS
- * tables, as they must still be empty.
- */
-if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE))
-	oids_to_truncate = lappend_oid(oids_to_truncate, oc->relid);
+oids_to_truncate = lappend_oid(oids_to_truncate, oc->relid);
 break;
 			case ONCOMMIT_DROP:
 oids_to_drop = lappend_oid(oids_to_drop, oc->relid);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 3f67aaf..565c868 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -3266,20 +3266,11 @@ OptTemp:	TEMPORARY	{ $$ = RELPERSISTENCE_TEMP; }
 			| TEMP		{ $$ = RELPERSISTENCE_TEMP; }
 			| LOCAL TEMPORARY			{ $$ = RELPERSISTENCE_TEMP; }
 			| LOCAL

Re: [Proposal] Global temporary tables

2019-10-29 Thread Konstantin Knizhnik

 the sequence's data */
 	tuple = heap_form_tuple(tupDesc, value, null);
-	fill_seq_with_data(rel, tuple);
+	fill_seq_with_data(rel, tuple, InvalidBuffer);
 
 	/* process OWNED BY if given */
 	if (owned_by)
@@ -327,7 +327,7 @@ ResetSequence(Oid seq_relid)
 	/*
 	 * Insert the modified tuple into the new storage file.
 	 */
-	fill_seq_with_data(seq_rel, tuple);
+	fill_seq_with_data(seq_rel, tuple, InvalidBuffer);
 
 	/* Clear local cache so that we don't think we have cached numbers */
 	/* Note that we do not change the currval() state */
@@ -340,18 +340,21 @@ ResetSequence(Oid seq_relid)
  * Initialize a sequence's relation with the specified tuple as content
  */
 static void
-fill_seq_with_data(Relation rel, HeapTuple tuple)
+fill_seq_with_data(Relation rel, HeapTuple tuple, Buffer buf)
 {
-	Buffer		buf;
 	Page		page;
 	sequence_magic *sm;
 	OffsetNumber offnum;
+	bool lockBuffer = false;
 
 	/* Initialize first page of relation with special magic number */
 
-	buf = ReadBuffer(rel, P_NEW);
-	Assert(BufferGetBlockNumber(buf) == 0);
-
+	if (buf == InvalidBuffer)
+	{
+		buf = ReadBuffer(rel, P_NEW);
+		Assert(BufferGetBlockNumber(buf) == 0);
+		lockBuffer = true;
+	}
 	page = BufferGetPage(buf);
 
 	PageInit(page, BufferGetPageSize(buf), sizeof(sequence_magic));
@@ -360,7 +363,8 @@ fill_seq_with_data(Relation rel, HeapTuple tuple)
 
 	/* Now insert sequence tuple */
 
-	LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
+	if (lockBuffer)
+		LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
 
 	/*
 	 * Since VACUUM does not process sequences, we have to force the tuple to
@@ -410,7 +414,8 @@ fill_seq_with_data(Relation rel, HeapTuple tuple)
 
 	END_CRIT_SECTION();
 
-	UnlockReleaseBuffer(buf);
+	if (lockBuffer)
+		UnlockReleaseBuffer(buf);
 }
 
 /*
@@ -502,7 +507,7 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
 		/*
 		 * Insert the modified tuple into the new storage file.
 		 */
-		fill_seq_with_data(seqrel, newdatatuple);
+		fill_seq_with_data(seqrel, newdatatuple, InvalidBuffer);
 	}
 
 	/* process OWNED BY if given */
@@ -1178,6 +1183,17 @@ read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple)
 	LockBuffer(*buf, BUFFER_LOCK_EXCLUSIVE);
 
 	page = BufferGetPage(*buf);
+	if (GlobalTempRelationPageIsNotInitialized(rel, page))
+	{
+		/* Initialize sequence for global temporary tables */
+		Datum		value[SEQ_COL_LASTCOL] = {0};
+		bool		null[SEQ_COL_LASTCOL] = {false};
+		HeapTuple tuple;
+		value[SEQ_COL_LASTVAL-1] = Int64GetDatumFast(1); /* start sequence with 1 */
+		tuple = heap_form_tuple(RelationGetDescr(rel), value, null);
+		fill_seq_with_data(rel, tuple, *buf);
+	}
+
 	sm = (sequence_magic *) PageGetSpecialPointer(page);
 
 	if (sm->magic != SEQ_MAGIC)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8d25d14..50d0402 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -587,7 +587,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 	 * Check consistency of arguments
 	 */
 	if (stmt->oncommit != ONCOMMIT_NOOP
-		&& stmt->relation->relpersistence != RELPERSISTENCE_TEMP)
+		&& !IsLocalRelpersistence(stmt->relation->relpersistence))
 		ereport(ERROR,
 (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
  errmsg("ON COMMIT can only be used on temporary tables")));
@@ -1772,7 +1772,8 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged,
 		 * table or the current physical file to be thrown away anyway.
 		 */
 		if (rel->rd_createSubid == mySubid ||
-			rel->rd_newRelfilenodeSubid == mySubid)
+			rel->rd_newRelfilenodeSubid == mySubid ||
+			rel->rd_rel->relpersistence == RELPERSISTENCE_SESSION)
 		{
 			/* Immediate, non-rollbackable truncation is OK */
 			heap_truncate_one_rel(rel);
@@ -7708,6 +7709,12 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
 		 errmsg("constraints on unlogged tables may reference only permanent or unlogged tables")));
 			break;
+		case RELPERSISTENCE_SESSION:
+			if (pkrel->rd_rel->relpersistence != RELPERSISTENCE_SESSION)
+ereport(ERROR,
+		(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+		 errmsg("constraints on session tables may reference only session tables")));
+			break;
 		case RELPERSISTENCE_TEMP:
 			if (pkrel->rd_rel->relpersistence != RELPERSISTENCE_TEMP)
 ereport(ERROR,
@@ -14140,6 +14147,13 @@ ATPrepChangePersistence(Relation rel, bool toLogged)
 			RelationGetRelationName(rel)),
 	 errtable(rel)));
 			break;
+		case RELPERSISTENCE_SESSION:
+			ereport(ERROR,
+	(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+	 errmsg("cannot change logged status of session table \"%s\"",
+			RelationGetRelationName(rel)),
+	 errtable(rel)));
+			break;
 		case RELPERSISTENCE_PERMANENT:
 			if (toLogged)
 /* not

Re: [Proposal] Global temporary tables

On Mon, Oct 28, 2019 at 9:37 AM Konstantin Knizhnik
 wrote:
> Sorry, but both statements are not true.

Well, I think they are true.

> I am not sure how vital is lack of aborts for transactions working with
> GTT at replica.
> Some people said that there is no sense in aborts of read-only
> transactions at replica (despite to the fact that them are saving
> intermediate results in GTT).
> Some people said something similar with your's "dead on arrival".
> But inconsistency is not possible: if such transaction is really
> aborted, then backend is terminated and nobody can see this inconsistency.

Aborting the current transaction is a very different thing from
terminating the backend.

Also, the idea that there is no sense in aborts of read-only
transactions on a replica seems totally wrong. Suppose that you insert
a row into the table and then you go to insert a row in each index,
but one of the index inserts fails - duplicate key, out of memory
error, I/O error, whatever. Now the table and the index are
inconsistent. Normally, we're protected against this by MVCC, but if
you use a solution that breaks MVCC by using the same XID for all
transactions, then it can happen.

> Concerning second alternative: you can check yourself that it is not
> *extremely* complicated and invasive.
> I extracted changes which are related with handling transactions at
> replica and attached them to this mail.
> It is just 500 lines (including diff contexts). Certainly there are some
> limitation of this implementation: number of  transactions working with
> GTT at replica is limited by 2^32
> and since GTT tuples are not frozen, analog of GTT CLOG kept in memory
> is never truncated.

I admit that this patch is not lengthy, but there remains the question
of whether it is correct. It's possible that the problem isn't as
complicated as I think it is, but I do think there are quite a number
of reasons why this patch wouldn't be considered acceptable...

> I agree with it and think that implementation of GTT with private
> buffers and no replica access is good starting point.

...but given that we seem to agree on this point, perhaps it isn't
necessary to argue about those things right now.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [Proposal] Global temporary tables

On Mon, Oct 28, 2019 at 9:48 AM Konstantin Knizhnik
 wrote:
> Logically it may be good decision. But piratically support of parallel
> access to GTT requires just accessing their data through shared buffer.
> But in case of local temp tables we need also need to some how share
> table's metadata between parallel workers. It seems to be much more
> complicated if ever possible.

Why? The backends all share a snapshot, and can load whatever they
need from the system catalogs.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [Proposal] Global temporary tables

2019-10-28 Thread Konstantin Knizhnik





On 28.10.2019 15:13, Robert Haas wrote:

On Fri, Oct 25, 2019 at 12:22 PM Konstantin Knizhnik
 wrote:

On 25.10.2019 18:01, Robert Haas wrote:

On Fri, Oct 11, 2019 at 9:50 AM Konstantin Knizhnik
 wrote:

Just to clarify.
I have now proposed several different solutions for GTT:

Shared vs. private buffers for GTT:
1. Private buffers. This is least invasive patch, requiring no changes in 
relfilenodes.
2. Shared buffers. Requires changing relfilenode but supports parallel query 
execution for GTT.

I vote for #1. I think parallel query for temp objects may be a
desirable feature, but I don't think it should be the job of a patch
implementing GTTs to make it happen. In fact, I think it would be an
actively bad idea, because I suspect that if we do eventually support
temp relations for parallel query, we're going to want a solution that
is shared between regular temp tables and global temp tables, not
separate solutions for each.

Sorry, may be I do not not understand you.
It seems to me that there is only one thing preventing usage of
temporary tables in parallel plans: private buffers.
If global temporary tables are accessed as normal tables though shared
buffers then them can be used in parallel queries
and no extra support is required for it.
At least I have checked that parallel queries are correctly worked for
my implementation of GTT with shared buffers.
So I do not understand about which "separate solutions" you are talking
about.

I can agree that private buffers may be  good starting point for GTT
implementation, because it is less invasive and GTT access speed is
exactly the same as of normal temp tables.
But I do not understand your argument why it is "actively bad idea".

Well, it sounds like you're talking about ending up in a situation
where local temporary tables are still in private buffers, but global
temporary table data is in shared buffers. I think that would be
inconsistent. And it would mean that when somebody wanted to make
local temporary tables accessible in parallel query, they'd have to
write a patch for that.  In other words, I don't support dividing the
patches like this:

Patch #1: Support global temporary tables + allow global temporary
tables to used by parallel query
Patch #2: Allow local temporary tables to be used by parallel query

I support dividing them like this:

Patch #1: Support global temporary tables
Patch #2: Allow (all kinds of) temporary tables to be used by parallel query

The second division looks a lot cleaner to me, although as always I
might be missing something.

Logically it may be good decision. But piratically support of parallel 
access to GTT requires just accessing their data through shared buffer.
But in case of local temp tables we need also need to some how share 
table's metadata between parallel workers. It seems to be much more 
complicated if ever possible.


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [Proposal] Global temporary tables

2019-10-28 Thread Konstantin Knizhnik

ndicated buffer is marked dirty.
  */
 bool
-HeapTupleSatisfiesVisibility(HeapTuple tup, Snapshot snapshot, Buffer buffer)
+HeapTupleSatisfiesVisibility(Relation relation, HeapTuple tup, Snapshot snapshot, Buffer buffer)
 {
+	if (relation->rd_rel->relpersistence == RELPERSISTENCE_SESSION && RecoveryInProgress())
+	{
+		AccessTempRelationAtReplica = true;
+		return TempTupleSatisfiesVisibility(tup, snapshot->curcid, buffer);
+	}
+	AccessTempRelationAtReplica = false;
+
 	switch (snapshot->snapshot_type)
 	{
 		case SNAPSHOT_MVCC:
diff --git a/src/backend/access/transam/transam.c b/src/backend/access/transam/transam.c
index 365ddfb..bce9c4a 100644
--- a/src/backend/access/transam/transam.c
+++ b/src/backend/access/transam/transam.c
@@ -22,6 +22,7 @@
 #include "access/clog.h"
 #include "access/subtrans.h"
 #include "access/transam.h"
+#include "access/xact.h"
 #include "utils/snapmgr.h"
 
 /*
@@ -126,6 +127,9 @@ TransactionIdDidCommit(TransactionId transactionId)
 {
 	XidStatus	xidstatus;
 
+	if (AccessTempRelationAtReplica)
+		return !IsReplicaCurrentTransactionId(transactionId) && !IsReplicaTransactionAborted(transactionId);
+
 	xidstatus = TransactionLogFetch(transactionId);
 
 	/*
@@ -182,6 +186,9 @@ TransactionIdDidAbort(TransactionId transactionId)
 {
 	XidStatus	xidstatus;
 
+	if (AccessTempRelationAtReplica)
+		return IsReplicaTransactionAborted(transactionId);
+
 	xidstatus = TransactionLogFetch(transactionId);
 
 	/*
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 9162286..cf6bf4e 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -192,6 +192,7 @@ typedef struct TransactionStateData
 	int			parallelModeLevel;	/* Enter/ExitParallelMode counter */
 	bool		chain;			/* start a new block after this one */
 	struct TransactionStateData *parent;	/* back link to parent */
+	TransactionId replicaTransactionId;/* pseudo XID for inserting data in global temp tables at replica */
 } TransactionStateData;
 
 typedef TransactionStateData *TransactionState;
@@ -286,6 +287,12 @@ typedef struct XactCallbackItem
 
 static XactCallbackItem *Xact_callbacks = NULL;
 
+static TransactionId replicaTransIdCount = FirstNormalTransactionId;
+static TransactionId replicaTopTransId;
+static Bitmapset*replicaAbortedXids;
+
+bool AccessTempRelationAtReplica;
+
 /*
  * List of add-on start- and end-of-subxact callbacks
  */
@@ -443,6 +450,48 @@ GetCurrentTransactionIdIfAny(void)
 }
 
 /*
+ * Transactions at replica can update only global temporary tables.
+ * Them are assigned backend-local XIDs which are independent from normal XIDs received from primary node.
+ * So tuples of temporary tables at replica requires special visibility rules.
+ *
+ * XIDs for such transactions at replica are created on demand (when tuple of temp table is updated).
+ * XID wrap-around and adjusting XID horizon is not supported. So number of such transactions at replica is
+ * limited by 2^32 and require up to 2^29 in-memory bitmap for marking aborted transactions.
+  */
+TransactionId
+GetReplicaTransactionId(void)
+{
+	TransactionState s = CurrentTransactionState;
+	if (!TransactionIdIsValid(s->replicaTransactionId))
+		s->replicaTransactionId = ++replicaTransIdCount;
+	return s->replicaTransactionId;
+}
+
+/*
+ * At replica transaction can update only temporary tables
+ * and them are assigned special XIDs (not related with normal XIDs received from primary node).
+ * As far as we see only own transaction it is not necessary to mark committed transactions.
+ * Only marking aborted ones is enough. All transactions which are not marked as aborted are treated as
+ * committed or self in-progress transactions.
+ */
+bool
+IsReplicaTransactionAborted(TransactionId xid)
+{
+	return bms_is_member(xid, replicaAbortedXids);
+}
+
+/*
+ * As far as XIDs for transactions at replica are generated individually for each backends,
+ * we can check that XID belongs to the current transaction or any of its subtransactions by
+ * just comparing it with XID of top transaction.
+ */
+bool
+IsReplicaCurrentTransactionId(TransactionId xid)
+{
+	return xid > replicaTopTransId;
+}
+
+/*
  *	GetTopFullTransactionId
  *
  * This will return the FullTransactionId of the main transaction, assigning
@@ -855,6 +904,9 @@ TransactionIdIsCurrentTransactionId(TransactionId xid)
 {
 	TransactionState s;
 
+	if (AccessTempRelationAtReplica)
+		return IsReplicaCurrentTransactionId(xid);
+
 	/*
 	 * We always say that BootstrapTransactionId is "not my transaction ID"
 	 * even when it is (ie, during bootstrap).  Along with the fact that
@@ -1206,7 +1258,7 @@ static TransactionId
 RecordTransactionCommit(void)
 {
 	TransactionId xid = GetTopTransactionIdIfAny();
-	bool		markXidCommitted = TransactionIdIsValid(xid);
+	bool		markXidCommitted = TransactionIdIsNormal(xid);
 	TransactionId latestXid = InvalidT

Re: [Proposal] Global temporary tables

On Fri, Oct 25, 2019 at 12:22 PM Konstantin Knizhnik
 wrote:
> On 25.10.2019 18:01, Robert Haas wrote:
> > On Fri, Oct 11, 2019 at 9:50 AM Konstantin Knizhnik
> >  wrote:
> >> Just to clarify.
> >> I have now proposed several different solutions for GTT:
> >>
> >> Shared vs. private buffers for GTT:
> >> 1. Private buffers. This is least invasive patch, requiring no changes in 
> >> relfilenodes.
> >> 2. Shared buffers. Requires changing relfilenode but supports parallel 
> >> query execution for GTT.
> > I vote for #1. I think parallel query for temp objects may be a
> > desirable feature, but I don't think it should be the job of a patch
> > implementing GTTs to make it happen. In fact, I think it would be an
> > actively bad idea, because I suspect that if we do eventually support
> > temp relations for parallel query, we're going to want a solution that
> > is shared between regular temp tables and global temp tables, not
> > separate solutions for each.
>
> Sorry, may be I do not not understand you.
> It seems to me that there is only one thing preventing usage of
> temporary tables in parallel plans: private buffers.
> If global temporary tables are accessed as normal tables though shared
> buffers then them can be used in parallel queries
> and no extra support is required for it.
> At least I have checked that parallel queries are correctly worked for
> my implementation of GTT with shared buffers.
> So I do not understand about which "separate solutions" you are talking
> about.
>
> I can agree that private buffers may be  good starting point for GTT
> implementation, because it is less invasive and GTT access speed is
> exactly the same as of normal temp tables.
> But I do not understand your argument why it is "actively bad idea".

Well, it sounds like you're talking about ending up in a situation
where local temporary tables are still in private buffers, but global
temporary table data is in shared buffers. I think that would be
inconsistent. And it would mean that when somebody wanted to make
local temporary tables accessible in parallel query, they'd have to
write a patch for that.  In other words, I don't support dividing the
patches like this:

Patch #1: Support global temporary tables + allow global temporary
tables to used by parallel query
Patch #2: Allow local temporary tables to be used by parallel query

I support dividing them like this:

Patch #1: Support global temporary tables
Patch #2: Allow (all kinds of) temporary tables to be used by parallel query

The second division looks a lot cleaner to me, although as always I
might be missing something.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [Proposal] Global temporary tables