Обсуждение: releaseOk and LWLockWaitForVar
Hi Heikki, All, Amit just pointed me to a case where the lwlock scalability patch apparently causes problems and I went on to review it and came across the following problem in 9.4/master: LWLockWaitForVar() doesn't set releaseOk to true when waiting again. Isn't that a bug? What if there's another locker coming in after LWLockWaitForVar() returns from the PGSemaphoreLock() but before it has acquire the spinlock? Now, it might be that it's unproblematic because of hte specific way these locks are used right now, but it doesn't seem like a good idea to leave it that way. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Jun 17, 2014 at 5:47 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>
> Hi Heikki, All,
>
> Amit just pointed me to a case where the lwlock scalability patch
> apparently causes problems and I went on to review it and came across
> the following problem in 9.4/master:
> LWLockWaitForVar() doesn't set releaseOk to true when waiting
> again. Isn't that a bug? What if there's another locker coming in after
> LWLockWaitForVar() returns from the PGSemaphoreLock() but before it has
> acquire the spinlock?
>
> Hi Heikki, All,
>
> Amit just pointed me to a case where the lwlock scalability patch
> apparently causes problems and I went on to review it and came across
> the following problem in 9.4/master:
> LWLockWaitForVar() doesn't set releaseOk to true when waiting
> again. Isn't that a bug? What if there's another locker coming in after
> LWLockWaitForVar() returns from the PGSemaphoreLock() but before it has
> acquire the spinlock?
I also think above mentioned scenario is a problem if releaseOk
is not set to true in above case.
While looking at function LWLockWaitForVar(), espacially below
code:
TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(l), T_ID(l), LW_EXCLUSIVE);
I think in this function tracing is done considering the Exclusive lock
is acquired, however it might have granted access because of
variable updation. Basically this function's trace doesn't distinguish
whether the access is granted due to the reason that there is no other
exclusive locker or variable is updated.
On 06/17/2014 03:17 PM, Andres Freund wrote: > LWLockWaitForVar() doesn't set releaseOk to true when waiting > again. Isn't that a bug? LWLockWaitForVar() waits in LW_WAIT_UNTIL_FREE mode, because it's not interested in acquiring the lock, it just wants to be woken up when it's released (or the "var" is updated). LWLockRelease doesn't clear releaseOK when it wakes up a LW_WAIT_UNTIL_FREE-mode waiter. > What if there's another locker coming in after > LWLockWaitForVar() returns from the PGSemaphoreLock() but before it has > acquire the spinlock? Now, it might be that it's unproblematic because > of hte specific way these locks are used right now, but it doesn't seem > like a good idea to leave it that way. In that scenario, LWLockWaitForVar() will grab the spinlock, after the other process. What happens next depends on the whether the value of the variable it guards was changed. If it was, LWLockWaitForVar() will see that it changed, and return false without waiting again. If the value didn't change, it will sleep until the new locker releases the lock. In either case, I don't see a problem with releaseOK. It seems correct as it is. - Heikki
On 06/23/2014 05:38 PM, Amit Kapila wrote: > While looking at function LWLockWaitForVar(), espacially below > code: > > TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(l), T_ID(l), LW_EXCLUSIVE); > > I think in this function tracing is done considering the Exclusive lock > is acquired, however it might have granted access because of > variable updation. Basically this function's trace doesn't distinguish > whether the access is granted due to the reason that there is no other > exclusive locker or variable is updated. Yeah. Not sure it's worth it to add new TRACE points for this, I'm not really familiar with the way the traces work or how people use them. - Heikki
On Mon, Jun 23, 2014 at 10:10 PM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
> On 06/23/2014 05:38 PM, Amit Kapila wrote:
>>
>> While looking at function LWLockWaitForVar(), espacially below
>> code:
>>
>> TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(l), T_ID(l), LW_EXCLUSIVE);
>>
>> I think in this function tracing is done considering the Exclusive lock
>> is acquired, however it might have granted access because of
>> variable updation. Basically this function's trace doesn't distinguish
>> whether the access is granted due to the reason that there is no other
>> exclusive locker or variable is updated.
>
>
> Yeah. Not sure it's worth it to add new TRACE points for this, I'm not really familiar with the way the traces work or how people use them.
Even if we don't want to add new trace points for new usage, I
> On 06/23/2014 05:38 PM, Amit Kapila wrote:
>>
>> While looking at function LWLockWaitForVar(), espacially below
>> code:
>>
>> TRACE_POSTGRESQL_LWLOCK_ACQUIRE(T_NAME(l), T_ID(l), LW_EXCLUSIVE);
>>
>> I think in this function tracing is done considering the Exclusive lock
>> is acquired, however it might have granted access because of
>> variable updation. Basically this function's trace doesn't distinguish
>> whether the access is granted due to the reason that there is no other
>> exclusive locker or variable is updated.
>
>
> Yeah. Not sure it's worth it to add new TRACE points for this, I'm not really familiar with the way the traces work or how people use them.
Even if we don't want to add new trace points for new usage, I
think using existing might give wrong information to people who
want to use Dynamic tracing.
Another thing I have noticed is that docs are not updated for trace
macro's
TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT
TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT_FAIL
TRACE_POSTGRESQL_LWLOCK_ACQUIRE_OR_WAIT_FAIL
Information related to existing tracing macro's is present at below link: