Обсуждение: LockDatabaseObject vs. LockSharedObject
It seems suspicious to me that LockSharedObject() calls AcceptInvalidationMessges() and LockDatabaseObject() does not. Since the only caller of LockSharedObject() at present is AcquireDeletionLock(), I'm not sure there's an observable bug here at the moment, but then again, I'm also not sure there isn't. The call in LockSharedObject() was added here: http://archives.postgresql.org/pgsql-committers/2006-05/msg00026.php -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> writes:
> It seems suspicious to me that LockSharedObject() calls
> AcceptInvalidationMessges() and LockDatabaseObject() does not. Since
> the only caller of LockSharedObject() at present is
> AcquireDeletionLock(), I'm not sure there's an observable bug here at
> the moment, but then again, I'm also not sure there isn't.
ITYM the only caller of LockDatabaseObject is AcquireDeletionLock.
Given that the other logic path in AcquireDeletionLock calls
LockRelationOid, which *will* result in an AcceptInvalidationMessages
call, it does seem pretty suspicious. The type of bug that you'd
expect to have from this is that a recent DDL change on a non-relation
object might not be seen by a concurrent drop being done on that object.
I'm not sure that we have any non-relation objects that are both complex
enough and changeable enough for there to be an observable bug here,
but it seems like a risk factor going forward. It seems to me both safe
and reasonable to add an AcceptInvalidationMessages call in HEAD.
regards, tom lane
On Sun, Aug 15, 2010 at 3:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> It seems suspicious to me that LockSharedObject() calls >> AcceptInvalidationMessges() and LockDatabaseObject() does not. Since >> the only caller of LockSharedObject() at present is >> AcquireDeletionLock(), I'm not sure there's an observable bug here at >> the moment, but then again, I'm also not sure there isn't. > > ITYM the only caller of LockDatabaseObject is AcquireDeletionLock. Right, sorry. > Given that the other logic path in AcquireDeletionLock calls > LockRelationOid, which *will* result in an AcceptInvalidationMessages > call, it does seem pretty suspicious. The type of bug that you'd > expect to have from this is that a recent DDL change on a non-relation > object might not be seen by a concurrent drop being done on that object. That's what I was thinking, too. > I'm not sure that we have any non-relation objects that are both complex > enough and changeable enough for there to be an observable bug here, > but it seems like a risk factor going forward. It seems to me both safe > and reasonable to add an AcceptInvalidationMessages call in HEAD. My "comment refactoring" patch (already posted to the list) makes this change among others. With the added locking introduce by that patch, it's possible to demonstrate a live bug here without AcceptInvalidationMessages(). I'd sort of like to get that committed, by the way, though I haven't had time to revise it yet per the feedback already received. To get anything useful done for SE-PostgreSQL this release is going to require AT LEAST one more good-sized patch after that one, and I'd like to not be landing it at the very end of the release cycle when there's no time for second or third thoughts. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Aug 15, 2010 at 3:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm not sure that we have any non-relation objects that are both complex
>> enough and changeable enough for there to be an observable bug here,
>> but it seems like a risk factor going forward. �It seems to me both safe
>> and reasonable to add an AcceptInvalidationMessages call in HEAD.
> My "comment refactoring" patch (already posted to the list) makes this
> change among others.
OK. I haven't read that yet, and wasn't intending to until you respond
to Alvaro's suggestions. But it might be worth committing this change
separately, since it seems to have merit independently of whether we
make those other changes.
regards, tom lane