On 2/3/17 6:39 PM, Andres Freund wrote:
> Hi,
>
> On 2017-02-03 18:32:03 -0600, Jim Nasby wrote:
>> Commit 48354581a49c30f5757c203415aa8412d85b0f70 (Allow Pin/UnpinBuffer to
>> operate in a lockfree manner) removed the code in PinBuffer that
>> conditionally incremented usage_count when a ring buffer was in use. Was
>> that intentional? ISTM the old behavior should have been retained.
>
> Hm. You mean the else in
> if (strategy == NULL)
> {
> if (buf->usage_count < BM_MAX_USAGE_COUNT)
> buf->usage_count++;
> }
> else
> {
> if (buf->usage_count == 0)
> buf->usage_count = 1;
> }
> (Not sure what you exactly mean with "conditionally increment")?
Exactly that.
> I don't really recall - I suspect it wasn't (otherwise we'd have had to
> update the function's comment and remove the arguument). Alexander? I
> suspect I'd skipped implementing it in my prototype and when finishing
> the patch Alexander didn't see that part.
>
> I have a hard time believing it makes any sort of meaningful difference
> though - you see one?
No, I noticed it while reading code. Removing that does mean that if any
non-default strategy (in any backend) hits that buffer again then the
buffer will almost certainly migrate into the main buffer pool the next
time one of the rings hits that buffer, whereas previously the only way
that could happen is if someone accessed the buffer outside of a ring
and the clock hadn't visited the buffer yet. In other words, this is
about as fuzzy as a two week old grapefruit.
Obviously the code and comments should be made to match though.
Also, shouldn't there be warnings or something from having a function
argument that's never used?
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)