Re: 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(at)ak(dot)jp(dot)nec(dot)com>

2010/7/6 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)gmail(dot)com> writes:
> 2010/7/6 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)gmail(dot)com> writes:
>> 2010/7/6 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)com>

2010/7/6 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> (2010/07/06 23:33), Tom Lane wrote:
>> Robert Haas<robertmhaas(at)gmail(dot)com>  writes:
>>> 2010/7/6 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)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(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)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(at)commandprompt(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> > Robert Haas <robertmhaas(at)gmail(dot)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(at)gmail(dot)com> writes:
> On Tue, Jul 6, 2010 at 10:59 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas<robertmhaas(at)gmail(dot)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(at)ak(dot)jp(dot)nec(dot)com>

2010/7/9 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> (2010/07/07 11:31), Robert Haas wrote:
>> On Tue, Jul 6, 2010 at 10:18 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us>  wrote:
>>> Robert Haas<robertmhaas(at)gmail(dot)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