From: | Noah Misch <noah(at)leadboat(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: findDependentObjects() mutual exclusion vs. MVCC catalog scans |
Date: | 2013-07-17 00:30:06 |
Message-ID: | 20130717003006.GC55849@tornado.leadboat.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Noah Misch | 2013-07-17 00:31:15 | Re: Re: FILTER for aggregates [was Re: Department of Redundancy Department: makeNode(FuncCall) division] |
Previous Message | Noah Misch | 2013-07-16 23:19:49 | Re: SSL renegotiation |