Re: HS locking broken in HEAD

On 2013-01-18 10:15:20 -0500, Robert Haas wrote:
> 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.

Yea, those are the two sites I had "fixed" (incorrectly) as well. I
looked a bit more yesterday night and I didn't find any additional
locations, so I hope we got this.

Yep, seems to work.

I am still stupefied nobody noticed that locking in HS (where just about
all locks are going to be fast path locks) was completely broken for
nearly two years.

Greetings,

Andres Freund

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