Re: HS locking broken in HEAD

From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Andres Freund <andres(at)2ndquadrant(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: HS locking broken in HEAD
Date: 2013-01-18 15:15:20
Message-ID: CA+Tgmob_SkqkQW1PxjgaGBSh7ha5-ww1wPV+0tPbn-aRAGs70Q@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, Jan 17, 2013 at 9:00 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > I think it might also be a dangerous assumption for shared objects?
>>
>> Locks on shared objects can't be taken via the fast path. In order to
>> take a fast-path lock, a backend must be bound to a database and the
>> locktag must be for a relation in that database.
>
> I assumed it wasn't allowed, but didn't find where that happens at first
> - I found it now though (c.f. SetLocktagRelationOid).
>
>> > A patch minimally addressing the is attached, but it only addresses part
>> > of the problem.
>>
>> Isn't the right fix to compare proc->databaseId to
>> locktag->locktag_field1 rather than to MyDatabaseId? The subsequent
>> loop assumes that if the relid matches, the lock tag is an exact
>> match, which will be correct with that rule but not the one you
>> propose.
>
> I don't know much about the locking code, you're probably right. I also
> didn't look very far after finding the guilty commit.
>
> (reading code)
>
> Yes, I think you're right, that seems to be the actually correct fix.
>
> I am a bit worried that there are more such assumptions in the code, but
> I didn't find any, but I really don't know that code.

I found two instances of this. See attached patch.

Can you test whether this fixes it for you?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachment Content-Type Size
fix-mydatabaseid-compare.patch application/octet-stream 1.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2013-01-18 15:23:07 Re: Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend
Previous Message Andres Freund 2013-01-18 15:09:18 Re: Re: [PATCH 1/5] Centralize Assert* macros into c.h so its common between backend/frontend