Re: Static snapshot data

Up to now, SerializableSnapshot and QuerySnapshot are malloc'ed and
free'd for every transaction or statement, respectively. This patch
puts these data structures into static memory, thus saving a few CPU
cycles and two malloc calls per transaction or (in isolation level
READ COMMITTED) per query.

Servus
Manfred

Manfred Koizar <mkoi-pg(at)aon(dot)at> writes:
> Up to now, SerializableSnapshot and QuerySnapshot are malloc'ed and
> free'd for every transaction or statement, respectively. This patch
> puts these data structures into static memory, thus saving a few CPU
> cycles and two malloc calls per transaction or (in isolation level
> READ COMMITTED) per query.

I do not like this patch. Two mallocs per transaction is an utterly
insignificant overhead. And isn't the patch going in quite the wrong
direction for nested transactions? The assumption that there's
never more than one QuerySnapshot seems to fly in the face of that...

regards, tom lane

On Fri, 09 May 2003 23:08:38 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
wrote:
>I do not like this patch.

That's not a surprise, but ...

>Two mallocs per transaction is an utterly insignificant overhead.

2002-05-25 you said: "a cycle saved is a cycle earned."

More importantly the patch makes it clearer that there is always at
most one instance of SerializableSnapshotData and [current]
QuerySnapshotData.

>And isn't the patch going in quite the wrong
>direction for nested transactions?

