On Tue, Aug 26, 2014 at 7:52 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> Here's the next version of this patch.
+ * much never requried. So we keep a small array of reference counts
Typo. But I think you could just drop the whole sentence about how
things used to be, especially since it's recapitulated elsewhere.
+#define REFCOUNT_ARRAY_ENTRIES 8 /* one full cacheline */
Obviously that's not always going to be the case. You could say
"about", or just drop the comment. Shouldn't "cache line" be two
words?
+ * refcounts are kept track of in the array, after that new array entries
s/, after that/; after that,/
+ if (!found && !create)
+ else if (!found && free != NULL)
+ else if (!found)
+ else if (found && !do_move)
+ else if (found && free != NULL)
+ else if (found)
+ Assert(false); /* unreachable */
+ return res;
There's not much point in testing found when you've already handled
the not-found cases. But I'd reorganize this whole thing like this:
if (!found) { if (!create) { return; } if (free != NULL) { stuff;
return }; stuff; return; }
if (!do_move) { return; } if (free != NULL) { stuff; return; } stuff; return;
+ * Stop tracking the refcount of the buffer ref is tracking the refcount
+ * for. Nono, there's no circularity here.
Incomprehensible babble. Perhaps: "Release resources used to track
the reference count of a buffer which we no longer have pinned."
That's all I see on a first-read through. There might be other
issues, and I haven't checked through it in great detail for mundane
bugs, but generally, I favor pressing on relatively rapidly toward a
commit. It seems highly likely that this idea is a big win, and if
there's some situation in which it's a loss, we're more likely to find
out with it in the tree (and thus likely to be tested by many more
people) than by analysis from first principles.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
