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

On 2014-08-26 22:04:03 -0400, Robert Haas wrote:
> 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.

Ok. I actually wonder about chucking out the whole explanation in
buf_init.c. There's been something there historically, but it's not
really a better place than just keeping everything in bufmgr.c.

> +#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?

Ok, will make it /* one cache line in common architectures */ - I want
the reasoning for the current size somewhere...

> + * 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;

The current if () ... isn't particularly nice, I agree.

> 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.

I agree. As long as people are happy with the approach I think we can
iron out performance edge cases later.

I'll try to send a cleaned up version soon. I'm currently wondering
about adding some minimal regression test coverage for this. What I have
right now is stuff like
DECLARE c_01 CURSOR FOR SELECT * FROM pg_attribute WHERE ctid = '(0, 1)';
DECLARE c_02 CURSOR FOR SELECT * FROM pg_attribute WHERE ctid = '(1, 1)';
...
FETCH NEXT FROM c_01;
FETCH NEXT FROM c_02;
...
CLOSE c_01;
...

While that provides some coverage, I'm unconvinced that it's appropriate
for the regression tests?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services