Our (Alvaro's and my) current understanding is that snapshots are not
influenced by nested transactions.

ad SerializableSnapshot: A subtransaction operates in the context of
the main transaction. We do not want to see different snapshots at
different nesting levels.

> The assumption that there's
>never more than one QuerySnapshot seems to fly in the face of that...

ad QuerySnapshot: If there is a need for a query snapshot stack, then
it is not because of nested transactions but due to queries invoking
functions containing queries ... This is currently handled by
CopyQuerySnapshot(), AFAIK.

Servus
Manfred

Manfred Koizar <mkoi-pg(at)aon(dot)at> writes:
>> And isn't the patch going in quite the wrong
>> direction for nested transactions?

> Our (Alvaro's and my) current understanding is that snapshots are not
> influenced by nested transactions.

What was that long article Alvaro posted yesterday, then? He definitely
came to the conclusion that nested transactions need different
QuerySnapshots, and I think it was still open whether they need
different SerializableSnapshots.

regards, tom lane

On Mon, May 12, 2003 at 09:40:37AM -0400, Tom Lane wrote:

Moved to -hackers.

> Manfred Koizar <mkoi-pg(at)aon(dot)at> writes:
> >> And isn't the patch going in quite the wrong
> >> direction for nested transactions?
>
> > Our (Alvaro's and my) current understanding is that snapshots are not
> > influenced by nested transactions.
>
> What was that long article Alvaro posted yesterday, then? He definitely
> came to the conclusion that nested transactions need different
> QuerySnapshots, and I think it was still open whether they need
> different SerializableSnapshots.

Well, I'm not sure about this. In fact, if the reasoning below is
correct, we can get away with static Serializable- and QuerySnapshots.

I don't think it makes sense to change the isolation level for a
non-toplevel transaction. That is, if the topmost transaction is
ISOLATION LEVEL SERIALIZABLE, all its child transactions will be. And
if it's not, then there's no way to make its child transactions be so.

Is there an objection to this idea? If there is, all the following is
incorrect, I think.

With "constant isolation" in mind, it's clear that the
SerializableSnapshot is going to be constant for all transactions. We
don't need to calculate different SerializableSnapshots for child
transactions; thus going with a static variable for SerializableSnapshot
isn't wrong.

And about QuerySnapshots: given some running transaction with a given
QuerySnapshot, a newly created child transaction's first QuerySnapshot
can be calculated easily as:

- Xmin, Xmax and xip are the same as in the current implementation
(i.e. the values from GetSnapshotData)
- childxact is my parent's childxact
- parentxact is created by adding my parent XID to my parent's
parentxact

And given some non-topmost ending transaction, its parent transaction
next QuerySnapshot can be calculated as:

- Xmin, Xmax and xip are the same as in the current implementation
- childxact is created by adding the finishing child's XID to its
childxact
- parentxact is created by removing my own XID from my child's XID.

(The border case of a topmost starting transaction's QuerySnapshot is
the copy of its SerializableSnapshot, just as it is now).

Thus we don't need to keep track of multiple QuerySnapshots either --
the new one can always be calculated from the last one.

Please note that yesterday's article contained an error about childxact:
we need to know not only what are my own parent's child transactions.
We need to know all the XIDs that were completed within the same topmost
transaction, because all of them have to be taken into consideration for
the visibility rules. IOW, we have to consider all of them like they
were only one transaction, discarding the changes made by the ones that
were aborted.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
Jajaja! Solo hablaba en serio!

Alvaro Herrera <alvherre(at)dcc(dot)uchile(dot)cl> writes:
> I don't think it makes sense to change the isolation level for a
> non-toplevel transaction. That is, if the topmost transaction is
> ISOLATION LEVEL SERIALIZABLE, all its child transactions will be. And
> if it's not, then there's no way to make its child transactions be so.
> Is there an objection to this idea?

I have a feeling that there might be some value in running a
SERIALIZABLE subtransaction inside a READ COMMITTED parent. Too tired
to come up with an example right now, though. I agree that the other
way 'round (READ COMMITTED inside SERIALIZABLE) would be a bad idea,
since it negates the fundamental premise of SERIALIZABLE.

> With "constant isolation" in mind, it's clear that the
> SerializableSnapshot is going to be constant for all transactions.

But ... your definition of the snapshot includes the list of successful
previous subtransactions of the parent. How's that static?

> And about QuerySnapshots: given some running transaction with a given
> QuerySnapshot, a newly created child transaction's first QuerySnapshot
> can be calculated easily as:
> - Xmin, Xmax and xip are the same as in the current implementation
> (i.e. the values from GetSnapshotData)

Not entirely sure about that in READ COMMITTED mode. Should a child
xact be able to see commits from other backends that happened before it
started, but after its parent started? I can think of arguments on both
sides ...

regards, tom lane

On Mon, 12 May 2003 23:55:31 -0400, Alvaro Herrera
<alvherre(at)dcc(dot)uchile(dot)cl> wrote:
>On Mon, May 12, 2003 at 09:40:37AM -0400, Tom Lane wrote:
>> > Our (Alvaro's and my) current understanding is that snapshots are not
>> > influenced by nested transactions.
>>
>> What was that long article Alvaro posted yesterday, then?

Unfortunately I sent my reply to Tom before I read my inbox :-(

>[...] if the reasoning below is
>correct, we can get away with static Serializable- and QuerySnapshots.
>
>I don't think it makes sense to change the isolation level for a
>non-toplevel transaction. That is, if the topmost transaction is
>ISOLATION LEVEL SERIALIZABLE, all its child transactions will be. And
>if it's not, then there's no way to make its child transactions be so.

I agree.

Tom replied:
|I have a feeling that there might be some value in running a
|SERIALIZABLE subtransaction inside a READ COMMITTED parent.

Never thought of that, most probably due to my notion of nested
transactions (which may be weird). Let's try to sort it out. Here is
my view, it's not much more than three simple rules:

Rule 1) Subtransactions can be nested to arbitrary levels. During
execution of a subtransaction there is no change to the state of the
enclosing transaction.

Rule 2) On subtransaction ROLLBACK, changes done by the
subtransaction are effectively undone.

Rule 3) After subtransaction COMMIT, changes done by the
subtransaction are effectively treated as if done by the enclosing
transaction.

So this sequence of commands

BEGIN; -- main transaction T1
query 1;
BEGIN; -- subtransaction T1.1
query 2;
BEGIN; -- subtransaction T1.1.1
query 3;
ROLLBACK; -- subtransaction T1.1.1
COMMIT; -- subtransaction T1.1
query 4;
BEGIN; -- subtransaction T1.2
query 5;
ROLLBACK; -- subtransaction T1.2
query 6;
COMMIT; -- main transaction T1

effectively behaves like

BEGIN; -- main transaction T1
query 1;
query 2;
query 4;
query 6;
COMMIT; -- main transaction T1

I.e. it does not matter whether query 2 has been issued inside the
(later committed) subtransaction T1.1 or directly in the main
transaction T1. Query 3 and query 5 which are part of aborted
subtransactions (T1.1.1 and T1.2) look as if they had never been
issued.

I'm inclined to include "SET TRANSACTION ISOLATION LEVEL" in the kind
of changes that rules 2 and 3 deal with. Perhaps the rules should say
"commands executed" instead of "changes done". This would forbid
running parts of the same main transaction with different isolation
levels.

>With "constant isolation" in mind, it's clear that the
>SerializableSnapshot is going to be constant for all transactions. We
>don't need to calculate different SerializableSnapshots for child
>transactions; thus going with a static variable for SerializableSnapshot
>isn't wrong.
|
|But ... your definition of the snapshot includes the list of successful
|previous subtransactions of the parent.

Apart from possible (future) performance hacks, I see no need to
include a list of completed subtransactions in any local data
structure, neither Snapshots nor TransactionStates. We do not
enumerate the children of a transaction. We look into the other
direction: When we check visibility we find a transaction id in a
tuple header and want to know its parent transaction id. This
question can be answered by pg_subtrans which will be built on top of
the SimpleLRU patch submitted a few days ago.

| How's that static?

>And about QuerySnapshots: given some running transaction with a given
>QuerySnapshot, a newly created child transaction's first QuerySnapshot
>can be calculated easily as:
>
>- Xmin, Xmax and xip are the same as in the current implementation
> (i.e. the values from GetSnapshotData)

Yes, and this overwrites the current QuerySnapshot.

>- childxact is my parent's childxact
>- parentxact is created by adding my parent XID to my parent's
> parentxact

No need for childxact and parentxact (see below).

|Not entirely sure about that in READ COMMITTED mode. Should a child
|xact be able to see commits from other backends that happened before it
|started, but after its parent started?

Why not? Even if there is no subtransaction, a new *query* sees
commits from other backends. I don't see why query 2 in the right
case should see commits that are invisible to query 2 in the left
case.

BEGIN; BEGIN;
query 1; query 1;
BEGIN; --
query 2; query 2;

|I can think of arguments on both sides ...

??

>And given some non-topmost ending transaction, its parent transaction
>next QuerySnapshot can be calculated as:

A QuerySnapshot is always taken at the start of a query. It does not
depend on the transaction nesting level.

>Thus we don't need to keep track of multiple QuerySnapshots either --
>the new one can always be calculated from the last one.

Not only "from the last one" but "independently from the last one".
GetSnapshotData does not care about subtransactions.

>We need to know all the XIDs that were completed within the same topmost
>transaction, because all of them have to be taken into consideration for
>the visibility rules.

pg_subtrans keeps track of that (sort of, because it can navigate from
child to parent but not vice versa).

> IOW, we have to consider all of them like they
>were only one transaction, discarding the changes made by the ones that
>were aborted.

Okay, cf. rules 2 and 3.

[While we are at it, I continue with some comments to Alvaro's other
message.]

On Sun, 11 May 2003 19:29:27 -0400, Alvaro Herrera
<alvherre(at)dcc(dot)uchile(dot)cl> wrote:
:In the current implementation, it's sufficient to know
:a) what transactions come before me (Xmin),
:b) what transactions come after me (Xmax),
:c) what transactions are in progress (xip), and
:d) what commands come before me in the current transaction
: (curcid)

I propose that we don't change this, except that d) should say "... in
the current transaction tree"

:In the nested transactions case, we also need to know
:
:e) what subtransactions of my own parent transactions come before me,
: and
:f) what commands of my parent transactions come before me.

Yes, we need to have this information. But that doesn't mean we have
to store it in a snapshot.

ad e) I can't see a need to directly answer this question. What we
need is e') Does a given xid belong to the current xact tree?
This can be answered using pg_subtrans and the transaction information
stack (see below).

ad f) I'd write this as:
f') What commands of my transaction tree come before me?

