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.".
> 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.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com