On 9 June 2018 at 15:41, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Jun 8, 2018 at 5:27 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> On 8 June 2018 at 11:33, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>> On Fri, Jun 8, 2018 at 1:53 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>>>
>>>> So the attached patch fixes both the bug in the recent commit and the
>>>> one I just found by observation of 49bff5300d527, since they are
>>>> related.
>>>>
>>>> StandbyReleaseOldLocks() can sweep in the same way as
>>>> ExpireOldKnownAssignedTransactionIds().
>>>>
>>>
>>
>>> How will this avoid the bug introduced by your recent commit
>>> (32ac7a11)? After that commit, we no longer consider vacuum's xid in
>>> RunningTransactions->oldestRunningXid calculation, so it is possible
>>> that we still remove/release its lock on standby earlier than
>>> required.
>>
>> In the past, the presence of an xid was required to prevent removal by
>> StandbyReleaseOldLocks().
>>
>> The new patch removes that requirement and removes when the xid is
>> older than oldestxid
>>
>
> The case I have in mind is: "Say vacuum got xid 600 while truncating,
> and then some other random transactions 601,602 starts modifying the
> database. At this point, I think the computed value of
> RunningTransactions->oldestRunningXid will be 601. Now, on standby
> when StandbyReleaseOldLocks sees the lock from transaction 600, it
> will remove it which doesn't appear correct to me.".
OK, thanks. Patch attached.
>> The normal path of removal is at commit or abort,
>> StandbyReleaseOldLocks is used almost never (as originally intended).
>>
> Okay, but that doesn't prevent us to ensure that whenever used, it
> does the right thing.
What do you mean? Has anyone argued in favour of doing the wrong thing?
I'm playing around with finding a test case to prove this area works,
rather than just manual testing. Suggestions welcome.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services