:Consider the following scenario:
:
:BEGIN; xid=1
:CREATE TABLE a (p int UNIQUE, q int); xid=1 cid=1
:INSERT INTO a (p) VALUES (1); xid=1 cid=2
:BEGIN; xid=2
: -- should fail due to unique constraint
: INSERT INTO a (p) VALUES (1); xid=2 cid=1
:ROLLBACK;
:BEGIN; xid=3
: INSERT INTO a (p) VALUES (2); cid=1
: DELETE FROM a WHERE one=1; cid=2
: -- "a" should have 1 tuple
:COMMIT;
:-- should work, because the old tuple doesn't exist anymore
:INSERT INTO a (p) VALUES (1); xid=1 cid=3
:COMMIT;

It might help, if we continue to increment cid across subtransaction
boundaries.

BEGIN; xid=1
CREATE TABLE a (p int UNIQUE, q int); xid=1 cid=1
INSERT INTO a (p) VALUES (1); xid=1 cid=2
BEGIN; xid=2
-- should fail due to unique constraint
INSERT INTO a (p) VALUES (1); xid=2 cid=1 -> 3
ROLLBACK;
BEGIN; xid=3
INSERT INTO a (p) VALUES (2); cid=1 -> 4
DELETE FROM a WHERE one=1; cid=2 -> 5
-- "a" should have 1 tuple
COMMIT;
-- should work, because the old tuple doesn't exist anymore
INSERT INTO a (p) VALUES (1); xid=1 cid=3 -> 6
COMMIT;

:Here, the QuerySnapshot of xid 1, at the time of cid=3 [6] should see the
:results of execution from xid 3, but it is not before Xmin, and it's
:after Xmax, and is not in the xip array.

This will be handled by HeapTupleSatisfiesXxxx using pg_subtrans:

. We find a tuple (having p=2) with xmin=3.

. In pg_clog we find that xact 3 is a committed subtransaction.

. We lookup xact 3's parent transaction in pg_subtrans and get
parent xact = 1.

. Consulting the transaction information stack we find out that
xact 1 is one of our own currently active transactions (in this
case the only one).

. Because the tuple's cmin (4) is less than CurrentCommandId (6)
the tuple is visible.

The snapshot is only consulted for transactions outside our own
transaction tree. This is a natural extension to the current logic,
where we check for IsCurrentTransactionId before we look at the
snapshot.

:Also, the QuerySnapshot of xid 3, should see the results of commands
:from xid 1 just like they'd be seen if they where in the same xact but
:with a lesser CommandId.

Yes, because if we find a tuple with cmin still active, we look for
this xid on our transaction information stack.

:Both cases are not implementable with the current notion of a Snapshot.

I think they are. What we need is not an extension to the snapshot
structure, but a transaction information stack holding transaction
specific information: TransactionId, TransState, TBlockState, ...

This looks almost like struct TransactionStateData, except that
commandId, startTime, and startTimeUsec belong only to the main
transaction.

:I'm not sure what the SerializableSnapshot should be. It does need to
:take into account the changes made by previous committed
:subtransactions, right?

Per rule 3 previous committed subtransactions are equivalent to
previous queries. So whether their effects are visible depends on the
current query snapshot.

Consider this

UPDATE a SET q = 1 WHERE ...; -- xid=1 cid=7

when there is a TRIGGER BEFORE UPDATE FOR EACH ROW containing:

...
BEGIN; -- subtransaction
UPDATE ...;
COMMIT;
...

While the second tuple is processed, visibility rules for the effects
of the trigger executed for the first tuple are the same as if the
trigger had executed its UPDATE without wrapping it into a
subtransaction.

:It's also clear that we need to differentiate a parent's QuerySnapshot
:from their child's.

No, a QuerySnapshot is taken at the start of a query ...

: It's not clear to me what should be done in the
:case of a SerializableSnapshot.

A SerializableSnapshot is the first snapshot taken during a
*transaction tree*.

Servus
Manfred

On Tue, May 13, 2003 at 09:57:39PM +0200, Manfred Koizar wrote:
> On Mon, 12 May 2003 23:55:31 -0400, Alvaro Herrera
> <alvherre(at)dcc(dot)uchile(dot)cl> wrote:
> >On Mon, May 12, 2003 at 09:40:37AM -0400, Tom Lane wrote:

> Tom replied:
> |I have a feeling that there might be some value in running a
> |SERIALIZABLE subtransaction inside a READ COMMITTED parent.

After digesting Manfred's ideas, I agree that there's no need for
parentxact or childxact arrays in the Snapshots. Also I agree that
there's no need for a QuerySnapshot stack -- there's always one, no
matter how deep you are in the transaction tree. It also doesn't matter
if you are in a SERIALIZABLE or READ COMMITTED transaction: if in READ
COMMITTED, the QuerySnapshot is always calculated afresh for each query,
and if in SERIALIZABLE, the QuerySnapshot is always equal to the
SerializableSnapshot.

When a SERIALIZABLE subtransaction is started in a READ COMMITTED
parent, the SerializableSnapshot is just calculated again. There's no
need to keep the old SerializableSnapshot, because it's useless.

> Rule 1) Subtransactions can be nested to arbitrary levels.
> Rule 2) On subtransaction ROLLBACK, changes done by the
> subtransaction are effectively undone.
> Rule 3) After subtransaction COMMIT, changes done by the
> subtransaction are effectively treated as if done by the enclosing
> transaction.

Right.

> I'm inclined to include "SET TRANSACTION ISOLATION LEVEL" in the kind
> of changes that rules 2 and 3 deal with. Perhaps the rules should say
> "commands executed" instead of "changes done". This would forbid
> running parts of the same main transaction with different isolation
> levels.

Yeah, the TRANSACTION ISOLATION LEVEL should be part of the current
transaction state, but it is inherited to subtransactions. The user can
change from READ COMMITTED to SERIALIZABLE when starting a
subtransaction, but not the other way around. (Note that it _is_
possible to change from SERIALIZABLE to READ COMMITTED in the topmost
transaction).

It's also not possible to start a SERIALIZABLE transaction with a
different SerializableSnapshot inside a SERIALIZABLE parent. It would
violate the rules of serializability, as it means changing the snapshot
in the middle of the (parent) SERIALIZABLE.

> GetSnapshotData does not care about subtransactions.

Right.

> :In the current implementation, it's sufficient to know
> :a) what transactions come before me (Xmin),
> :b) what transactions come after me (Xmax),
> :c) what transactions are in progress (xip), and
> :d) what commands come before me in the current transaction
> : (curcid)
>
> I propose that we don't change this, except that d) should say "... in
> the current transaction tree"

