Thread: Bug? Concurrent COMMENT ON and DROP object
In the following scenario, we can see orphan comments. session.1 session.2 ---------------------- ---------------------- 1: CREATE TYPE my_typ AS (a int, b text); 2: BEGIN; 3: COMMENT ON TYPE my_typ IS 'testtest'; 4: DROP TYPE my_typ; 5: COMMIT; SELECT * FROM pg_description WHERE description = 'testtest'; objoid | classoid | objsubid | description --------+----------+----------+------------- 16393 | 1247 | 0 | testtest (1 row) ---------------------- ---------------------- The CommentRelation() has the following code: | static void | CommentRelation(int objtype, List *relname, char *comment) | { | Relation relation; | RangeVar *tgtrel; | | tgtrel = makeRangeVarFromNameList(relname); | | /* | * Open the relation. We do this mainly to acquire a lock that ensures no | * one else drops the relation before we commit. (If they did, they'd | * fail to remove the entry we are about to make in pg_description.) | */ | relation = relation_openrv(tgtrel, AccessShareLock); | : | : | /* Done, but hold lock until commit */ | relation_close(relation, NoLock); | } It says the purpose of the relation_openrv() to acquire a lock that ensures no one else drops the relation before we commit. So, I was blocked when I tried to comment on the table which was already dropped in another session but uncommited yet. However, it is not a problem limited to relations. For example, we need to acquire a lock on the pg_type catalog using For example, we need to acquire a lock on the pg_type catalog when we try to comment on any type object. Perhaps, I think LockRelationOid() should be injected at head of the CommentType() in this case. Any comments? -- KaiGai Kohei <kaigai@ak.jp.nec.com>
2010/7/6 KaiGai Kohei <kaigai@ak.jp.nec.com>: > In the following scenario, we can see orphan comments. Yeah. I think the reason we haven't seen any complaints about this before is that the worst-case scenario is that a comment for a dropped database object eventually becomes associated with a new database object. But that can only happen if the OID counter wraps around and then OID then gets reused for another object of the same type, and even then you might easily fail to notice. Still, it would be nice to clean this up. > It says the purpose of the relation_openrv() to acquire a lock that > ensures no one else drops the relation before we commit. So, I was > blocked when I tried to comment on the table which was already dropped > in another session but uncommited yet. > However, it is not a problem limited to relations. For example, we need > to acquire a lock on the pg_type catalog using > > For example, we need to acquire a lock on the pg_type catalog when we > try to comment on any type object. Perhaps, I think LockRelationOid() > should be injected at head of the CommentType() in this case. > > Any comments? A more fine-grained lock would be preferable, if we can manage it. Can we just lock the relevant pg_type tuple, rather than the whole relation? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> writes: > 2010/7/6 KaiGai Kohei <kaigai@ak.jp.nec.com>: >> In the following scenario, we can see orphan comments. > Yeah. I think the reason we haven't seen any complaints about this > before is that the worst-case scenario is that a comment for a dropped > database object eventually becomes associated with a new database > object. Well, in general there is very little DDL locking for any object type other than tables. I think the original rationale for that was that most other object types are defined by single catalog entries, so that attempts to update/delete the object would naturally block on changing its tuple anyway. But between comments and pg_depend entries that seems not particularly true anymore. IIRC there is now some attempt to lock objects of all types during DROP. Maybe the COMMENT code could acquire a conflicting lock. >> For example, we need to acquire a lock on the pg_type catalog when we >> try to comment on any type object. Perhaps, I think LockRelationOid() >> should be injected at head of the CommentType() in this case. >> >> Any comments? > A more fine-grained lock would be preferable, s/preferable/essential/. This cure would be *far* worse than the disease. Can you say "deadlock"? regards, tom lane
(2010/07/06 23:33), Tom Lane wrote: > Robert Haas<robertmhaas@gmail.com> writes: >> 2010/7/6 KaiGai Kohei<kaigai@ak.jp.nec.com>: >>> In the following scenario, we can see orphan comments. > >> Yeah. I think the reason we haven't seen any complaints about this >> before is that the worst-case scenario is that a comment for a dropped >> database object eventually becomes associated with a new database >> object. > > Well, in general there is very little DDL locking for any object type > other than tables. I think the original rationale for that was that > most other object types are defined by single catalog entries, so that > attempts to update/delete the object would naturally block on changing > its tuple anyway. But between comments and pg_depend entries that seems > not particularly true anymore. > > IIRC there is now some attempt to lock objects of all types during > DROP. Maybe the COMMENT code could acquire a conflicting lock. > Are you saying AcquireDeletionLock()? It seems to me fair enough to prevent the problem, although it is declared as a static function. >>> For example, we need to acquire a lock on the pg_type catalog when we >>> try to comment on any type object. Perhaps, I think LockRelationOid() >>> should be injected at head of the CommentType() in this case. >>> >>> Any comments? > >> A more fine-grained lock would be preferable, > > s/preferable/essential/. This cure would be *far* worse than the > disease. Can you say "deadlock"? > > regards, tom lane > -- KaiGai Kohei <kaigai@ak.jp.nec.com>
2010/7/6 KaiGai Kohei <kaigai@ak.jp.nec.com>: > (2010/07/06 23:33), Tom Lane wrote: >> Robert Haas<robertmhaas@gmail.com> writes: >>> 2010/7/6 KaiGai Kohei<kaigai@ak.jp.nec.com>: >>>> In the following scenario, we can see orphan comments. >> >>> Yeah. I think the reason we haven't seen any complaints about this >>> before is that the worst-case scenario is that a comment for a dropped >>> database object eventually becomes associated with a new database >>> object. >> >> Well, in general there is very little DDL locking for any object type >> other than tables. I think the original rationale for that was that >> most other object types are defined by single catalog entries, so that >> attempts to update/delete the object would naturally block on changing >> its tuple anyway. But between comments and pg_depend entries that seems >> not particularly true anymore. >> >> IIRC there is now some attempt to lock objects of all types during >> DROP. Maybe the COMMENT code could acquire a conflicting lock. >> > Are you saying AcquireDeletionLock()? > > It seems to me fair enough to prevent the problem, although it is declared > as a static function. Obviously not. We don't need to acquire an AccessExclusiveLock to comment on an object - just something that will CONFLICT WITH an AccessExclusiveLock. So, use the same locking rules, perhaps, but take a much weaker lock, like AccessShareLock. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> writes: > Obviously not. We don't need to acquire an AccessExclusiveLock to > comment on an object - just something that will CONFLICT WITH an > AccessExclusiveLock. So, use the same locking rules, perhaps, but > take a much weaker lock, like AccessShareLock. Well, it probably needs to be a self-conflicting lock type, so that two COMMENTs on the same object can't run concurrently. But I agree AccessExclusiveLock is too strong: that implies locking out read-only examination of the object, which we don't want. regards, tom lane
On Tue, Jul 6, 2010 at 10:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> Obviously not. We don't need to acquire an AccessExclusiveLock to >> comment on an object - just something that will CONFLICT WITH an >> AccessExclusiveLock. So, use the same locking rules, perhaps, but >> take a much weaker lock, like AccessShareLock. > > Well, it probably needs to be a self-conflicting lock type, so that > two COMMENTs on the same object can't run concurrently. But I agree > AccessExclusiveLock is too strong: that implies locking out read-only > examination of the object, which we don't want. Hmm... so, maybe ShareUpdateExclusiveLock? That looks to be the weakest thing that is self-conflicting. The others are ShareRowExclusiveLock, ExclusiveLock, and AccessExclusiveLock. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Excerpts from Robert Haas's message of mar jul 06 22:31:40 -0400 2010: > On Tue, Jul 6, 2010 at 10:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > >> Obviously not. We don't need to acquire an AccessExclusiveLock to > >> comment on an object - just something that will CONFLICT WITH an > >> AccessExclusiveLock. So, use the same locking rules, perhaps, but > >> take a much weaker lock, like AccessShareLock. > > > > Well, it probably needs to be a self-conflicting lock type, so that > > two COMMENTs on the same object can't run concurrently. But I agree > > AccessExclusiveLock is too strong: that implies locking out read-only > > examination of the object, which we don't want. > > Hmm... so, maybe ShareUpdateExclusiveLock? So COMMENT ON will conflict with (auto)vacuum? Seems a bit weird ...
On Tue, Jul 6, 2010 at 10:59 PM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > Excerpts from Robert Haas's message of mar jul 06 22:31:40 -0400 2010: >> On Tue, Jul 6, 2010 at 10:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> > Robert Haas <robertmhaas@gmail.com> writes: >> >> Obviously not. We don't need to acquire an AccessExclusiveLock to >> >> comment on an object - just something that will CONFLICT WITH an >> >> AccessExclusiveLock. So, use the same locking rules, perhaps, but >> >> take a much weaker lock, like AccessShareLock. >> > >> > Well, it probably needs to be a self-conflicting lock type, so that >> > two COMMENTs on the same object can't run concurrently. But I agree >> > AccessExclusiveLock is too strong: that implies locking out read-only >> > examination of the object, which we don't want. >> >> Hmm... so, maybe ShareUpdateExclusiveLock? > > So COMMENT ON will conflict with (auto)vacuum? Seems a bit weird ... Well, I'm open to suggestions... I doubt we want to create a new lock level just for this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company
Robert Haas <robertmhaas@gmail.com> writes: > On Tue, Jul 6, 2010 at 10:59 PM, Alvaro Herrera > <alvherre@commandprompt.com> wrote: >> Excerpts from Robert Haas's message of mar jul 06 22:31:40 -0400 2010: >>> Hmm... so, maybe ShareUpdateExclusiveLock? >> >> So COMMENT ON will conflict with (auto)vacuum? �Seems a bit weird ... > Well, I'm open to suggestions... I doubt we want to create a new lock > level just for this. [ shrug... ] COMMENT ON is DDL, and most forms of DDL will conflict with vacuum. I can't get excited about that complaint. regards, tom lane
(2010/07/07 11:31), Robert Haas wrote: > On Tue, Jul 6, 2010 at 10:18 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: >> Robert Haas<robertmhaas@gmail.com> writes: >>> Obviously not. We don't need to acquire an AccessExclusiveLock to >>> comment on an object - just something that will CONFLICT WITH an >>> AccessExclusiveLock. So, use the same locking rules, perhaps, but >>> take a much weaker lock, like AccessShareLock. >> >> Well, it probably needs to be a self-conflicting lock type, so that >> two COMMENTs on the same object can't run concurrently. But I agree >> AccessExclusiveLock is too strong: that implies locking out read-only >> examination of the object, which we don't want. > > Hmm... so, maybe ShareUpdateExclusiveLock? That looks to be the > weakest thing that is self-conflicting. The others are > ShareRowExclusiveLock, ExclusiveLock, and AccessExclusiveLock. > Is it necessary to confirm existence of the database object being commented on after we got acquired the lock, isn't it? Since the logic of AcquireDeletionLock() requires us to provide argument as object-id form, but we have to translate the object name into object-id outside of the critical section, so the object being commented might be already dropped and committed before we got acquired the lock. Thanks, -- KaiGai Kohei <kaigai@ak.jp.nec.com>
2010/7/9 KaiGai Kohei <kaigai@ak.jp.nec.com>: > (2010/07/07 11:31), Robert Haas wrote: >> On Tue, Jul 6, 2010 at 10:18 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: >>> Robert Haas<robertmhaas@gmail.com> writes: >>>> Obviously not. We don't need to acquire an AccessExclusiveLock to >>>> comment on an object - just something that will CONFLICT WITH an >>>> AccessExclusiveLock. So, use the same locking rules, perhaps, but >>>> take a much weaker lock, like AccessShareLock. >>> >>> Well, it probably needs to be a self-conflicting lock type, so that >>> two COMMENTs on the same object can't run concurrently. But I agree >>> AccessExclusiveLock is too strong: that implies locking out read-only >>> examination of the object, which we don't want. >> >> Hmm... so, maybe ShareUpdateExclusiveLock? That looks to be the >> weakest thing that is self-conflicting. The others are >> ShareRowExclusiveLock, ExclusiveLock, and AccessExclusiveLock. >> > Is it necessary to confirm existence of the database object being > commented on after we got acquired the lock, isn't it? > > Since the logic of AcquireDeletionLock() requires us to provide > argument as object-id form, but we have to translate the object > name into object-id outside of the critical section, so the object > being commented might be already dropped and committed before we > got acquired the lock. Yep. I'm going to work up a patch for this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company