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)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Feb 4, 2017 at 4:33 AM, Andres Freund <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 thebuffer
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
Attachments:
put-buffer-usagecount–logic–back.patchapplication/octet-stream; name="=?UTF-8?B?cHV0LWJ1ZmZlci11c2FnZWNvdW504oCTbG9naWPigJNiYWNrLnBhdGNo?="Download
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
new file mode 100644
index 3cb5120..58c9216
*** a/src/backend/storage/buffer/bufmgr.c
--- b/src/backend/storage/buffer/bufmgr.c
*************** PinBuffer(BufferDesc *buf, BufferAccessS
*** 1590,1598 ****
/* increase refcount */
buf_state += BUF_REFCOUNT_ONE;
! /* increase usagecount unless already max */
! if (BUF_STATE_GET_USAGECOUNT(buf_state) != BM_MAX_USAGE_COUNT)
! buf_state += BUF_USAGECOUNT_ONE;
if (pg_atomic_compare_exchange_u32(&buf->state, &old_buf_state,
buf_state))
--- 1590,1610 ----
/* increase refcount */
buf_state += BUF_REFCOUNT_ONE;
! if (strategy == NULL)
! {
! /* Default case: increase usagecount unless already max. */
! if (BUF_STATE_GET_USAGECOUNT(buf_state) != BM_MAX_USAGE_COUNT)
! buf_state += BUF_USAGECOUNT_ONE;
! }
! else
! {
! /*
! * Ring buffers shouldn't evict others from pool. Thus we
! * don't make usagecount more than 1.
! */
! if (BUF_STATE_GET_USAGECOUNT(buf_state) == 0)
! buf_state += BUF_USAGECOUNT_ONE;
! }
if (pg_atomic_compare_exchange_u32(&buf->state, &old_buf_state,
buf_state))
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)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 bufferWell, 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Mar 19, 2017 at 3:51 AM, Jim Nasby <jim.nasby@openscg.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
Attachments:
put-buffer-usagecount–logic–back-2.patchapplication/octet-stream; name="=?UTF-8?B?cHV0LWJ1ZmZlci11c2FnZWNvdW504oCTbG9naWPigJNiYWNrLTIucGF0Y2g=?="Download
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
new file mode 100644
index 2ca2ab9..2109cbf
*** a/src/backend/storage/buffer/bufmgr.c
--- b/src/backend/storage/buffer/bufmgr.c
*************** PinBuffer(BufferDesc *buf, BufferAccessS
*** 1595,1603 ****
/* increase refcount */
buf_state += BUF_REFCOUNT_ONE;
! /* increase usagecount unless already max */
! if (BUF_STATE_GET_USAGECOUNT(buf_state) != BM_MAX_USAGE_COUNT)
! buf_state += BUF_USAGECOUNT_ONE;
if (pg_atomic_compare_exchange_u32(&buf->state, &old_buf_state,
buf_state))
--- 1595,1615 ----
/* increase refcount */
buf_state += BUF_REFCOUNT_ONE;
! if (strategy == NULL)
! {
! /* Default case: increase usagecount unless already max. */
! if (BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)
! buf_state += BUF_USAGECOUNT_ONE;
! }
! else
! {
! /*
! * Ring buffers shouldn't evict others from pool. Thus we
! * don't make usagecount more than 1.
! */
! if (BUF_STATE_GET_USAGECOUNT(buf_state) == 0)
! buf_state += BUF_USAGECOUNT_ONE;
! }
if (pg_atomic_compare_exchange_u32(&buf->state, &old_buf_state,
buf_state))
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/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Mar 20, 2017 at 6:09 PM, Teodor Sigaev <teodor@sigaev.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
Attachments:
put-buffer-usagecount-logic-back-2.patchapplication/octet-stream; name=put-buffer-usagecount-logic-back-2.patchDownload
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
new file mode 100644
index 2ca2ab9..2109cbf
*** a/src/backend/storage/buffer/bufmgr.c
--- b/src/backend/storage/buffer/bufmgr.c
*************** PinBuffer(BufferDesc *buf, BufferAccessS
*** 1595,1603 ****
/* increase refcount */
buf_state += BUF_REFCOUNT_ONE;
! /* increase usagecount unless already max */
! if (BUF_STATE_GET_USAGECOUNT(buf_state) != BM_MAX_USAGE_COUNT)
! buf_state += BUF_USAGECOUNT_ONE;
if (pg_atomic_compare_exchange_u32(&buf->state, &old_buf_state,
buf_state))
--- 1595,1615 ----
/* increase refcount */
buf_state += BUF_REFCOUNT_ONE;
! if (strategy == NULL)
! {
! /* Default case: increase usagecount unless already max. */
! if (BUF_STATE_GET_USAGECOUNT(buf_state) < BM_MAX_USAGE_COUNT)
! buf_state += BUF_USAGECOUNT_ONE;
! }
! else
! {
! /*
! * Ring buffers shouldn't evict others from pool. Thus we
! * don't make usagecount more than 1.
! */
! if (BUF_STATE_GET_USAGECOUNT(buf_state) == 0)
! buf_state += BUF_USAGECOUNT_ONE;
! }
if (pg_atomic_compare_exchange_u32(&buf->state, &old_buf_state,
buf_state))