Hello all,

Cache invalidation mechanism was much improved. 
Thanks to Tom.

But as far as I see,neither relation cache nor system catalog cache 
aren't be rollbacked correctly.
This should be solved if we would execute DDL statement inside 
transactions. 

For example,

	create table t1 (id int4);
	CREATE
	begin;
	BEGIN
	alter table t1 add column dt1 text;
	ADD
	select * from t1;
	id|dt1
	--+---
	(0 rows)

	abort;
	ABORT
	visco=> select * from t1;
	id|dt1
	--+---
	(0 rows)

I added time_qualification_check to SearchSysCache() on trial
(see the patch at the end of this posting).

After this change,
	.
	.
	abort;
	ABORT
	select * from t1;
	ERROR:  Relation t1 does not have attribute dt1

Seems relation cache is not invalidated yet.
I also tried to add time_qualification_check to RelationId(Name)-
CacheGetRelation(). But unfortunately,Relation doesn't have
such a information.

Any ideas ?
Comments ?

Regards.

Hiroshi Inoue
Inoue@tpf.co.jp

*** utils/cache/catcache.c.orig	Mon Jul 26 12:45:14 1999
--- utils/cache/catcache.c	Fri Sep 17 08:57:50 1999
***************
*** 872,878 ****
  					cache->cc_skey,
  					res);
  		if (res)
! 			break;
  	}
  
  	/* ----------------
--- 872,881 ----
  					cache->cc_skey,
  					res);
  		if (res)
! 		{
! 			if (HeapTupleSatisfiesNow(ct->ct_tup->t_data))
! 				break;
! 		}
  	}
  
  	/* ----------------


Fri, 17 Sep 1999 10:40:48 +0900
"Hiroshi Inoue" <Inoue@tpf.co.jp>
couldn't rollback cache ?
"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> But as far as I see,neither relation cache nor system catalog cache 
> aren't be rollbacked correctly.
> I added time_qualification_check to SearchSysCache() on trial
> (see the patch at the end of this posting).

Hmm.  This must be a bug of very long standing; surprising it hasn't
been noticed before.  I think you are probably right, because a little
glimpsing shows that SearchSysCache() is the *only* place in the whole
system where HeapKeyTest() is called directly --- everyone else goes
through HeapTupleSatisfies() which adds a timequal check of one sort or
another.  I don't know the timequal stuff at all, but it seems likely
that we want one here.  (Vadim, is this fix right?)

> After this change,
> 	abort;
> 	ABORT
> 	select * from t1;
> 	ERROR:  Relation t1 does not have attribute dt1

> Seems relation cache is not invalidated yet.
> I also tried to add time_qualification_check to RelationId(Name)-
> CacheGetRelation(). But unfortunately,Relation doesn't have
> such a information.

I think the real bug here is in inval.c: see
InvalidationMessageCacheInvalidate, which scans pending SI messages
at abort time.  If we had committed, we'd have sent an SI message
telling other backends to refresh their relcache entries for t1;
so there is an entry for t1 in the pending-SI-message list.  We can
use that entry to tell us to invalidate our own relcache entry instead.
It looks like this is done correctly for tuple SI messages but not for
relation SI messages --- and in fact the code sez
			/* XXX ignore this--is this correct ??? */
Evidently not.  (BTW, please add some comments to this code!  It's
not obvious that what it's doing is throwing away cache entries that
have been changed by a transaction now being aborted.)

It would probably be a good idea to switch the order of the operations
in AtAbort_Cache() in xact.c, so that relcache reference counts get
reset to 0 before we do the pending-SI-message scan.  That way, we could
just discard the bogus relcache entry and not waste time rebuilding an
entry we might not need again.  (This might even be essential to avoid
an error in the aborted-table-create case; not sure.  The routine
RelationCacheAbort() didn't exist till last week, so the present call
order is certainly not gospel.)

We could probably do this in other ways too, like marking all relcache
entries with the transaction ID of their last change and using that to
detect what to throw away.  But the SI message queue is already there
so might as well use it...

			regards, tom lane

Fri, 17 Sep 1999 09:57:33 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] couldn't rollback cache ? 
Tom Lane wrote:
> 
> "Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> > But as far as I see,neither relation cache nor system catalog cache
> > aren't be rollbacked correctly.
> > I added time_qualification_check to SearchSysCache() on trial
> > (see the patch at the end of this posting).
> 
> Hmm.  This must be a bug of very long standing; surprising it hasn't
> been noticed before.  I think you are probably right, because a little
> glimpsing shows that SearchSysCache() is the *only* place in the whole
> system where HeapKeyTest() is called directly --- everyone else goes
> through HeapTupleSatisfies() which adds a timequal check of one sort or
> another.  I don't know the timequal stuff at all, but it seems likely
> that we want one here.  (Vadim, is this fix right?)

Sorry, but currently I have no ability to deal with anything
but WAL. As for cache/SI issues, I would like to see shared 
catalog cache implemented (and remove current SI stuff),
but I was not able to do it for ~ 2 years, -:(

Vadim

Mon, 20 Sep 1999 10:17:18 +0800
Vadim Mikheev <vadim@krs.ru>
Re: [HACKERS] couldn't rollback cache ?
> -----Original Message-----
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> Sent: Friday, September 17, 1999 10:58 PM
> To: Hiroshi Inoue
> Cc: pgsql-hackers
> Subject: Re: [HACKERS] couldn't rollback cache ? 
> 
> 
> "Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> > But as far as I see,neither relation cache nor system catalog cache 
> > aren't be rollbacked correctly.
> > I added time_qualification_check to SearchSysCache() on trial
> > (see the patch at the end of this posting).
> 
> Hmm.  This must be a bug of very long standing; surprising it hasn't
> been noticed before.  I think you are probably right, because a little
> glimpsing shows that SearchSysCache() is the *only* place in the whole
> system where HeapKeyTest() is called directly --- everyone else goes
> through HeapTupleSatisfies() which adds a timequal check of one sort or
> another.  I don't know the timequal stuff at all, but it seems likely
> that we want one here.  (Vadim, is this fix right?)
> 
> > After this change,
> > 	abort;
> > 	ABORT
> > 	select * from t1;
> > 	ERROR:  Relation t1 does not have attribute dt1
> 
> > Seems relation cache is not invalidated yet.
> > I also tried to add time_qualification_check to RelationId(Name)-
> > CacheGetRelation(). But unfortunately,Relation doesn't have
> > such a information.
> 
> I think the real bug here is in inval.c: see
> InvalidationMessageCacheInvalidate, which scans pending SI messages
> at abort time.  If we had committed, we'd have sent an SI message
> telling other backends to refresh their relcache entries for t1;
> so there is an entry for t1 in the pending-SI-message list.  We can
> use that entry to tell us to invalidate our own relcache entry instead.
> It looks like this is done correctly for tuple SI messages but not for

I think it's not done correctly for tuple SI messages either.
I didn't use current cache invalidation mechanism when I made the
patch for SearchSysCache() because of the following 2 reasons. 

1. SI messages are eaten by CommandCounterIncrement(). So they
    may vanish before transaction end/aborts.
2. The tuples which should be invalidated in case of abort are different
    from ones in case of commit.
    In case of commit,deleting old tuples should be invalidated for all
    backends.
    In case of abort,insert(updat)ing new tuples should be invalidated 
    for the insert(updat)ing backend.
    Currently heap_insert() calls RelationInvalidateHeapTuple() for a 
    inserting new tuple but heap_replace() doesn't call RelationInvalid-
    ateHeapTuple() for a updating new tuple. I don't understand which
    is right.

Regards.

Hiroshi Inoue
Inoue@tpf.co.jp

Mon, 20 Sep 1999 11:47:49 +0900
"Hiroshi Inoue" <Inoue@tpf.co.jp>
RE: [HACKERS] couldn't rollback cache ? 
"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> I think it's not done correctly for tuple SI messages either.
> I didn't use current cache invalidation mechanism when I made the
> patch for SearchSysCache() because of the following 2 reasons. 

> 1. SI messages are eaten by CommandCounterIncrement(). So they
>     may vanish before transaction end/aborts.

I think this is OK.  The sending backend does not send the SI message
in the first place until it has committed.  Other backends can read
the messages at CommandCounterIncrement; it doesn't matter whether the
other backends later commit or abort their own transactions.  I think.
Do you have a counterexample?

> 2. The tuples which should be invalidated in case of abort are different
>     from ones in case of commit.
>     In case of commit,deleting old tuples should be invalidated for all
>     backends.
>     In case of abort,insert(updat)ing new tuples should be invalidated 
>     for the insert(updat)ing backend.

I wonder whether it wouldn't be cleaner to identify the target tuples
by OID instead of ItemPointer.  That way would work for both new and
update tuples...

>     Currently heap_insert() calls RelationInvalidateHeapTuple() for a 
>     inserting new tuple but heap_replace() doesn't call RelationInvalid-
>     ateHeapTuple() for a updating new tuple. I don't understand which
>     is right.

Hmm.  Invalidating the old tuple is the right thing for heap_replace in
terms of sending a message to other backends at commit; it's the old
tuple that they might have cached and need to get rid of.  But for
getting rid of this backend's uncommitted new tuple in case of abort,
it's not the right thing.  OTOH, your change to add a time qual check
to SearchSysCache would fix that, wouldn't it?  But invalidating by OID
would make the issue moot.

Possibly heap_insert doesn't need to be calling
RelationInvalidateHeapTuple at all --- a new tuple can't be cached by
any other backend, by definition, until it has committed; so there's no
need to send out an SI message for it.  That call must be there to
ensure that the local cache gets purged of the tuple in case of abort.
Maybe we could remove that call (and reduce SI traffic) if we rely on
a time qual to purge bogus entries from the local caches after abort.

			regards, tom lane

Mon, 20 Sep 1999 10:28:08 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] couldn't rollback cache ? 
> -----Original Message-----
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> Sent: Monday, September 20, 1999 11:28 PM
> To: Hiroshi Inoue
> Cc: pgsql-hackers
> Subject: Re: [HACKERS] couldn't rollback cache ?
>
>
> "Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> > I think it's not done correctly for tuple SI messages either.
> > I didn't use current cache invalidation mechanism when I made the
> > patch for SearchSysCache() because of the following 2 reasons.
>
> > 1. SI messages are eaten by CommandCounterIncrement(). So they
> >     may vanish before transaction end/aborts.
>
> I think this is OK.  The sending backend does not send the SI message
> in the first place until it has committed.  Other backends can read

