Re: [HACKERS] logical decoding of two-phase transactions

On 4/5/18 8:50 AM, Nikhil Sontakke wrote:
> Hi Tomas,
> 
>> 1) There's a race condition in LogicalLockTransaction. The code does
>> roughly this:
>>
>>   if (!BecomeDecodeGroupMember(...))
>>      ... bail out ...
>>
>>   Assert(MyProc->decodeGroupLeader);
>>   lwlock = LockHashPartitionLockByProc(MyProc->decodeGroupLeader);
>>   ...
>>
>> but AFAICS there is no guarantee that the transaction does not commit
>> (or even abort) right after the become decode group member. In which
>> case LogicalDecodeRemoveTransaction might have already reset our pointer
>> to a leader to NULL. In which case the Assert() and lock will fail.
>>
>> I've initially thought this can be fixed by setting decodeLocked=true in
>> BecomeDecodeGroupMember, but that's not really true - that would fix the
>> race for aborts, but not commits. LogicalDecodeRemoveTransaction skips
>> the wait for commits entirely, and just resets the flags anyway.
>>
>> So this needs a different fix, I think. BecomeDecodeGroupMember also
>> needs the leader PGPROC pointer, but it does not have the issue because
>> it gets it as a parameter. I think the same thing would work for here
>> too - that is, use the AssignDecodeGroupLeader() result instead.
>>
> 
> That's a good catch. One of the earlier patches had a check for this
> (it also had an ill-placed assert above though) which we removed as
> part of the ongoing review.
> 
> Instead of doing the above, we can just re-check if the
> decodeGroupLeader pointer has become NULL and if so, re-assert that
> the leader has indeed gone away before returning false. I propose a
> diff like below.
> 
>         /*
> 
>          * If we were able to add ourself, then Abort processing will
> 
> -        * interlock with us.
> 
> +        * interlock with us. If the leader was done in the meanwhile
> 
> +        * it could have removed us and gone away as well.
> 
>          */
> 
> -       Assert(MyProc->decodeGroupLeader);
> 
> +       if (MyProc->decodeGroupLeader == NULL)
> 
> +       {
> 
> +               Assert(BackendXidGetProc(txn->xid) == NULL);
> 
> +               return false
> 
> +       }
> 
> 

Uh? Simply rechecking if MyProc->decodeGroupLeader is NULL obviously
does not fix the race condition - it might get NULL right after the
check. So we need to either lookup the PROC again (and then get the
associated lwlock), or hold some other type of lock.


>>
>> 3) I don't quite understand why BecomeDecodeGroupMember does the
>> cross-check using PID. In which case would it help?
>>
> 
> When I wrote this support, I had written it with the intention of
> supporting both 2PC (in which case pid is 0) and in-progress regular
> transactions. That's why the presence of PID in these functions. The
> current use case is just for 2PC, so we could remove it.
> 

Sure, but why do we need to cross-check the PID at all? I may be missing
something here, but I don't see what does this protect against?

> 
>>
>> 5) ReorderBufferCommitInternal does elog(LOG) about interrupting the
>> decoding of aborted transaction only in one place. There are about three
>> other places where we check LogicalLockTransaction. Seems inconsistent.
>>
> 
> Note that I have added it for the OPTIONAL test_decoding test cases
> (which AFAIK we don't plan to commit in that state) which demonstrate
> concurrent rollback interlocking with the lock/unlock APIs. The first
> ELOG was enough to catch the interaction. If we think these elogs
> should be present in the code, then yes, I can add it elsewhere as
> well as part of an earlier patch.
> 

Ah, I see. Makes sense. I've been looking at the patch as a whole and
haven't realized it's part of this piece.

> 
> Ok, I am looking at your provided patch and incorporating relevant
> changes from it. WIll submit a patch set soon.
> 

OK.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services