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 -- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