Doesn't the sending backend send the SI message when Command-
CounterIncrement() is executed ?
AtCommit_Cache() is called not only from CommitTransaction() but
also from CommandCounterIncrement().

AtCommit_Cache() in CommandCounterIncrement() eats local
invalidation messages and register SI information (this seems too
early for other backends though it's not so harmful). Then AtAtart_
Cache() eats the SI information and invalidates related syscache
and relcache for the backend(this seems right). At this point,invali-
dation info for the backend vanishes. Isn't it right ?

> the messages at CommandCounterIncrement; it doesn't matter whether the
> other backends later commit or abort their own transactions.  I think.
> Do you have a counterexample?
>
> > 2. The tuples which should be invalidated in case of abort are different
> >     from ones in case of commit.
> >     In case of commit,deleting old tuples should be invalidated for all
> >     backends.
> >     In case of abort,insert(updat)ing new tuples should be invalidated
> >     for the insert(updat)ing backend.
>
> I wonder whether it wouldn't be cleaner to identify the target tuples
> by OID instead of ItemPointer.  That way would work for both new and
> update tuples...
>

This may be a better way because the cache entry which should be
invalidated are invalidated.
However,we may invalidate still valid cache entry by OID(it's not so
harmful).  Even time qualification is useless in this case.

> >     Currently heap_insert() calls RelationInvalidateHeapTuple() for a
> >     inserting new tuple but heap_replace() doesn't call RelationInvalid-
> >     ateHeapTuple() for a updating new tuple. I don't understand which
> >     is right.
>
> Hmm.  Invalidating the old tuple is the right thing for heap_replace in
> terms of sending a message to other backends at commit; it's the old
> tuple that they might have cached and need to get rid of.  But for
> getting rid of this backend's uncommitted new tuple in case of abort,
> it's not the right thing.  OTOH, your change to add a time qual check
> to SearchSysCache would fix that, wouldn't it?

Probably.  Because time qualification is applied for uncommitted tuples.

Regards.

Hiroshi Inoue
Inoue@tpf.co.jp


Tue, 21 Sep 1999 18:58:54 +0900
"Hiroshi Inoue" <Inoue@tpf.co.jp>
RE: [HACKERS] couldn't rollback cache ? 
"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
>> I think this is OK.  The sending backend does not send the SI message
>> in the first place until it has committed.  Other backends can read

> Doesn't the sending backend send the SI message when Command-
> CounterIncrement() is executed ?
> AtCommit_Cache() is called not only from CommitTransaction() but
> also from CommandCounterIncrement().

Oooh, you are right.  I think that is a bug.  We should postpone
sending SI messages until commit.  Also, if I recall correctly,
CommandCounterIncrement() still gets called after we have decided to
abort the current transaction (while we are eating commands looking for
END or ABORT).  It shouldn't do anything to the pending-SI list then
either.

> AtCommit_Cache() in CommandCounterIncrement() eats local
> invalidation messages and register SI information (this seems too
> early for other backends though it's not so harmful). Then AtAtart_
> Cache() eats the SI information and invalidates related syscache
> and relcache for the backend(this seems right). At this point,invali-
> dation info for the backend vanishes. Isn't it right ?

I think it is OK for AtStart_Cache to read *incoming* SI messages,
if those relate to transactions that other backends have committed.
But we should sit on our own list of pending outgoing messages until
we know we are committing (or aborting).

>> I wonder whether it wouldn't be cleaner to identify the target tuples
>> by OID instead of ItemPointer.  That way would work for both new and
>> update tuples...

> This may be a better way because the cache entry which should be
> invalidated are invalidated.
> However,we may invalidate still valid cache entry by OID(it's not so
> harmful).  Even time qualification is useless in this case.

Doesn't bother me --- we'll just re-read it.  We'd have to do some work
in that case anyway to verify whether we have the correct copy of the
tuple.

>> OTOH, your change to add a time qual check
>> to SearchSysCache would fix that, wouldn't it?

> Probably.  Because time qualification is applied for uncommitted tuples.

One thing we need to think about here: as it stands, the syscache will
only store a single copy of any particular tuple (maybe I should say
"of a particular OID").  But there might be several copies of that tuple
with different t_min/t_max in the database.  With your change to check
time qual, as soon as we realize that the copy we have no longer
SatisfiesNow(), we'll go look for a new copy.  And we'll go look
for a new copy after receiving a SI message indicating someone else has
committed an update.  The question is, are there any *other* times where
we need to look for a new copy?  I think we are OK if we change SI
message sending to only send after commit, but I'm not sure about it.

			regards, tom lane

Tue, 21 Sep 1999 10:43:51 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] couldn't rollback cache ? 
> 
> "Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> >> I think this is OK.  The sending backend does not send the SI message
> >> in the first place until it has committed.  Other backends can read
> 
> > Doesn't the sending backend send the SI message when Command-
> > CounterIncrement() is executed ?
> > AtCommit_Cache() is called not only from CommitTransaction() but
> > also from CommandCounterIncrement().
> 
> Oooh, you are right.  I think that is a bug.  We should postpone
> sending SI messages until commit.  Also, if I recall correctly,
> CommandCounterIncrement() still gets called after we have decided to
> abort the current transaction (while we are eating commands looking for
> END or ABORT).  It shouldn't do anything to the pending-SI list then
> either.
> 
> > AtCommit_Cache() in CommandCounterIncrement() eats local
> > invalidation messages and register SI information (this seems too
> > early for other backends though it's not so harmful). Then AtAtart_
> > Cache() eats the SI information and invalidates related syscache
> > and relcache for the backend(this seems right). At this point,invali-
> > dation info for the backend vanishes. Isn't it right ?
> 
> I think it is OK for AtStart_Cache to read *incoming* SI messages,
> if those relate to transactions that other backends have committed.
> But we should sit on our own list of pending outgoing messages until
> we know we are committing (or aborting).
>

What about delet(updat)ing backend itself ?
Shouldn't the backend invalidate delet(updat)ing old tuples ?
 
> >> I wonder whether it wouldn't be cleaner to identify the target tuples
> >> by OID instead of ItemPointer.  That way would work for both new and
> >> update tuples...
>

[snip]
 
> One thing we need to think about here: as it stands, the syscache will
> only store a single copy of any particular tuple (maybe I should say
> "of a particular OID").  But there might be several copies of that tuple
> with different t_min/t_max in the database.  With your change to check
> time qual, as soon as we realize that the copy we have no longer
> SatisfiesNow(), we'll go look for a new copy.  And we'll go look
> for a new copy after receiving a SI message indicating someone else has
> committed an update.  The question is, are there any *other* times where
> we need to look for a new copy?  I think we are OK if we change SI
> message sending to only send after commit, but I'm not sure about it.
>

What kind of tuples are read into system catalog cache ?
And what should we do to invalidate the tuples just in time ?
As far as I see,

1. HEAP_XMIN_INVALID
    i.e the tuple wasn't inserted 
    No backend regards this tuple as valid. 

2. HEAP_XMIN_??????? && HEAP_XMAX_INVALID
    i.e the tuple is being inserted now. 
    Only inserting backend regards this tuple as valid.
    Time qualification check which my patch does would 
    work in this case.
    Otherwise SI message should be sent to the backend 
    when insertion is rollbacked.

3. HEAP_XMIN_??????? && HEAP_XMAX_???????
    i.e the tuple is being deleted after insertion in a transaction
    now. 
    No backend regards this tuple as valid.

4. HEAP_XMIN_COMMITTED && HEAP_XMAX_INVALID
    i.e the tuple is inserted,not deleted and not being deleted.
    HeapTupleSatisifies..() doesn't take effect.
    SI message should be sent to all backends immediately
    after the tuple was deleted.
    SI message should be sent to a backend immediately after
    the backend marked the tuple as being deleted.

5. HEAP_XMIN_COMMITTED && HEAP_XMAX_???????
    i.e the tuple is being deleted now. 
    Deleting backend doesn't regard this tuple as valid.
    If SI messages are postponed to send for other backends
    until commit,the tuple is invalidated correctly.
    Otherwise a check as my patch does would be necessary.

6. HEAP_XMAX_COMMITTED
    i.e the tuple is deleted
    SnapshotNow never regard this tuple as valid.

Regards.
 
Hiroshi Inoue
Inoue@tpf.co.jp 

Wed, 22 Sep 1999 19:12:00 +0900
"Hiroshi Inoue" <Inoue@tpf.co.jp>
RE: [HACKERS] couldn't rollback cache ? 