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