Re: BUG #5952: SetRWConflict assertion failure

"YAMAMOTO Takashi" wrote:

> Description: SetRWConflict assertion failure

> SerializableXactHashLock relocking in CheckTargetForConflictsIn()
> seems racy to me.

You're right. The attached patch should fix the assertion you hit.

I will take a close look at the code above the patched area (for the
optimization to drop the predicate lock when we acquire a write
lock). Looking at it just now I'm wondering if it really is a safe
optimization in PostgreSQL. It was in the Cahill paper, but InnoDB
doesn't have subtransactions. I fear that we could give up the
SIReadLock within a subtransaction and then have problems if the
subtransaction rolls back, effectively discarding the write lock. I
suspect we need to do this only if we're not within a subtransaction.
Will follow up on that after further review.

Thanks!

-Kevin

On Sun, Mar 27, 2011 at 3:18 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> "YAMAMOTO Takashi"  wrote:
>
>> Description: SetRWConflict assertion failure
>
>> SerializableXactHashLock relocking in CheckTargetForConflictsIn()
>> seems racy to me.
>
> You're right.  The attached patch should fix the assertion you hit.

This patch looks reasonable, but I'm a bit concerned about the chunk
immediately preceding the patched area.

When we do this:

LWLockRelease(SerializableXactHashLock);
LWLockRelease(partitionLock);
LWLockRelease(SerializablePredicateLockListLock);
LWLockAcquire(partitionLock, LW_SHARED);
LWLockAcquire(SerializableXactHashLock, LW_SHARED);

Don't we need to also reset nextpredlock to the head of the list? I'm
assuming it's the partitionLock that's keeping the PREDICATELOCKs from
bouncing out from under us, so if we release it, aren't we potentially
point off into thin air?

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

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> This patch looks reasonable, but I'm a bit concerned about the
> chunk immediately preceding the patched area.
>
> When we do this:
>
> LWLockRelease(SerializableXactHashLock);
> LWLockRelease(partitionLock);
> LWLockRelease(SerializablePredicateLockListLock);
> LWLockAcquire(partitionLock, LW_SHARED);
> LWLockAcquire(SerializableXactHashLock, LW_SHARED);
>
> Don't we need to also reset nextpredlock to the head of the list?
> I'm assuming it's the partitionLock that's keeping the
> PREDICATELOCKs from bouncing out from under us, so if we release
> it, aren't we potentially point off into thin air?

I think you are right. That sequence should be followed by a copy
of the same "nextpredlock = " statement that's just above. Do you
want me to revise the patch or do you just want to take care of it
as part of the commit?

Thanks for catching that.

-Kevin

On Tue, Apr 5, 2011 at 11:18 AM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> This patch looks reasonable, but I'm a bit concerned about the
>> chunk immediately preceding the patched area.
>>
>> When we do this:
>>
>>     LWLockRelease(SerializableXactHashLock);
>>     LWLockRelease(partitionLock);
>>     LWLockRelease(SerializablePredicateLockListLock);
>>     LWLockAcquire(partitionLock, LW_SHARED);
>>     LWLockAcquire(SerializableXactHashLock, LW_SHARED);
>>
>> Don't we need to also reset nextpredlock to the head of the list?
>> I'm assuming it's the partitionLock that's keeping the
>> PREDICATELOCKs from bouncing out from under us, so if we release
>> it, aren't we potentially point off into thin air?
>
> I think you are right.  That sequence should be followed by a copy
> of the same "nextpredlock = " statement that's just above.  Do you
> want me to revise the patch or do you just want to take care of it
> as part of the commit?
>
> Thanks for catching that.

If you could send a revised patch, that would be great. I don't want
to muck it up by accident.

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

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> If you could send a revised patch, that would be great.

Attached. I put it in the same spot relative to the lock
acquisition that was used earlier in the function.

-Kevin

I wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> If you could send a revised patch, that would be great.
>
> Attached. I put it in the same spot relative to the lock
> acquisition that was used earlier in the function.

And version 3 which might actually work. [sigh]

-Kevin

On Tue, Apr 5, 2011 at 12:13 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> I wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>
>>> If you could send a revised patch, that would be great.
>>
>> Attached.  I put it in the same spot relative to the lock
>> acquisition that was used earlier in the function.
>
> And version 3 which might actually work.  [sigh]

Committed.

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