On Mon, Nov 9, 2015 at 8:07 AM, Steve Singer <steve@ssinger.info> wrote:
> On 10/15/2015 05:47 PM, Kevin Grittner wrote:
>>
>> All other issues raised by Álvaro and Steve have been addressed, except
>> for this one, which I will argue against:
>
>
> I've been looking through the updated patch
>
>
> In snapmgr.c
>
>
> + * XXX: If we can trust a read of an int64 value to be atomic, we can skip
> the
> + * spinlock here.
> + */
> +int64
> +GetOldSnapshotThresholdTimestamp(void)
>
>
> Was your intent with the XXX for it to be a TODO to only aquire the lock on
> platforms without the atomic 64bit operations?
>
> On a more general note:
>
> I've tried various manual tests of this feature and it sometimes works as
> expected and sometimes doesn't.
> I'm getting the feeling that how I expect it to work isn't quite in sync
> with how it does work.
>
> I'd expect the following to be sufficient to generate the test
>
> T1: Obtains a snapshot that can see some rows
> T2: Waits 60 seconds and performs an update on those rows
> T2: Performs a vacuum
> T1: Waits 60 seconds, tries to select from the table. The snapshot should
> be too old
>
>
> For example it seems that in test 002 the select_ok on conn1 following the
> vacuum but right before the final sleep is critical to the snapshot too old
> error showing up (ie if I remove that select_ok but leave in the sleep I
> don't get the error)
>
> Is this intended and if so is there a better way we can explain how things
> work?
>
>
> Also is 0 intended to be an acceptable value for old_snapshot_threshold and
> if so what should we expect to see then?
There has been a review but no replies for more than 1 month. Returned
with feedback?
--
Michael