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 Sat, Feb 4, 2017 at 4:33 AM, Andres Freund <andres(at)anarazel(dot)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.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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(at)anarazel(dot)de
> <mailto:andres(at)anarazel(dot)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(at)pgmasters(dot)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(at)anarazel(dot)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

On Sun, Mar 19, 2017 at 3:51 AM, Jim Nasby <jim(dot)nasby(at)openscg(dot)com> wrote:

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

Ok, let's be paranoic and do this same way as before. Revised patch is
attached.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

> 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(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/

On Mon, Mar 20, 2017 at 6:09 PM, Teodor Sigaev <teodor(at)sigaev(dot)ru> wrote:

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

I think so. This patch reverts unintentional change and can be considered
as bug fix.
BTW, sorry for unicode filename in previous letter.
Patch with normal ASCII name is attached.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company