There's no need for this. Starting a transaction should be a new
CommandId for the parent transaction, so the tuples written by a
transaction that's not me but belong to my transaction tree are
effectively treated as if they were from a previous CommandId.

It should only be a matter of incrementing CommandId in the parent
transaction just before starting the subtransaction, or just after
ending it. It is critical to do so, or we risk considering changes
neighbouring the subxact with the same CommandId, which would be bogus.

> ad e) I can't see a need to directly answer this question. What we
> need is e') Does a given xid belong to the current xact tree?
> This can be answered using pg_subtrans and the transaction information
> stack (see below).
> ad f) I'd write this as:
> f') What commands of my transaction tree come before me?

I agree.

> It might help, if we continue to increment cid across subtransaction
> boundaries.

We don't need to, because

> This will be handled by HeapTupleSatisfiesXxxx using pg_subtrans:
>
> . We find a tuple (having p=2) with xmin=3.
>
> . In pg_clog we find that xact 3 is a committed subtransaction.
>
> . We lookup xact 3's parent transaction in pg_subtrans and get
> parent xact = 1.
>
> . Consulting the transaction information stack we find out that
> xact 1 is one of our own currently active transactions (in this
> case the only one).
>
> . Because the tuple's cmin (4) is less than CurrentCommandId (6)
> the tuple is visible.

This last rule should be replaced by:

. Because the tuple's xmin is not my XID, the tuple is visible.

We need to check the CurrentCommandId only if xmin (or xmax) is my own
XID.

> :Both cases are not implementable with the current notion of a Snapshot.
>
> I think they are. What we need is not an extension to the snapshot
> structure, but a transaction information stack holding transaction
> specific information: TransactionId, TransState, TBlockState, ...
>
> This looks almost like struct TransactionStateData, except that
> commandId, startTime, and startTimeUsec belong only to the main
> transaction.

Hm, why do you want to left out the startTime and startTimeUsec? Maybe
it's useful to know the start time of the current transaction, instead
of the start time of the current transaction tree.

Also the CommandId belongs to each transaction and not only the topmost.
This way we are not limited to 2^32 commands per transaction tree, but
to 2^32 commands per transaction. No, I don't really think anyone cares
about that limit :-)

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"La soledad es compañia"

On Thu, 15 May 2003 19:39:55 -0400, Alvaro Herrera
<alvherre(at)dcc(dot)uchile(dot)cl> wrote:
>When a SERIALIZABLE subtransaction is started in a READ COMMITTED
>parent, the SerializableSnapshot is just calculated again.

I would not allow this, see below ...

>The user can
>change from READ COMMITTED to SERIALIZABLE when starting a
>subtransaction, but not the other way around.

You cannot propose this and agree to my three rules at the same time.
Rule 3 says that these two sequences of commands are equivalent:

A B
BEGIN; BEGIN;
SET TRANSACTION ISOLATION LEVEL SET TRANSACTION ISOLATION LEVEL
READ COMMITTED; READ COMMITTED;
SELECT version(); SELECT version();
BEGIN;
SET TRANSACTION ISOLATION LEVEL SET TRANSACTION ISOLATION LEVEL
SERIALIZABLE; SERIALIZABLE; -- error!
COMMIT;
SELECT timeofday(); SELECT timeofday();
COMMIT; COMMIT;

While you want A to succeed, it's clear that B results in an error.

What we *do* need is a saved_isolation_level on the transaction
information stack, so we can restore the state of the enclosing
transaction on subtransaction ROLLBACK (the same is true for
changeable GUC variables):

BEGIN;
SET TRANSACTION ISOLATION LEVEL READ COMMITTED;
BEGIN;
-- This is allowed (no query so far):
SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
SET random_page_cost = 100;
ROLLBACK;
-- main transaction still active,
-- TRANSACTION ISOLATION LEVEL is READ COMMITTED

> (Note that it _is_
>possible to change from SERIALIZABLE to READ COMMITTED in the topmost
>transaction).

fred=# BEGIN;
BEGIN
fred=# SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
SET
fred=# SET TRANSACTION ISOLATION LEVEL READ COMMITTED;
SET
fred=# SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
SET
fred=# SELECT version();
version
-------------------------------------------------------------
PostgreSQL 7.3 on i586-pc-linux-gnu, compiled by GCC 2.95.2
(1 row)

fred=# SET TRANSACTION ISOLATION LEVEL READ COMMITTED;
ERROR: SET TRANSACTION ISOLATION LEVEL must be called before any
query

>> :d) what commands come before me in the current transaction
>> : (curcid)
>>
>> I propose that we don't change this, except that d) should say "... in
>> the current transaction tree"
>
>There's no need for this. Starting a transaction should be a new
>CommandId for the parent transaction, so the tuples written by a
>transaction that's not me but belong to my transaction tree are
>effectively treated as if they were from a previous CommandId.

But if you have subtransactions in functions called by the current
query, they should be treated like *higher* commandIds.

>> It might help, if we continue to increment cid across subtransaction
>> boundaries.
>
>We don't need to, because
>
>> This will be handled by HeapTupleSatisfiesXxxx using pg_subtrans:
>>
>> . We find a tuple (having p=2) with xmin=3.
>>
>> . In pg_clog we find that xact 3 is a committed subtransaction.
>>
>> . We lookup xact 3's parent transaction in pg_subtrans and get
>> parent xact = 1.
>>
>> . Consulting the transaction information stack we find out that
>> xact 1 is one of our own currently active transactions (in this
>> case the only one).
>>
>> . Because the tuple's cmin (4) is less than CurrentCommandId (6)
>> the tuple is visible.
>
>This last rule should be replaced by:
>
> . Because the tuple's xmin is not my XID, the tuple is visible.
>
>We need to check the CurrentCommandId only if xmin (or xmax) is my own
>XID.

This sounds good as long as queries are issued by a client application
one after the other. But will it still work when we have
subtransactions in functions?

CREATE TABLE a (i int, t text);
INSERT INTO a VALUES (1, '1');
INSERT INTO a VALUES (2, '2');
INSERT INTO a VALUES (3, '3');

CREATE OR REPLACE FUNCTION fa (int) RETURNS bool AS '
BEGIN
INSERT INTO a VALUES (2 * $1, ''new'');
RETURN true;
END;
' LANGUAGE 'plpgsql';

