Re: [RFC, POC] Don't require a NBuffer sized PrivateRefCount array of local buffer pins

On Tue, Aug 26, 2014 at 7:52 PM, Andres Freund <andres(at)2ndquadrant(dot)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