Thread: ImmediateSharedRelationCacheInvalidate considered harmful
Hiroshi, I have been looking at the cache invalidation changes you committed
on 10 Jan. Most of them look fine, but I am suspicious of the routine
ImmediateSharedRelationCacheInvalidate, which you added for md.c to
call when it truncates or removes a relation. I believe that this
routine is unnecessary, and since it makes for a very ugly linkage
between md.c and the cache code, I would like to take it out again.
I think it is unnecessary because no backend should ever be deleting
or truncating a relation unless it has an exclusive lock on the
relation. It should be impossible for any other backend to try to
touch the relation until it's acquired some kind of lock on the relation
--- which cannot happen until the deleting/truncating transaction
commits, by which time it will have sent the normal SI message for the
relation's pg_class tuple. And since LockRelation processes SI messages
after acquiring the relation's lock, the other backend should have seen
the SI update before it can do anything with the relation.
Of course, the other backend may have open file descriptors for the
relation's file(s), but so what? The descriptors will get closed when
the SI message is processed, before they can be used for anything; so
the only consequence is that the Unix kernel won't release the physical
file storage until then.
Furthermore, if it *were* necessary to force other backends to react
immediately to md.c's truncation or deletion, the SI message mechanism
will not do the trick, even if a message is sent instantly; there is
no guarantee that other backends will process it promptly.
So I can see no value in this code. Have I missed something?
regards, tom lane
> -----Original Message----- > From: Tom Lane [mailto:tgl@sss.pgh.pa.us] > > Hiroshi, > I have been looking at the cache invalidation changes you committed > on 10 Jan. Most of them look fine, but I am suspicious of the routine > ImmediateSharedRelationCacheInvalidate, which you added for md.c to > call when it truncates or removes a relation. I believe that this > routine is unnecessary, and since it makes for a very ugly linkage > between md.c and the cache code, I would like to take it out again. > I'm happy you have noticed it. The call is for abort/crash after mdunlink()/mdtruncate(). mdunlink()/mdtruncate() is executed immediately but SI registration for all backends isn't executed until commit. Yes the call is ugly and it doesn't solve the flaw basically. Transaction control of relation files' handling would solve it basically. Though I have hesitated to add the call,after all I added it because it may be brought to developers' notice. I don't mind to take it out BTW strictly speaking,even a possibilty exists that backends fail between RecordTransactionCommit() and AtCommit_Cache() in CommitTransaction(). This isn't so little a problem if we want a really strict consistency but seems very hard to solve. Regards. Hiroshi Inoue Inoue@tpf.co.jp
"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
>> I have been looking at the cache invalidation changes you committed
>> on 10 Jan. Most of them look fine, but I am suspicious of the routine
>> ImmediateSharedRelationCacheInvalidate, which you added for md.c to
>> call when it truncates or removes a relation. I believe that this
>> routine is unnecessary, and since it makes for a very ugly linkage
>> between md.c and the cache code, I would like to take it out again.
> The call is for abort/crash after mdunlink()/mdtruncate().
> mdunlink()/mdtruncate() is executed immediately but
> SI registration for all backends isn't executed until commit.
> Yes the call is ugly and it doesn't solve the flaw basically.
Right. As the code currently stands, we are up the creek with no
paddle if an abort occurs after mdunlink/mdtruncate. The only real
solution is to postpone these operations until after commit, which
can only be done if we change the naming convention for relation files.
I think we are drifting towards a consensus that that has to be done.
So the question is, does ImmediateSharedRelationCacheInvalidate add
any useful amount of (incomplete) robustness in the meantime?
I'm not sure --- but since it's not a step towards a real solution,
I'm inclined to leave it out.
Just my $0.02 worth; if you think it's better to leave it in until
we have a complete solution, I will go along.
> BTW strictly speaking,even a possibilty exists that backends
> fail between RecordTransactionCommit() and AtCommit_Cache()
> in CommitTransaction(). This isn't so little a problem if we want
> a really strict consistency but seems very hard to solve.
Hmm, haven't looked at that...
regards, tom lane
-----Original Message----- > From: Tom Lane [mailto:tgl@sss.pgh.pa.us] > Sent: Sunday, January 30, 2000 1:19 PM > To: Hiroshi Inoue > Cc: pgsql-hackers@postgreSQL.org > Subject: Re: ImmediateSharedRelationCacheInvalidate considered harmful > > > "Hiroshi Inoue" <Inoue@tpf.co.jp> writes: > >> I have been looking at the cache invalidation changes you committed > >> on 10 Jan. Most of them look fine, but I am suspicious of the routine > >> ImmediateSharedRelationCacheInvalidate, which you added for md.c to > >> call when it truncates or removes a relation. I believe that this > >> routine is unnecessary, and since it makes for a very ugly linkage > >> between md.c and the cache code, I would like to take it out again. > > > The call is for abort/crash after mdunlink()/mdtruncate(). > > mdunlink()/mdtruncate() is executed immediately but > > SI registration for all backends isn't executed until commit. > > Yes the call is ugly and it doesn't solve the flaw basically. > > Right. As the code currently stands, we are up the creek with no > paddle if an abort occurs after mdunlink/mdtruncate. The only real > solution is to postpone these operations until after commit, which > can only be done if we change the naming convention for relation files. > I think we are drifting towards a consensus that that has to be done. > > So the question is, does ImmediateSharedRelationCacheInvalidate add > any useful amount of (incomplete) robustness in the meantime? > I'm not sure --- but since it's not a step towards a real solution, > I'm inclined to leave it out. > OK,I would remove the call. Regards. Hiroshi Inoue Inoue@tpf.co.jp