BEGIN;
-- do some queries to raise CommandId ...
UPDATE a SET i = 3 * i, t = 'old' WHERE fa(a.i);
COMMIT;

SELECT xmin,cmin,* FROM a;
xmin | cmin | i | t
--------+------+---+-----
243871 | 12 | 2 | new
243871 | 10 | 3 | old
243871 | 15 | 4 | new
243871 | 10 | 6 | old
243871 | 17 | 6 | new
243871 | 10 | 9 | old
(6 rows)

The 'new' rows are not seen by the UPDATE because they are inserted by
the same transaction but with a higher CommandId.

Now change fa() to wrap the INSERT statement into a subtransaction.
Per your proposal the 'new' rows would have xmin = 243872, 243873,
243874 and cmin = 0. "Because the tuple's xmin is not my XID", they
would be visible to the UPDATE. Halloween!

>> This looks almost like struct TransactionStateData, except that
>> commandId, startTime, and startTimeUsec belong only to the main
>> transaction.
>
>Hm, why do you want to left out the startTime and startTimeUsec?

I thought they're useless ...

Maybe the term "nested transactions" is confusing. I'm starting to
believe that my position is better described by "multiple savepoints".
Are we still talking about the same thing?

Servus
Manfred

On Fri, May 16, 2003 at 07:12:31PM +0200, Manfred Koizar wrote:
> On Thu, 15 May 2003 19:39:55 -0400, Alvaro Herrera
> <alvherre(at)dcc(dot)uchile(dot)cl> wrote:

> >The user can
> >change from READ COMMITTED to SERIALIZABLE when starting a
> >subtransaction, but not the other way around.
>
> You cannot propose this and agree to my three rules at the same time.
> Rule 3 says that these two sequences of commands are equivalent:
> [example]

I see. Then I don't fully agree with your rules. Let's say I find that
the rules are very good guidelines, but they fail WRT the isolation
level, which is a special exception.

> What we *do* need is a saved_isolation_level on the transaction
> information stack, so we can restore the state of the enclosing
> transaction on subtransaction ROLLBACK (the same is true for
> changeable GUC variables):

Yes, I think it will be necessary to keep the isolation level state in
the transaction state stack.

I'm not sure how are SET changes supposed to work. In fact, I don't
know how the rollback of such is implemented currently. If there is
some static variable around for each GUC variable, there's probably
something to be done about this. I don't know yet, but I bet it's going
to be a real PITA.

> fred=# BEGIN;
> BEGIN
> fred=# SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
> SET
> fred=# SET TRANSACTION ISOLATION LEVEL READ COMMITTED;
> SET
> fred=# SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
> SET
> fred=# SELECT version();
> version
> -------------------------------------------------------------
> PostgreSQL 7.3 on i586-pc-linux-gnu, compiled by GCC 2.95.2
> (1 row)
>
> fred=# SET TRANSACTION ISOLATION LEVEL READ COMMITTED;
> ERROR: SET TRANSACTION ISOLATION LEVEL must be called before any
> query

Well, this is what I meant: the transaction isolation level is
changeable to SERIALIZABLE only if:

- There has been no command in the current transaction(*), and
- I am the topmost transaction, or
- my parent transaction is READ COMMITTED

And the transaction isolation level is changeable to READ COMMITTED only
if there has been no command in the current transaction, and I am the
topmost transaction. This should be necessary only for the cases where
the default transaction isolation level is serializable.

(*) should be a test equivalent to the current
SerializableSnapshot == NULL

> >We need to check the CurrentCommandId only if xmin (or xmax) is my own
> >XID.
>
> This sounds good as long as queries are issued by a client application
> one after the other. But will it still work when we have
> subtransactions in functions?

Oh, you are right. I was thinking that changing the test to be
"xmin/xmax should be lesser than my current XID", but that also fails.
We'll have to continue incrementing CommandIds inside the transaction
tree.

> >Hm, why do you want to left out the startTime and startTimeUsec?
>
> I thought they're useless ...

Maybe they are. I don't really care much about them either.

> Maybe the term "nested transactions" is confusing. I'm starting to
> believe that my position is better described by "multiple savepoints".
> Are we still talking about the same thing?

The only difference so far is in the isolation level changeability
issue, I think. So yes, we are still talking the same.

I was thinking (much earlier, back in the days I started looking for a
useful project) that savepoints could be implementable on top of nested
transactions by means of having "named subtransactions", with special
commands like "COMMIT/ROLLBACK TO subxact-name".

But there are a lot of things to do before nested transactions become a
reality. For example there is need to abort any transaction, including
possible subtransactions, in the case of deadlock. How is this supposed
to be? Do we abort the whole transaction tree? Do we abort only the
branch of the tree that has the needed locks? I haven't even read the
deadlock code.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Nunca confiaré en un traidor. Ni siquiera si el traidor lo he creado yo"
(Barón Vladimir Harkonnen)

Alvaro Herrera <alvherre(at)dcc(dot)uchile(dot)cl> writes:
> But there are a lot of things to do before nested transactions become a
> reality. For example there is need to abort any transaction, including
> possible subtransactions, in the case of deadlock. How is this supposed
> to be? Do we abort the whole transaction tree? Do we abort only the
> branch of the tree that has the needed locks?

Deadlock is not different from any other elog(ERROR) condition: you
abort the innermost transaction.

regards, tom lane

On Sat, 17 May 2003 19:14:25 -0400, Alvaro Herrera
<alvherre(at)dcc(dot)uchile(dot)cl> wrote:
>> >The user can
>> >change from READ COMMITTED to SERIALIZABLE when starting a
>> >subtransaction, but not the other way around.
>>
>> You cannot propose this and agree to my three rules at the same time.
>> Rule 3 says that these two sequences of commands are equivalent:
>> [example]
>
>I see. Then I don't fully agree with your rules. Let's say I find that
>the rules are very good guidelines, but they fail WRT the isolation
>level, which is a special exception.

If there is not a compelling reason for making things more
complicated, I vote for implementing the most simple usable solution,
i.e. the whole transaction tree has to run with the same isolation
level.

If SERIALIZABLE subtransactions in a READ COMMITTED transaction are a
useful feature, this enhancement can be added later without breaking
compatibility.

