On Tue, Jul 16, 2013 at 11:27:02AM -0400, Robert Haas wrote:
> I recommend reworking the header comment to avoid mention of
> SnapshotNow, since if we get rid of SnapshotNow, the reference might
> not be too clear to far-future hackers.
>
> + /*
> + * For a scan using a non-MVCC snapshot like SnapshotSelf, we would simply
> + * reuse the old snapshot. So far, the only caller uses MVCC snapshots.
> + */
> + freshsnap = GetCatalogSnapshot(RelationGetRelid(sysscan->heap_rel));
>
> This comment is not very clear, because it doesn't describe what the
> code actually does, but rather speculates about what the code could do
> if the intention of some future caller were different. I recommend
> adding Assert(IsMVCCSnapshot(scan->xs_snapshot)) and changing the
> comment to something like this: "For now, we don't handle the case of
> a non-MVCC scan snapshot. This is adequate for existing uses of this
> function, but might need to be changed in the future."
On Tue, Jul 16, 2013 at 11:35:48AM -0400, Tom Lane wrote:
> I agree with Robert's comments, and in addition suggest that this code
> needs a comment about why it's safe to use the snapshot without doing
> RegisterSnapshot or equivalent.
Committed with hopefully-better comments. Thanks.
--
Noah Misch
EnterpriseDB http://www.enterprisedb.com