Thread: [HACKERS] PinBuffer() no longer makes use of strategy

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

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")?

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?

Greetings,

Andres Freund

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)

Hi,

On 2017-02-03 19:13:45 -0600, Jim Nasby wrote:
> 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

Well, as long as the buffer is used from the ring, BufferAlloc() -
BufferAlloc() will reset the usagecount when rechristening the
buffer. So unless anything happens inbetween the buffer being remapped
last and remapped next, it'll be reused. Right?

The only case where I can see the old logic mattering positively is for
synchronized seqscans.  For pretty much else that logic seems worse,
because it essentially prevents any buffers ever staying in s_b when
only ringbuffer accesses are performed.

I'm tempted to put the old logic back, but more because this likely was
unintentional, not because I think it's clearly better.


> Also, shouldn't there be warnings or something from having a function
> argument that's never used?

No, that's actually fairly common in our codebase.


- Andres

On 2/4/17 1:47 PM, Alexander Korotkov wrote:
>     I'm tempted to put the old logic back, but more because this likely was
>     unintentional, not because I think it's clearly better.
>
>
> +1
> Yes, it was unintentional change.  So we should put old logic back
> unless we have proof that this change make it better.
> Patch is attached.  I tried to make some comments, but probably they are
> not enough.

Added to CF: https://commitfest.postgresql.org/13/1029/
-- 
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)

On 2/4/17 2:47 PM, Alexander Korotkov wrote:
> On Sat, Feb 4, 2017 at 4:33 AM, Andres Freund <andres@anarazel.de
> <mailto:andres@anarazel.de>> wrote:
> 
>     On 2017-02-03 19:13:45 -0600, Jim Nasby wrote:
>     > 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
> 
>     Well, as long as the buffer is used from the ring, BufferAlloc() -
>     BufferAlloc() will reset the usagecount when rechristening the
>     buffer. So unless anything happens inbetween the buffer being remapped
>     last and remapped next, it'll be reused. Right?
> 
>     The only case where I can see the old logic mattering positively is for
>     synchronized seqscans.  For pretty much else that logic seems worse,
>     because it essentially prevents any buffers ever staying in s_b when
>     only ringbuffer accesses are performed.
> 
>     I'm tempted to put the old logic back, but more because this likely was
>     unintentional, not because I think it's clearly better.
> 
> 
> +1
> Yes, it was unintentional change.  So we should put old logic back
> unless we have proof that this change make it better.
> Patch is attached.  I tried to make some comments, but probably they are
> not enough.

This patch looks pretty straight forward and applies cleanly and
compiles at cccbdde.

It's not a straight revert, though, so still seems to need review.

Jim, do you know when you'll have a chance to look at that?

-- 
-David
david@pgmasters.net

On 3/16/17 12:48 PM, David Steele wrote:
> This patch looks pretty straight forward and applies cleanly and
> compiles at cccbdde.
>
> It's not a straight revert, though, so still seems to need review.
>
> Jim, do you know when you'll have a chance to look at that?

Yes. Compiles and passes for me as well.

One minor point: previously the code did
  if (buf->usage_count < BM_MAX_USAGE_COUNT)

but now it does
  if (BUF_STATE_GET_USAGECOUNT(buf_state) != BM_MAX_USAGE_COUNT)

being prone to paranoia, I prefer the first, but I've seen both styles 
in the code so I don't know if it's worth futzing with.

Marked as RFC.
-- 
Jim Nasby, Chief Data Architect, Austin TX
OpenSCG                 http://OpenSCG.com

On 4 February 2017 at 09:33, Andres Freund <andres@anarazel.de> wrote:

> For pretty much else that logic seems worse,
> because it essentially prevents any buffers ever staying in s_b when
> only ringbuffer accesses are performed.

That was exactly its intent. The ringbuffer was designed to avoid
cache spoiling by large scans.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

>       if (buf->usage_count < BM_MAX_USAGE_COUNT)
>       if (BUF_STATE_GET_USAGECOUNT(buf_state) != BM_MAX_USAGE_COUNT)
>
>     being prone to paranoia, I prefer the first, but I've seen both styles in
>     the code so I don't know if it's worth futzing with.
>
>
> Ok, let's be paranoic and do this same way as before.  Revised patch is attached.

I see the change was done in 9.6 release cycle in commit 
48354581a49c30f5757c203415aa8412d85b0f70 at April, 10. Does it mean the fix 
should be backpatched too?

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
  WWW: http://www.sigaev.ru/