BTW, do we have to invent a new syntax for starting and ending
subtransactions? COMMIT/ROLLBACK should be no problem. But does
BEGIN [subtransaction] conflict with BEGIN ... END in pl/pgslq?

Servus
Manfred

On Fri, May 23, 2003 at 12:17:21PM +0200, Manfred Koizar wrote:
> On Sat, 17 May 2003 19:14:25 -0400, Alvaro Herrera
> <alvherre(at)dcc(dot)uchile(dot)cl> wrote:

> >I see. Then I don't fully agree with your rules. Let's say I find that
> >the rules are very good guidelines, but they fail WRT the isolation
> >level, which is a special exception.
>
> If there is not a compelling reason for making things more
> complicated, I vote for implementing the most simple usable solution,
> i.e. the whole transaction tree has to run with the same isolation
> level.

Ok, I'll do this and if it's needed the other thing can be done later.

> BTW, do we have to invent a new syntax for starting and ending
> subtransactions? COMMIT/ROLLBACK should be no problem. But does
> BEGIN [subtransaction] conflict with BEGIN ... END in pl/pgslq?

I don't think we have to create a new syntax for starting a
subtransaction in the main parser. But the PL/pgSQL parser will have to
be changed somehow. I don't know a bit about parsers but maybe it's
possible to require a "BEGIN TRANSACTION" command to start a new
transaction so it doesn't conflicts with plpgsql's BEGIN. It'll be
confusing for sure if we don't do it this way, I think.

--
Alvaro Herrera (<alvherre[(at)]dcc(dot)uchile(dot)cl>)
Officer Krupke, what are we to do?
Gee, officer Krupke, Krup you! (West Side Story, "Gee, Officer Krupke")

Alvaro Herrera Munoz wrote:
> On Fri, May 23, 2003 at 12:17:21PM +0200, Manfred Koizar wrote:
> > On Sat, 17 May 2003 19:14:25 -0400, Alvaro Herrera
> > <alvherre(at)dcc(dot)uchile(dot)cl> wrote:
>
> > >I see. Then I don't fully agree with your rules. Let's say I find that
> > >the rules are very good guidelines, but they fail WRT the isolation
> > >level, which is a special exception.
> >
> > If there is not a compelling reason for making things more
> > complicated, I vote for implementing the most simple usable solution,
> > i.e. the whole transaction tree has to run with the same isolation
> > level.
>
> Ok, I'll do this and if it's needed the other thing can be done later.

Good.

>
> > BTW, do we have to invent a new syntax for starting and ending
> > subtransactions? COMMIT/ROLLBACK should be no problem. But does
> > BEGIN [subtransaction] conflict with BEGIN ... END in pl/pgslq?
>
> I don't think we have to create a new syntax for starting a
> subtransaction in the main parser. But the PL/pgSQL parser will have to
> be changed somehow. I don't know a bit about parsers but maybe it's
> possible to require a "BEGIN TRANSACTION" command to start a new
> transaction so it doesn't conflicts with plpgsql's BEGIN. It'll be
> confusing for sure if we don't do it this way, I think.

I don't think we will be starting new subtransactions in pl/pgsql, will
we? I suppose we could allow it some day, but I don't see a need to do
it right away.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Alvaro Herrera Munoz <alvherre(at)dcc(dot)uchile(dot)cl> writes:
> On Fri, May 23, 2003 at 12:17:21PM +0200, Manfred Koizar wrote:
>> If there is not a compelling reason for making things more
>> complicated, I vote for implementing the most simple usable solution,
>> i.e. the whole transaction tree has to run with the same isolation
>> level.

> Ok, I'll do this and if it's needed the other thing can be done later.

I think it would be a real bad idea to proceed on the assumption that
you won't be wanting this ability. There are many things that are
easier to do in SERIALIZABLE mode than otherwise --- especially so if
you can put an error-retry loop around the SERIALIZABLE subtransaction.
To implement such logic entirely in a server-side function, you *must*
be able to do serializable inside read-committed. (I'm assuming here
that when the outer transaction is serializable, every subtransaction
will use the same SerializableSnapshot as the outer transaction. But
then it is impossible to cope with changes committed by concurrent
xacts. You want an outer READ COMMITTED xact so that each successive
retry can start with an up-to-date SerializableSnapshot.)

You may care to read my slides from last year's O'Reilly conference,
if you are not convinced by the above argument.

>> BTW, do we have to invent a new syntax for starting and ending
>> subtransactions? COMMIT/ROLLBACK should be no problem. But does
>> BEGIN [subtransaction] conflict with BEGIN ... END in pl/pgslq?

> I don't think we have to create a new syntax for starting a
> subtransaction in the main parser.

We already have START TRANSACTION and COMMIT. I see no need to invent
any new syntax. We'll just legislate that BEGIN and END continue to
mean what they now do *inside plpgsql*. If you want to manipulate
subtransactions inside plpgsql, then you use the other syntaxes.

regards, tom lane

On Fri, 23 May 2003 13:15:07 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
wrote:
>[good reasons for having SERIALIZABLE subtransactions in READ
> COMMITTED main transactions]

All I'm saying is if we can have
(1) a simple version with some restrictions for 7.4 and
SERIALIZABLE subtransactions for 7.5 or
(2) nothing for 7.4 and everything for 7.5
I'd rather have (1); as long as we don't cause incompatibilities, of
course.

>We already have START TRANSACTION [...]

Great. I was so used to BEGIN that I didn't even think of trying \h.
:-/

Servus
Manfred

Manfred Koizar <mkoi-pg(at)aon(dot)at> writes:
> All I'm saying is if we can have
> (1) a simple version with some restrictions for 7.4 and
> SERIALIZABLE subtransactions for 7.5 or
> (2) nothing for 7.4 and everything for 7.5
> I'd rather have (1); as long as we don't cause incompatibilities, of
> course.

I don't object to that plan. The scenario I was sketching also requires
the ability to catch errors from subtransactions in plpgsql, which is
something not likely to be there in the first cut either. I'm just
saying not to paint yourself into a corner.

regards, tom lane

Manfred Koizar wrote:
> On Fri, 23 May 2003 13:15:07 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
> wrote:
> >[good reasons for having SERIALIZABLE subtransactions in READ
> > COMMITTED main transactions]
>
> All I'm saying is if we can have
> (1) a simple version with some restrictions for 7.4 and
> SERIALIZABLE subtransactions for 7.5 or
> (2) nothing for 7.4 and everything for 7.5
> I'd rather have (1); as long as we don't cause incompatibilities, of
> course.

Agreed. Let's get this boat in the water first unless it will be harder
add this functionality later.

> >We already have START TRANSACTION [...]
>
> Great. I was so used to BEGIN that I didn't even think of trying \h.
> :-/

I am just now realizing that because autocommit off is assumed, there
wasn't any being transaction statement in SQL92, and the only standard
one is in SQL99 and it is START TRANSACTION, not BEGIN WORK. I thought
BEGIN WORK was standard, but I guess not.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

After lots of discussion, it seems this is to be applied.

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---------------------------------------------------------------------------

Manfred Koizar wrote:
> Up to now, SerializableSnapshot and QuerySnapshot are malloc'ed and
> free'd for every transaction or statement, respectively. This patch
> puts these data structures into static memory, thus saving a few CPU
> cycles and two malloc calls per transaction or (in isolation level
> READ COMMITTED) per query.
>
> Servus
> Manfred

> diff -ruN ../base/src/backend/storage/ipc/sinval.c src/backend/storage/ipc/sinval.c
> --- ../base/src/backend/storage/ipc/sinval.c 2003-04-04 15:45:37.000000000 +0200
> +++ src/backend/storage/ipc/sinval.c 2003-05-08 21:39:15.000000000 +0200
> @@ -305,9 +305,8 @@
> *----------
> */
> Snapshot
> -GetSnapshotData(bool serializable)
> +GetSnapshotData(Snapshot snapshot, bool serializable)
> {
> - Snapshot snapshot = (Snapshot) malloc(sizeof(SnapshotData));
> SISeg *segP = shmInvalBuffer;
> ProcState *stateP = segP->procState;
> TransactionId xmin;
> @@ -316,18 +315,29 @@
> int index;
> int count = 0;
>
> - if (snapshot == NULL)
> - elog(ERROR, "Memory exhausted in GetSnapshotData");
> + Assert(snapshot != NULL);
>
> /*
> * Allocating space for MaxBackends xids is usually overkill;
> * lastBackend would be sufficient. But it seems better to do the
> * malloc while not holding the lock, so we can't look at lastBackend.
> + *
> + * if (snapshot->xip != NULL)
> + * no need to free and reallocate xip;
> + *
> + * We can reuse the old xip array, because MaxBackends does not change
> + * at runtime.
> */
> - snapshot->xip = (TransactionId *)
> - malloc(MaxBackends * sizeof(TransactionId));
> if (snapshot->xip == NULL)
> - elog(ERROR, "Memory exhausted in GetSnapshotData");
> + {
> + /*
> + * First call for this snapshot
> + */
> + snapshot->xip = (TransactionId *)
> + malloc(MaxBackends * sizeof(TransactionId));
> + if (snapshot->xip == NULL)
> + elog(ERROR, "Memory exhausted in GetSnapshotData");
> + }
>
> globalxmin = xmin = GetCurrentTransactionId();
>
> diff -ruN ../base/src/backend/utils/time/tqual.c src/backend/utils/time/tqual.c
> --- ../base/src/backend/utils/time/tqual.c 2003-04-04 15:45:45.000000000 +0200
> +++ src/backend/utils/time/tqual.c 2003-05-08 21:45:55.000000000 +0200
> @@ -30,6 +30,8 @@
> static SnapshotData SnapshotDirtyData;
> Snapshot SnapshotDirty = &SnapshotDirtyData;
>
> +static SnapshotData QuerySnapshotData;
> +static SnapshotData SerializableSnapshotData;
> Snapshot QuerySnapshot = NULL;
> Snapshot SerializableSnapshot = NULL;
>
> @@ -941,23 +943,16 @@
> /* 1st call in xaction? */
> if (SerializableSnapshot == NULL)
> {
> - SerializableSnapshot = GetSnapshotData(true);
> + SerializableSnapshot = GetSnapshotData(&SerializableSnapshotData, true);
> QuerySnapshot = SerializableSnapshot;
> Assert(QuerySnapshot != NULL);
> return;
> }
>
> - if (QuerySnapshot != SerializableSnapshot)
> - {
> - free(QuerySnapshot->xip);
> - free(QuerySnapshot);
> - QuerySnapshot = NULL;
> - }
> -
> if (XactIsoLevel == XACT_SERIALIZABLE)
> QuerySnapshot = SerializableSnapshot;
> else
> - QuerySnapshot = GetSnapshotData(false);
> + QuerySnapshot = GetSnapshotData(&QuerySnapshotData, false);
>
> Assert(QuerySnapshot != NULL);
> }
> @@ -1003,19 +998,11 @@
> void
> FreeXactSnapshot(void)
> {
> - if (QuerySnapshot != NULL && QuerySnapshot != SerializableSnapshot)
> - {
> - free(QuerySnapshot->xip);
> - free(QuerySnapshot);
> - }
> -
> + /*
> + * We do not free(QuerySnapshot->xip);
> + * or free(SerializableSnapshot->xip);
> + * they will be reused soon
> + */
> QuerySnapshot = NULL;
> -
> - if (SerializableSnapshot != NULL)
> - {
> - free(SerializableSnapshot->xip);
> - free(SerializableSnapshot);
> - }
> -
> SerializableSnapshot = NULL;
> }
> diff -ruN ../base/src/include/utils/tqual.h src/include/utils/tqual.h
> --- ../base/src/include/utils/tqual.h 2003-04-04 15:45:50.000000000 +0200
> +++ src/include/utils/tqual.h 2003-05-08 21:40:04.000000000 +0200
> @@ -113,7 +113,7 @@
> extern HTSV_Result HeapTupleSatisfiesVacuum(HeapTupleHeader tuple,
> TransactionId OldestXmin);
>
> -extern Snapshot GetSnapshotData(bool serializable);
> +extern Snapshot GetSnapshotData(Snapshot snapshot, bool serializable);
> extern void SetQuerySnapshot(void);
> extern Snapshot CopyQuerySnapshot(void);
> extern void FreeXactSnapshot(void);

