Re: findDependentObjects() mutual exclusion vs. MVCC catalog scans

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