Interesting, but it's still not entirely clear to me from reading the comments why we should think that this is safe.
In overall, I think this is safe, because we do not change algorithm here. More specific, threshold_timestamp have only used in a few cases:
1). To get the latest value by calling GetOldSnapshotThresholdTimestamp. This will work, since we only change the sync type here from the spinlock to an atomic.
2). In TransactionIdLimitedForOldSnapshots, but here no changes in the behaviour will be done. Sync model will be the save as before the patch.
3). In SnapshotTooOldMagicForTest, which is a stub to make old_snapshot_threshold tests appear "working". But no coherence with the threshold_xid here.
So, we have a two use cases for the threshold_timestamp:
a). When the threshold_timestamp is used in conjunction with the threshold_xid. We must use spinlock to sync.
b). When the threshold_timestamp is used without conjunction with the threshold_xid. In this case, we use atomic values.