>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo(at)postgresql(dot)org

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> After lots of discussion, it seems this is to be applied.

I'm still concerned that this will create problems for nested
transactions, while saving only an insignificant number of cycles per
transaction. I would suggest putting the idea on hold until the
dust has settled from nested transactions. If it's still workable
after that feature is complete, we can shave cycles then.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > After lots of discussion, it seems this is to be applied.
>
> I'm still concerned that this will create problems for nested
> transactions, while saving only an insignificant number of cycles per
> transaction. I would suggest putting the idea on hold until the
> dust has settled from nested transactions. If it's still workable
> after that feature is complete, we can shave cycles then.

My feeling was that Manfred/Alvero are dealing with nested transactions,
so if they both want the patch applied, we apply it. If they want it
back, they can grab it from CVS.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Patch applied. Thanks.

---------------------------------------------------------------------------

Manfred Koizar wrote:
> Up to now, SerializableSnapshot and QuerySnapshot are malloc'ed and
> free'd for every transaction or statement, respectively. This patch
> puts these data structures into static memory, thus saving a few CPU
> cycles and two malloc calls per transaction or (in isolation level
> READ COMMITTED) per query.
>
> Servus
> Manfred

> diff -ruN ../base/src/backend/storage/ipc/sinval.c src/backend/storage/ipc/sinval.c
> --- ../base/src/backend/storage/ipc/sinval.c 2003-04-04 15:45:37.000000000 +0200
> +++ src/backend/storage/ipc/sinval.c 2003-05-08 21:39:15.000000000 +0200
> @@ -305,9 +305,8 @@
> *----------
> */
> Snapshot
> -GetSnapshotData(bool serializable)
> +GetSnapshotData(Snapshot snapshot, bool serializable)
> {
> - Snapshot snapshot = (Snapshot) malloc(sizeof(SnapshotData));
> SISeg *segP = shmInvalBuffer;
> ProcState *stateP = segP->procState;
> TransactionId xmin;
> @@ -316,18 +315,29 @@
> int index;
> int count = 0;
>
> - if (snapshot == NULL)
> - elog(ERROR, "Memory exhausted in GetSnapshotData");
> + Assert(snapshot != NULL);
>
> /*
> * Allocating space for MaxBackends xids is usually overkill;
> * lastBackend would be sufficient. But it seems better to do the
> * malloc while not holding the lock, so we can't look at lastBackend.
> + *
> + * if (snapshot->xip != NULL)
> + * no need to free and reallocate xip;
> + *
> + * We can reuse the old xip array, because MaxBackends does not change
> + * at runtime.
> */
> - snapshot->xip = (TransactionId *)
> - malloc(MaxBackends * sizeof(TransactionId));
> if (snapshot->xip == NULL)
> - elog(ERROR, "Memory exhausted in GetSnapshotData");
> + {
> + /*
> + * First call for this snapshot
> + */
> + snapshot->xip = (TransactionId *)
> + malloc(MaxBackends * sizeof(TransactionId));
> + if (snapshot->xip == NULL)
> + elog(ERROR, "Memory exhausted in GetSnapshotData");
> + }
>
> globalxmin = xmin = GetCurrentTransactionId();
>
> diff -ruN ../base/src/backend/utils/time/tqual.c src/backend/utils/time/tqual.c
> --- ../base/src/backend/utils/time/tqual.c 2003-04-04 15:45:45.000000000 +0200
> +++ src/backend/utils/time/tqual.c 2003-05-08 21:45:55.000000000 +0200
> @@ -30,6 +30,8 @@
> static SnapshotData SnapshotDirtyData;
> Snapshot SnapshotDirty = &SnapshotDirtyData;
>
> +static SnapshotData QuerySnapshotData;
> +static SnapshotData SerializableSnapshotData;
> Snapshot QuerySnapshot = NULL;
> Snapshot SerializableSnapshot = NULL;
>
> @@ -941,23 +943,16 @@
> /* 1st call in xaction? */
> if (SerializableSnapshot == NULL)
> {
> - SerializableSnapshot = GetSnapshotData(true);
> + SerializableSnapshot = GetSnapshotData(&SerializableSnapshotData, true);
> QuerySnapshot = SerializableSnapshot;
> Assert(QuerySnapshot != NULL);
> return;
> }
>
> - if (QuerySnapshot != SerializableSnapshot)
> - {
> - free(QuerySnapshot->xip);
> - free(QuerySnapshot);
> - QuerySnapshot = NULL;
> - }
> -
> if (XactIsoLevel == XACT_SERIALIZABLE)
> QuerySnapshot = SerializableSnapshot;
> else
> - QuerySnapshot = GetSnapshotData(false);
> + QuerySnapshot = GetSnapshotData(&QuerySnapshotData, false);
>
> Assert(QuerySnapshot != NULL);
> }
> @@ -1003,19 +998,11 @@
> void
> FreeXactSnapshot(void)
> {
> - if (QuerySnapshot != NULL && QuerySnapshot != SerializableSnapshot)
> - {
> - free(QuerySnapshot->xip);
> - free(QuerySnapshot);
> - }
> -
> + /*
> + * We do not free(QuerySnapshot->xip);
> + * or free(SerializableSnapshot->xip);
> + * they will be reused soon
> + */
> QuerySnapshot = NULL;
> -
> - if (SerializableSnapshot != NULL)
> - {
> - free(SerializableSnapshot->xip);
> - free(SerializableSnapshot);
> - }
> -
> SerializableSnapshot = NULL;
> }
> diff -ruN ../base/src/include/utils/tqual.h src/include/utils/tqual.h
> --- ../base/src/include/utils/tqual.h 2003-04-04 15:45:50.000000000 +0200
> +++ src/include/utils/tqual.h 2003-05-08 21:40:04.000000000 +0200
> @@ -113,7 +113,7 @@
> extern HTSV_Result HeapTupleSatisfiesVacuum(HeapTupleHeader tuple,
> TransactionId OldestXmin);
>
> -extern Snapshot GetSnapshotData(bool serializable);
> +extern Snapshot GetSnapshotData(Snapshot snapshot, bool serializable);
> extern void SetQuerySnapshot(void);
> extern Snapshot CopyQuerySnapshot(void);
> extern void FreeXactSnapshot(void);

>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo(at)postgresql(dot)org

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073