Re: HS locking broken in HEAD

On 2013-01-17 20:36:43 -0500, Robert Haas wrote:
> On Thu, Jan 17, 2013 at 6:22 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > That only made the bug more visible - the real bug is somewhere
> > else. Which makes it even scarrier, the bug was in in the fast path
> > locking patch from the start...
> >
> > It assumes conflicting fast path locks can only ever be in the same
> > database as the the backend transfering the locks to itself. But thats
> > obviously not true for the startup process which is in no specific
> > database.
>
> Ugh.
>
> > 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.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services