Re: Scaling shared buffer eviction

As mentioned previously about my interest in improving shared
buffer eviction especially by reducing contention around
BufFreelistLock, I would like to share my progress about the
same.

The test used for this work is mainly the case when all the
data doesn't fit in shared buffers, but does fit in memory.
It is mainly based on previous comparison done by Robert
for similar workload:
http://rhaas.blogspot.in/2012/03/performance-and-scalability-on-ibm.html

To start with, I have taken LWLOCK_STATS report to confirm
the contention around BufFreelistLock and the data for HEAD
is as follows:

M/c details
IBM POWER-7 16 cores, 64 hardware threads
RAM - 64GB
Test
scale factor = 3000
shared_buffers = 8GB
number_of_threads = 64
duration = 5mins
./pgbench -c 64 -j 64 -T 300 -S postgres

LWLOCK_STATS data for BufFreeListLock
PID 11762 lwlock main 0: shacq 0 exacq 253988 blk 29023

Here the high *blk* count for scale factor 3000 clearly shows
that to find a usable buffer when data doesn't fit in shared buffers
it has to wait.

To solve this issue, I have implemented a patch which makes
sure that there are always enough buffers on freelist such that
the need for backend to run clock-sweep is minimal, the
implementation idea is more or less same as discussed
previously in below thread, so I will explain it at end of mail.
http://www.postgresql.org/message-id/006e01ce926c$c7768680$56639380$@kapila@huawei.com

LWLOCK_STATS data after Patch (test used is same as
used for HEAD):

BufFreeListLock
PID 7257 lwlock main 0: shacq 0 exacq 165 blk 18 spindelay 0

Here the low *exacq* and *blk* count shows that the need to
run clock sweep for backend has reduced significantly.

Performance Data
-------------------------------
shared_buffers= 8GB
number of threads - 64
sc - scale factor

sc tps
Head 3000 45569
Patch 3000 46457
Head 1000 93037
Patch 1000 92711

Above data shows that there is no significant change in
performance or scalability even after the contention is
reduced significantly around BufFreelistLock.

I have analyzed the patch both with perf record and
LWLOCK_STATS, both indicates that there is a high
contention around BufMappingLocks.

Data With perf record -a -g
-----------------------------------------

+ 10.14% swapper [kernel.kallsyms] [k]
.pseries_dedicated_idle_sleep
+ 7.77% postgres [kernel.kallsyms] [k] ._raw_spin_lock
+ 6.88% postgres [kernel.kallsyms] [k]
.function_trace_call
+ 4.15% pgbench [kernel.kallsyms] [k] .try_to_wake_up
+ 3.20% swapper [kernel.kallsyms] [k]
.function_trace_call
+ 2.99% pgbench [kernel.kallsyms] [k]
.function_trace_call
+ 2.41% postgres postgres [.] AllocSetAlloc
+ 2.38% postgres [kernel.kallsyms] [k] .try_to_wake_up
+ 2.27% pgbench [kernel.kallsyms] [k] ._raw_spin_lock
+ 1.49% postgres [kernel.kallsyms] [k]
._raw_spin_lock_irq
+ 1.36% postgres postgres [.]
AllocSetFreeIndex
+ 1.09% swapper [kernel.kallsyms] [k] ._raw_spin_lock
+ 0.91% postgres postgres [.] GetSnapshotData
+ 0.90% postgres postgres [.]
MemoryContextAllocZeroAligned

Expanded graph
------------------------------

- 10.14% swapper [kernel.kallsyms] [k]
.pseries_dedicated_idle_sleep
- .pseries_dedicated_idle_sleep
- 10.13% .pseries_dedicated_idle_sleep
- 10.13% .cpu_idle
- 10.00% .start_secondary
.start_secondary_prolog
- 7.77% postgres [kernel.kallsyms] [k] ._raw_spin_lock
- ._raw_spin_lock
- 6.63% ._raw_spin_lock
- 5.95% .double_rq_lock
- .load_balance
- 5.95% .__schedule
- .schedule
- 3.27% .SyS_semtimedop
.SyS_ipc
syscall_exit
semop
PGSemaphoreLock
LWLockAcquireCommon
- LWLockAcquire
- 3.27% BufferAlloc
ReadBuffer_common
- ReadBufferExtended
- 3.27% ReadBuffer
- 2.73% ReleaseAndReadBuffer
- 1.70% _bt_relandgetbuf
_bt_search
_bt_first
btgettuple

It shows BufferAlloc->LWLOCK as top contributor and we use
BufMappingLocks in BufferAlloc, I have checked other expanded
calls as well, StrategyGetBuffer is not present in top contributors.

Data with LWLOCK_STATS
----------------------------------------------
BufMappingLocks

PID 7245 lwlock main 38: shacq 41117 exacq 34561 blk 36274 spindelay 101
PID 7310 lwlock main 39: shacq 40257 exacq 34219 blk 25886 spindelay 72
PID 7308 lwlock main 40: shacq 41024 exacq 34794 blk 20780 spindelay 54
PID 7314 lwlock main 40: shacq 41195 exacq 34848 blk 20638 spindelay 60
PID 7288 lwlock main 41: shacq 84398 exacq 34750 blk 29591 spindelay 128
PID 7208 lwlock main 42: shacq 63107 exacq 34737 blk 20133 spindelay 81
PID 7245 lwlock main 43: shacq 278001 exacq 34601 blk 53473 spindelay 503
PID 7307 lwlock main 44: shacq 85155 exacq 34440 blk 19062 spindelay 71
PID 7301 lwlock main 45: shacq 61999 exacq 34757 blk 13184 spindelay 46
PID 7235 lwlock main 46: shacq 41199 exacq 34622 blk 9031 spindelay 30
PID 7324 lwlock main 46: shacq 40906 exacq 34692 blk 8799 spindelay 14
PID 7292 lwlock main 47: shacq 41180 exacq 34604 blk 8241 spindelay 25
PID 7303 lwlock main 48: shacq 40727 exacq 34651 blk 7567 spindelay 30
PID 7230 lwlock main 49: shacq 60416 exacq 34544 blk 9007 spindelay 28
PID 7300 lwlock main 50: shacq 44591 exacq 34763 blk 6687 spindelay 25
PID 7317 lwlock main 50: shacq 44349 exacq 34583 blk 6861 spindelay 22
PID 7305 lwlock main 51: shacq 62626 exacq 34671 blk 7864 spindelay 29
PID 7301 lwlock main 52: shacq 60646 exacq 34512 blk 7093 spindelay 36
PID 7324 lwlock main 53: shacq 39756 exacq 34359 blk 5138 spindelay 22

This data shows that after patch, there is no contention
for BufFreeListLock, rather there is a huge contention around
BufMappingLocks. I have checked that HEAD also has contention
around BufMappingLocks.

As per my analysis till now, I think reducing contention around
BufFreelistLock is not sufficient to improve scalability, we need
to work on reducing contention around BufMappingLocks as well.

Details of patch
------------------------
1. Changed bgwriter to move buffers (having usage_count as zero)
on free list based on threshold (high_watermark) and decrement the
usage count if usage_count is greater than zero.
2. StrategyGetBuffer() will wakeup bgwriter when the number of
buffers in freelist drop under low_watermark.
Currently I am using hard-coded values, we can choose to make
them as configurable later on if required.
3. Work done to get a buffer from freelist is done under spin lock
and clock sweep still runs under BufFreelistLock.

This is still a WIP patch and some of the changes are just kind
of prototype to check the idea, like I have hacked bgwriter code
such that it continuously fills the freelist till it is able to put
enough buffers on freelist such that it reaches high_watermark
and commented some part of previous code.

Thoughts?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 15, 2014 at 11:11 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>wrote:

>
> Data with LWLOCK_STATS
> ----------------------------------------------
> BufMappingLocks
>
> PID 7245 lwlock main 38: shacq 41117 exacq 34561 blk 36274 spindelay 101
> PID 7310 lwlock main 39: shacq 40257 exacq 34219 blk 25886 spindelay 72
> PID 7308 lwlock main 40: shacq 41024 exacq 34794 blk 20780 spindelay 54
> PID 7314 lwlock main 40: shacq 41195 exacq 34848 blk 20638 spindelay 60
> PID 7288 lwlock main 41: shacq 84398 exacq 34750 blk 29591 spindelay 128
> PID 7208 lwlock main 42: shacq 63107 exacq 34737 blk 20133 spindelay 81
> PID 7245 lwlock main 43: shacq 278001 exacq 34601 blk 53473 spindelay 503
> PID 7307 lwlock main 44: shacq 85155 exacq 34440 blk 19062 spindelay 71
> PID 7301 lwlock main 45: shacq 61999 exacq 34757 blk 13184 spindelay 46
> PID 7235 lwlock main 46: shacq 41199 exacq 34622 blk 9031 spindelay 30
> PID 7324 lwlock main 46: shacq 40906 exacq 34692 blk 8799 spindelay 14
> PID 7292 lwlock main 47: shacq 41180 exacq 34604 blk 8241 spindelay 25
> PID 7303 lwlock main 48: shacq 40727 exacq 34651 blk 7567 spindelay 30
> PID 7230 lwlock main 49: shacq 60416 exacq 34544 blk 9007 spindelay 28
> PID 7300 lwlock main 50: shacq 44591 exacq 34763 blk 6687 spindelay 25
> PID 7317 lwlock main 50: shacq 44349 exacq 34583 blk 6861 spindelay 22
> PID 7305 lwlock main 51: shacq 62626 exacq 34671 blk 7864 spindelay 29
> PID 7301 lwlock main 52: shacq 60646 exacq 34512 blk 7093 spindelay 36
> PID 7324 lwlock main 53: shacq 39756 exacq 34359 blk 5138 spindelay 22
>
> This data shows that after patch, there is no contention
> for BufFreeListLock, rather there is a huge contention around
> BufMappingLocks. I have checked that HEAD also has contention
> around BufMappingLocks.
>
> As per my analysis till now, I think reducing contention around
> BufFreelistLock is not sufficient to improve scalability, we need
> to work on reducing contention around BufMappingLocks as well.
>

To reduce the contention around BufMappingLocks, I have tried the patch
by just increasing the Number of Buffer Partitions, and it actually shows
a really significant increase in scalability both due to reduced contention
around BufFreeListLock and BufMappingLocks. The real effect of reducing
contention around BufFreeListLock was hidden because the whole contention
was shifted to BufMappingLocks. I have taken performance data for both
HEAD+increase_buf_part and Patch+increase_buf_part to clearly see the
benefit of reducing contention around BufFreeListLock. This data has been
taken using pgbench read only load (Select).

Performance Data
-------------------------------
HEAD + 64 = HEAD + (NUM_BUFFER_PARTITONS(64) +
LOG2_NUM_LOCK_PARTITIONS(6))
V1 + 64 = PATCH + (NUM_BUFFER_PARTITONS(64) +
LOG2_NUM_LOCK_PARTITIONS(6))
Similarly 128 means 128 buffer partitions

shared_buffers= 8GB
scale factor = 3000
RAM - 64GB

Thrds (64) Thrds (128) HEAD 45562 17128 HEAD + 64 57904 32810 V1 + 64
105557 81011 HEAD + 128 58383 32997 V1 + 128 110705 114544

shared_buffers= 8GB
scale factor = 1000
RAM - 64GB

Thrds (64) Thrds (128) HEAD 92142 31050 HEAD + 64 108120 86367 V1 + 64
117454 123429 HEAD + 128 107762 86902 V1 + 128 123641 124822

Observations
-------------------------
1. There is increase of upto 5 times in performance for data that can fit
in memory but not in shared buffers
2. Though there is a increase in performance by just increasing number
of buffer partitions, but it doesn't scale well (especially see the case
when partitions have increased to 128 from 64).

I have verified that contention has reduced around BufMappingLocks
by running the patch with LWLOCKS

BufFreeListLock
PID 17894 lwlock main 0: shacq 0 exacq 171 blk 27 spindelay 1

BufMappingLocks

PID 17902 lwlock main 38: shacq 12770 exacq 10104 blk 282 spindelay 0
PID 17924 lwlock main 39: shacq 11409 exacq 10257 blk 243 spindelay 0
PID 17929 lwlock main 40: shacq 13120 exacq 10739 blk 239 spindelay 0
PID 17940 lwlock main 41: shacq 11865 exacq 10373 blk 262 spindelay 0
..
..
PID 17831 lwlock main 162: shacq 12706 exacq 10267 blk 199 spindelay 0
PID 17826 lwlock main 163: shacq 11081 exacq 10256 blk 168 spindelay 0
PID 17903 lwlock main 164: shacq 11494 exacq 10375 blk 176 spindelay 0
PID 17899 lwlock main 165: shacq 12043 exacq 10485 blk 216 spindelay 0

We can clearly notice that the number for *blk* has reduced significantly
which shows that contention has reduced.

The patch is still in a shape to prove the merit of idea and I have just
changed the number of partitions so that if someone wants to verify
the performance for similar load, it can be done by just applying
the patch.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, May 16, 2014 at 10:51 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>wrote:

>
>
> Thrds (64) Thrds (128) HEAD 45562 17128 HEAD + 64 57904 32810 V1 + 64
> 105557 81011 HEAD + 128 58383 32997 V1 + 128 110705 114544
>

I haven't actually reviewed the code, but this sort of thing seems like
good evidence that we need your patch, or something like it. The fact that
the patch produces little performance improvement on it's own (though it
does produce some) shouldn't be held against it - the fact that the
contention shifts elsewhere when the first bottleneck is removed is not
your patch's fault.

In terms of ameliorating contention on the buffer mapping locks, I think it
would be better to replace the whole buffer mapping table with something
different. I started working on that almost 2 years ago, building a
hash-table that can be read without requiring any locks and written with,
well, less locking than what we have right now:

http://git.postgresql.org/gitweb/?p=users/rhaas/postgres.git;a=shortlog;h=refs/heads/chash

I never got quite as far as trying to hook that up to the buffer mapping
machinery, but maybe that would be worth doing.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, May 16, 2014 at 7:51 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>wrote:

> shared_buffers= 8GB
> scale factor = 3000
> RAM - 64GB
>
>
> Thrds (64) Thrds (128) HEAD 45562 17128 HEAD + 64 57904 32810 V1 + 64
> 105557 81011 HEAD + 128 58383 32997 V1 + 128 110705 114544
>
> shared_buffers= 8GB
> scale factor = 1000
> RAM - 64GB
>
>
> Thrds (64) Thrds (128) HEAD 92142 31050 HEAD + 64 108120 86367 V1 + 64
> 117454 123429 HEAD + 128 107762 86902 V1 + 128 123641 124822

I'm having a little trouble following this. These figure are transactions
per second for a 300 second pgbench tpc-b run? What does "Thrds" denote?

--
Peter Geoghegan

On Sat, May 17, 2014 at 6:29 AM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
>
>
> On Fri, May 16, 2014 at 7:51 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>>
>> shared_buffers= 8GB
>> scale factor = 3000
>> RAM - 64GB
>
> I'm having a little trouble following this. These figure are transactions
per second for a 300 second pgbench tpc-b run?

Yes, the figures are tps for a 300 second run.
It is for select-only transactions.

What does "Thrds" denote?
It denotes number of threads (-j in pgbench run)

I have used below statements to take data
./pgbench -c 64 -j 64 -T 300 -S postgres
./pgbench -c 128 -j 128 -T 300 -S postgres

The reason for posting the numbers for 64/128 threads is because we have
mainly concurrency bottleneck when the number of connections are higher
than CPU cores and I am using 16 cores, 64 hardware threads m/c.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sat, May 17, 2014 at 6:02 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> I haven't actually reviewed the code, but this sort of thing seems like
good evidence that we need your patch, or something like it. The fact that
the patch produces little performance improvement on it's own (though it
does produce some) shouldn't be held against it - the fact that the
contention shifts elsewhere when the first bottleneck is removed is not
your patch's fault.
>
> In terms of ameliorating contention on the buffer mapping locks, I think
it would be better to replace the whole buffer mapping table with something
different.

Is there anything bad except for may be increase in LWLocks with scaling
hash partitions w.r.t to shared buffers either by auto tuning or by having a
configuration knob. I understand that it would be bit difficult for users
to
estimate the correct value of such a parameter, we can provide info about
its usage in docs such that if user increases shared buffers by 'X' (20
times)
of default value (128M), then consider increasing such partitions and it
should
be always power of 2 or does something similar to above internally in code.

I agree that may be even by having a reasonably good estimate of number of
partitions w.r.t shared buffers, we might not be able to eliminate the
contention
around BufMappingLocks, but I think the scalability we get by doing that is
not
bad either.

> I started working on that almost 2 years ago, building a hash-table that
can be read without requiring any locks and written with, well, less
locking than what we have right now:

I have still not read the complete code, but by just going through initial
file
header, it seems to me that it will be much better than current
implementation in terms of concurrency, by the way does such an
implementation can extend to reducing scalability for hash indexes as well?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sat, May 17, 2014 at 6:02 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Fri, May 16, 2014 at 10:51 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>
>>
>>
>> Thrds (64) Thrds (128) HEAD 45562 17128 HEAD + 64 57904 32810 V1 + 64
>> 105557 81011 HEAD + 128 58383 32997 V1 + 128 110705 114544
>>
>
> I haven't actually reviewed the code, but this sort of thing seems like
> good evidence that we need your patch, or something like it. The fact that
> the patch produces little performance improvement on it's own (though it
> does produce some) shouldn't be held against it - the fact that the
> contention shifts elsewhere when the first bottleneck is removed is not
> your patch's fault.
>
>
>
I have improved the patch by making following changes:
a. Improved the bgwriter logic to log for xl_running_xacts info and
removed the hibernate logic as bgwriter will now work only when
there is scarcity of buffer's in free list. Basic idea is when the
number of buffers on freelist drops below the low threshold, the
allocating backend sets the latch and bgwriter wakesup and begin

adding buffer's to freelist until it reaches high threshold and then

again goes back to sleep.

b. New stats for number of buffers on freelist has been added, some
old one's like maxwritten_clean can be removed as new logic for
syncing buffers and moving them to free list doesn't use them.
However I think it's better to remove them once the new logic is
accepted. Added some new logs for info related to free list under
BGW_DEBUG

c. Used the already existing bgwriterLatch in BufferStrategyControl to
wake bgwriter when number of buffer's in freelist drops below
threshold.

d. Autotune the low and high threshold for freelist for various
configurations. Generally if keep small number (200~2000) of buffers
always available on freelist, then even for high shared buffers
like 15GB, it appears to be sufficient. However when the value
of shared buffer's is less, then we need much smaller number. I
think we can provide these as config knobs for user as well, but for
now based on LWLOCK_STATS result, I have chosen some hard
coded values for low and high threshold values for freelist.
Values for low and high threshold have been decided based on total
number of shared buffers, basically I have divided them into 5
categories (16~100, 100~1000, 1000~10000, 10000~100000,
100000 and above) and then ran tests(read-only pgbench) for various
configurations falling under these categories. The reason for keeping
lesser categories for larger shared buffers is that if there are small
number (200~2000) of buffers available on free list, then it seems to
be sufficient for quite high loads, however as the total number of
shared
buffer's decreases we need to be more careful as if we keep the number
as
too low then it will lead to more clock sweep by backends (which means
freelist lock contention) and if we keep number higher bgwriter will
evict
many useful buffers. Results based on LWLOCK_STATS is at end of mail.

e. One reason why I think number of buf-partitions is hard-coded to 16 is
that
minimum number of shared buffers allowed are 16 (128kb). However,
there
is handling in code (in function init_htab()) which ensure that even
if number
of partitions are more that shared buffers, it handles it safely.

I have checked the bgwriter CPU usage with and without patch
for various configurations and the observation is that for most of the
loads bgwriter's CPU usage after patch is between 8~20% and in
HEAD it is 0~2%. It shows that with patch when shared buffers
are under use by backends, bgwriter is constantly doing work to
ease the work of backends. Detailed data is provided later in the
mail.

Performance Data:
-------------------------------

Configuration and Db Details

IBM POWER-7 16 cores, 64 hardware threads

RAM = 64GB

Database Locale =C

checkpoint_segments=256

checkpoint_timeout =15min

shared_buffers=8GB

scale factor = 3000

Client Count = number of concurrent sessions and threads (ex. -c 8 -j 8)

Duration of each individual run = 5mins

Client Count/patch_ver (tps) 8 16 32 64 128 Head 26220 48686 70779 45232
17310 Patch 26402 50726 75574 111468 114521

Data is taken by using script (pert_buff_mgmt.sh) attached with mail.
This data is read-only pgbench data with different number of client
connections. All the numbers are in tps. This data is median of 3
5 min pgbench read-only runs. Please find the detailed data for 3 runs
in attached open office document (perf_read_scalability_data_v3.ods)

This data clearly shows that patch has improved improved performance
upto 5~6 times.

Results of BGwriter CPU usage:
--------------------------------------------------

Here sc is scale factor and sb is shared buffers and the data is
for read-only pgbench runs.

./pgbench -c 64 - j 64 -S -T 300 postgres
sc - 3000, sb - 8GB
HEAD
CPU usage - 0~2.3%
Patch v_3
CPU usage - 8.6%

sc - 100, sb - 128MB
./pgbench -c 64 - j 64 -S -T 300 postgres
HEAD
CPU Usage - 1~2%
tps- 36199.047132
Patch v_3
CPU usage - 12~13%
tps = 109182.681827

sc - 50, sb - 75MB
./pgbench -c 64 - j 64 -S -T 300 postgres
HEAD
CPU Usage - 0.7~2%
tps- 37760.575128
Patch v_3
CPU usage - 20~22%
tps = 106310.744198

./pgbench -c 16 - j 16 -S -T 300 postgres
sc - 100, sb - 128kb
--need to change pgbench for this.
HEAD
CPU Usage - 0~0.3%
tps- 40979.529254
Patch v_3
CPU usage - 35~40%
tps = 42956.785618

Results of LWLOCK_STATS based on low-high threshold values of freelist:
--------------------------------------------------------------------------------------------------------------

In the results, values of exacq and blk shows the contention on freelist
lock.
sc is scale factor and sb is number of shared_buffers. Below results shows
that for all except one (1MB) of configuration the contention around
buffreelist
lock is reduced significantly. For 1MB case also, it has reduced exacq
count
which shows that it has performed clock sweep lesser number of times.

sc - 3000, sb - 15GB --(sb > 100000)
./pgbench -c 64 - j 64 -S -T 300 postgres
HEAD
PID 4406 lwlock main 0: shacq 0 exacq 84482 blk 5139 spindelay 62
Patch v_3
PID 4864 lwlock main 0: shacq 0 exacq 34 blk 1 spindelay 0

sc - 3000, sb - 8GB --(sb > 100000)
./pgbench -c 64 - j 64 -S -T 300 postgres
HEAD
PID 24124 lwlock main 0: shacq 0 exacq 285155 blk 33910 spindelay 548
Patch v_3
PID 7257 lwlock main 0: shacq 0 exacq 165 blk 18 spindelay 0

sc - 100, sb - 768MB --(sb > 10000)
./pgbench -c 64 - j 64 -S -T 300 postgres
HEAD
PID 9144 lwlock main 0: shacq 0 exacq 284636 blk 34091 spindelay 555
Patch v-3 (lw=100,hg=1000)
PID 9428 lwlock main 0: shacq 0 exacq 306 blk 59 spindelay 0

sc - 100, sb - 128MB --(sb > 10000)
./pgbench -c 64 - j 64 -S -T 300 postgres
HEAD
PID 5405 lwlock main 0: shacq 0 exacq 285449 blk 32345 spindelay 714
Patch v-3
PID 8625 lwlock main 0: shacq 0 exacq 740 blk 178 spindelay 0

sc - 50, sb - 75MB --(sb > 1000)
./pgbench -c 64 - j 64 -S -T 300 postgres
HEAD
PID 12681 lwlock main 0: shacq 0 exacq 289347 blk 34064 spindelay 773
Patch v3
PID 12800 lwlock main 0: shacq 0 exacq 76287 blk 15183 spindelay 28

sc - 50, sb - 10MB --(sb > 1000)
./pgbench -c 64 - j 64 -S -T 300 postgres
HEAD
PID 10283 lwlock main 0: shacq 0 exacq 287500 blk 32177 spindelay 864
Patch v3 (for > 1000, lw = 50 hg =200)
PID 11629 lwlock main 0: shacq 0 exacq 60139 blk 12978 spindelay 40

sc - 1, sb - 7MB --(sb > 100)
./pgbench -c 64 - j 64 -S -T 300 postgres
HEAD
PID 47127 lwlock main 0: shacq 0 exacq 289462 blk 37057 spindelay 119
Patch v3
PID 47283 lwlock main 0: shacq 0 exacq 9507 blk 1656 spindelay 0

sc - 1, sb - 1MB --(sb > 100)
./pgbench -c 64 - j 64 -S -T 300 postgres
HEAD
PID 43215 lwlock main 0: shacq 0 exacq 301384 blk 36740 spindelay 902
Patch v3
PID 46542 lwlock main 0: shacq 0 exacq 197231 blk 37532 spindelay 294

sc - 100, sb - 128kb focus(sb > 16)
./pgbench -c 16 - j 16 -S -T 300 postgres (for this, I need to reduce value
of naccounts to 2500, else it was always giving no unpinned buffers
available)
HEAD
PID 49751 lwlock main 0: shacq 0 exacq 1821276 blk 130119 spindelay 7
Patch v3
PID 50768 lwlock main 0: shacq 0 exacq 382610 blk 46543 spindelay 1

More Datapoints and work:
a. I have yet to take data by merging it with scalable lwlock patch of
Andres (https://commitfest.postgresql.org/action/patch_view?id=1313).
There are many conflicts in the patch, so waiting for an updated patch.
b. Read-only data for more configurations.
c. Data for Write work load (tpc-b of pgbench, Bulk insert (Copy))
d. Update docs and Remove unused code.

Suggestions?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> I have improved the patch  by making following changes:
> a. Improved the bgwriter logic to log for xl_running_xacts info and
>    removed the hibernate logic as bgwriter will now work only when
>    there is scarcity of buffer's in free list. Basic idea is when the
>    number of buffers on freelist drops below the low threshold, the
>    allocating backend sets the latch and bgwriter wakesup and begin
>    adding buffer's to freelist until it reaches high threshold and then
>    again goes back to sleep.

The numbers from your benchmarks are very exciting, but the above
concerns me.  My tuning of the bgwriter in production has generally
*not* been aimed at keeping pages on the freelist, but toward
preventing shared_buffers from accumulating a lot of dirty pages,
which were leading to cascades of writes between caches and thus to
write stalls.  By pushing dirty pages into the (*much* larger) OS
cache, and letting write combining happen there, where the OS could
pace based on the total number of dirty pages instead of having
some hidden and appearing rather suddenly, latency spikes were
avoided while not causing any noticeable increase in the number of
OS writes to the RAID controller's cache.

Essentially I was able to tune the bgwriter so that a dirty page
was always push out to the OS cache within three seconds, which led
to a healthy balance of writes between the checkpoint process and
the bgwriter. Backend processes related to user connections still
performed about 30% of the writes, and this work shows promise
toward bringing that down, which would be great; but please don't
eliminate the ability to prevent write stalls in the process.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sun, Jun 8, 2014 at 7:21 PM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> > I have improved the patch by making following changes:
> > a. Improved the bgwriter logic to log for xl_running_xacts info and
> > removed the hibernate logic as bgwriter will now work only when
> > there is scarcity of buffer's in free list. Basic idea is when the
> > number of buffers on freelist drops below the low threshold, the
> > allocating backend sets the latch and bgwriter wakesup and begin
> > adding buffer's to freelist until it reaches high threshold and then
> > again goes back to sleep.
>
> The numbers from your benchmarks are very exciting, but the above
> concerns me. My tuning of the bgwriter in production has generally
> *not* been aimed at keeping pages on the freelist, but toward
> preventing shared_buffers from accumulating a lot of dirty pages,
> which were leading to cascades of writes between caches and thus to
> write stalls. By pushing dirty pages into the (*much* larger) OS
> cache, and letting write combining happen there, where the OS could
> pace based on the total number of dirty pages instead of having
> some hidden and appearing rather suddenly, latency spikes were
> avoided while not causing any noticeable increase in the number of
> OS writes to the RAID controller's cache.
>
> Essentially I was able to tune the bgwriter so that a dirty page
> was always push out to the OS cache within three seconds, which led
> to a healthy balance of writes between the checkpoint process and
> the bgwriter.

I think it would have been better if bgwriter does writes based on
the amount of buffer's that get dirtied to achieve the balance of
writes.

> Backend processes related to user connections still
> performed about 30% of the writes, and this work shows promise
> toward bringing that down, which would be great; but please don't
> eliminate the ability to prevent write stalls in the process.

I agree that for some cases as explained by you, the current bgwriter
logic does satisfy the need, however there are other cases as well
where actually it doesn't help much, one of such cases I am trying to
improve (ease backend buffer allocations) and other may be when
there is constant write activity for which I am not sure how much it
really helps. Part of the reason for trying to make bgwriter respond
mainly to ease backend allocations is the previous discussion for
the same, refer below link:
http://www.postgresql.org/message-id/CA+TgmoZ7dvhC4h-ffJmZCff6VWyNfOEAPZ021VxW61uH46R3QA@mail.gmail.com

However if we want to retain current property of bgwriter, we can do
the same by one of below ways:
a. Have separate processes for writing dirty buffers and moving buffers
to freelist.
b. In the current bgwriter, separate the two works based on the need.
The need can be decided based on whether bgwriter has been waked
due to shortage of buffers on free list or if it has been waked due to
BgWriterDelay.

Now as populating freelist and balance writes by writing dirty buffers
are two separate responsibilities, so not sure if doing that by one
process is a good idea.

I am planing to take some more performance data, part of which will
be write load as well, but I am now sure if that can anyway show the
need as mentioned by you.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sun, Jun 8, 2014 at 9:51 AM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> I have improved the patch by making following changes:
>> a. Improved the bgwriter logic to log for xl_running_xacts info and
>> removed the hibernate logic as bgwriter will now work only when
>> there is scarcity of buffer's in free list. Basic idea is when the
>> number of buffers on freelist drops below the low threshold, the
>> allocating backend sets the latch and bgwriter wakesup and begin
>> adding buffer's to freelist until it reaches high threshold and then
>> again goes back to sleep.
>
> The numbers from your benchmarks are very exciting, but the above
> concerns me. My tuning of the bgwriter in production has generally
> *not* been aimed at keeping pages on the freelist,

Just to be clear, prior to this patch, the bgwriter has never been in
the business of putting pages on the freelist in the first place, so
it wouldn't have been possible for you to tune for that.

> Essentially I was able to tune the bgwriter so that a dirty page
> was always push out to the OS cache within three seconds, which led
> to a healthy balance of writes between the checkpoint process and
> the bgwriter. Backend processes related to user connections still
> performed about 30% of the writes, and this work shows promise
> toward bringing that down, which would be great; but please don't
> eliminate the ability to prevent write stalls in the process.

I think, as Amit says downthread, that the crucial design question
here is whether we need two processes, one to populate the freelist so
that regular backends don't need to run the clock sweep, and a second
to flush dirty buffers, or whether a single process can serve both
needs. In favor of a single process, many people have commented that
the background writer doesn't seem to do much right now. If the
process is mostly sitting around idle, then giving it more
responsibilities might be OK. In favor of having a second process,
I'm a little concerned that if the background writer gets busy writing
a page, it might then be unavailable to populate the freelist until it
finishes, which might be a very long time relative to the buffer
allocation needs of other backends. I'm not sure what the right
answer is.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jun 9, 2014 at 9:33 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Sun, Jun 8, 2014 at 7:21 PM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> > Backend processes related to user connections still
> > performed about 30% of the writes, and this work shows promise
> > toward bringing that down, which would be great; but please don't
> > eliminate the ability to prevent write stalls in the process.
>
>
> I am planing to take some more performance data, part of which will
> be write load as well, but I am now sure if that can anyway show the
> need as mentioned by you.

After taking the performance data for write load using tpc-b with the
patch, I found that there is a regression in it. So I went ahead and
tried to figure out the reason for same and found that after patch,
Bgwriter started flushing buffers which were required by backends
and reason was that *nextVictimBuffer* was not getting updated
properly while we are running clock sweep kind of logic (decrement
the usage count when number of buffers on freelist fall below low
threshhold value) in Bgwriter. In HEAD, I noticed that at default
settings, BGwriter was not at all flushing any buffers which is at least
better than what my patch was doing (flushing buffers required by
backend).

So I tried to fix the issue by updating *nextVictimBuffer* in new
BGWriter logic and results are positive.

sbe - scalable buffer eviction

Select only Data
Client count/TPS64128Un-patched4523217310sbe_v3111468114521sbe_v4153137
160752
TPC-B

Client count/TPS
64128Un-patched825784sbe_v4814845

For Select Data, I am quite confident that it will improve if we introduce
nextVictimBuffer increments in BGwriter and rather it scales much better
with that change, however for TPC-B, I am getting fluctuation in data,
so not sure it has eliminated the problem. The main difference is that in
HEAD, BGwriter never increments nextVictimBuffer during syncing the
buffers, it just notes down the current setting before start and then
proceeds sequentially.

I think it will be good if we can have a new process for moving buffers to
free list due to below reasons:

a. while trying to move buffers to freelist, it should not block due
to in between write activity.
b. The writer should not increment nextVictimBuffer and maintain
the current logic.

One significant change in this version of patch is to use a separate
spin lock to protect nextVictimBuffer rather than using BufFreelistLock.

Suggestions?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Jun 5, 2014 at 4:43 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> I have improved the patch by making following changes:
> a. Improved the bgwriter logic to log for xl_running_xacts info and
> removed the hibernate logic as bgwriter will now work only when
> there is scarcity of buffer's in free list. Basic idea is when the
> number of buffers on freelist drops below the low threshold, the
> allocating backend sets the latch and bgwriter wakesup and begin
>
> adding buffer's to freelist until it reaches high threshold and then
>
> again goes back to sleep.
>

This essentially removes BgWriterDelay, but it's still mentioned in
BgBufferSync(). Looking further, I see that with the patch applied,
BgBufferSync() is still present in the source code but is no longer called
from anywhere. Please don't submit patches that render things unused
without actually removing them; it makes it much harder to see what you've
changed. I realize you probably left it that way for testing purposes, but
you need to clean such things up before submitting. Likewise, if you've
rendered GUCs or statistics counters removed, you need to rip them out, so
that the scope of the changes you've made is clear to reviewers.

A comparison of BgBufferSync() with BgBufferSyncAndMoveBuffersToFreelist()
reveals that you've removed at least one behavior that some people (at
least, me) will care about, which is the guarantee that the background
writer will scan the entire buffer pool at least every couple of minutes.
This is important because it guarantees that dirty data doesn't sit in
memory forever. When the system becomes busy again after a long idle
period, users will expect the system to have used the idle time to flush
dirty buffers to disk. This also improves data recovery prospects if, for
example, somebody loses their pg_xlog directory - there may be dirty
buffers whose contents are lost, of course, but they won't be months old.

b. New stats for number of buffers on freelist has been added, some
> old one's like maxwritten_clean can be removed as new logic for
> syncing buffers and moving them to free list doesn't use them.
> However I think it's better to remove them once the new logic is
> accepted. Added some new logs for info related to free list under
> BGW_DEBUG
>

If I'm reading this right, the new statistic is an incrementing counter
where, every time you update it, you add the number of buffers currently on
the freelist. That makes no sense. I think what you should be counting is
the number of allocations that are being satisfied from the free-list.
Then, by comparing the rate at which that value is incrementing to the rate
at which buffers_alloc is incrementing, somebody can figure out what
percentage of allocations are requiring a clock-sweep run. Actually, I
think it's better to flip it around: count the number of allocations that
require an individual backend to run the clock sweep (vs. being satisfied
from the free-list); call it, say, buffers_backend_clocksweep. We can then
try to tune the patch to make that number as small as possible under
varying workloads.

c. Used the already existing bgwriterLatch in BufferStrategyControl to
> wake bgwriter when number of buffer's in freelist drops below
> threshold.
>

Seems like a good idea.

> d. Autotune the low and high threshold for freelist for various
> configurations. Generally if keep small number (200~2000) of buffers
> always available on freelist, then even for high shared buffers
> like 15GB, it appears to be sufficient. However when the value
> of shared buffer's is less, then we need much smaller number. I
> think we can provide these as config knobs for user as well, but for
> now based on LWLOCK_STATS result, I have chosen some hard
> coded values for low and high threshold values for freelist.
> Values for low and high threshold have been decided based on total
> number of shared buffers, basically I have divided them into 5
> categories (16~100, 100~1000, 1000~10000, 10000~100000,
> 100000 and above) and then ran tests(read-only pgbench) for various
> configurations falling under these categories. The reason for keeping
> lesser categories for larger shared buffers is that if there are small
> number (200~2000) of buffers available on free list, then it seems to
> be sufficient for quite high loads, however as the total number of
> shared
> buffer's decreases we need to be more careful as if we keep the
> number as
> too low then it will lead to more clock sweep by backends (which means
> freelist lock contention) and if we keep number higher bgwriter will
> evict
> many useful buffers. Results based on LWLOCK_STATS is at end of mail.
>

I think we need to come up with some kind of formula here rather than just
a list of hard-coded constants. And it definitely needs some comments
explaining the logic behind the choices.

Aside from those specific remarks, I think the elephant in the room is the
question of whether it really makes sense to have one process which is
responsible both for populating the free list and for writing buffers to
disk. One problem, which I alluded to above under point (1), is that we
might sometimes want to ensure that dirty buffers are written out to disk
without decrementing usage counts or adding anything to the free list.
This is a potentially solvable problem, though, because we can figure out
the number of buffers that we need to scan for freelist population and the
number that we need to scan for minimum buffer pool cleaning (one cycle
every 2 minutes). Once we've met the first goal, any further buffers we
run into under the second goal get cleaned if appropriate but their usage
counts don't get pushed down nor do they get added to the freelist. Once
we meet the second goal, we can go back to sleep.

But the other problem, which I think is likely unsolvable, is that writing
a dirty page can take a long time on a busy system (multiple seconds) and
the freelist can be emptied much, much quicker than that (milliseconds).
Although your benchmark results show great speed-ups on read-only
workloads, we're not really going to get the benefit consistently on
read-write workloads -- unless of course the background writer fails to
actually write anything, which should be viewed as a bug, not a feature --
because the freelist will often be empty while the background writer is
blocked on I/O.

I'm wondering if it would be a whole lot simpler and better to introduce a
new background process, maybe with a name like bgreclaim. That process
wouldn't write dirty buffers. Instead, it would just run the clock sweep
(i.e. the last loop inside StrategyGetBuffer) and put the buffers onto the
free list. Then, we could leave the bgwriter logic more or less intact.
It certainly needs improvement, but that could be another patch.

Incidentally, while I generally think your changes to the locking regimen
in StrategyGetBuffer() are going in the right direction, they need
significant cleanup. Your patch adds two new spinlocks, freelist_lck and
victimbuf_lck, that mostly but not-quite replace BufFreelistLock, and
you've now got StrategyGetBuffer() running with no lock at all when
accessing some things that used to be protected by BufFreelistLock;
specifically, you're doing StrategyControl->numBufferAllocs++ and
SetLatch(StrategyControl->bgwriterLatch) without any locking. That's not
OK. I think you should get rid of BufFreelistLock completely and just
decide that freelist_lck will protect everything the freeNext links, plus
everything in StrategyControl except for nextVictimBuffer. victimbuf_lck
will protect nextVictimBuffer and nothing else.

Then, in StrategyGetBuffer, acquire the freelist_lck at the point where the
LWLock is acquired today. Increment StrategyControl->numBufferAllocs; save
the values of StrategyControl->bgwriterLatch; pop a buffer off the freelist
if there is one, saving its identity. Release the spinlock. Then, set the
bgwriterLatch if needed. In the first loop, first check whether the buffer
we previously popped from the freelist is pinned or has a non-zero usage
count and return it if not, holding the buffer header lock. Otherwise,
reacquire the spinlock just long enough to pop a new potential victim and
then loop around.

Under this locking strategy, StrategyNotifyBgWriter would use
freelist_lck. Right now, the patch removes the only caller, and should
therefore remove the function as well, but if we go with the new-process
idea listed above that part would get reverted, and then you'd need to make
it use the correct spinlock. You should also go through this patch and
remove all the commented-out bits and pieces that you haven't cleaned up;
those are distracting and unhelpful.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Aug 5, 2014 at 9:21 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jun 5, 2014 at 4:43 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> This essentially removes BgWriterDelay, but it's still mentioned in
BgBufferSync(). Looking further, I see that with the patch applied,
BgBufferSync() is still present in the source code but is no longer called
from anywhere. Please don't submit patches that render things unused
without actually removing them; it makes it much harder to see what you've
changed. I realize you probably left it that way for testing purposes, but
you need to clean such things up before submitting. Likewise, if you've
rendered GUCs or statistics counters removed, you need to rip them out, so
that the scope of the changes you've made is clear to reviewers.

I have kept it just for the reason that if the basic approach is
sounds reasonable/accepted, then I will clean it up. Sorry for
the inconvenience, I didn't realized that it can be annoying for
reviewer, I will remove all such code from patch in next version.

>
> A comparison of BgBufferSync() with
BgBufferSyncAndMoveBuffersToFreelist() reveals that you've removed at least
one behavior that some people (at least, me) will care about, which is the
guarantee that the background writer will scan the entire buffer pool at
least every couple of minutes.

Okay, I will take care of this based on the conclusion of
the other points in this mail.

>This is important because it guarantees that dirty data doesn't sit in
memory forever. When the system becomes busy again after a long idle
period, users will expect the system to have used the idle time to flush
dirty buffers to disk. This also improves data recovery prospects if, for
example, somebody loses their pg_xlog directory - there may be dirty
buffers whose contents are lost, of course, but they won't be months old.
>
>> b. New stats for number of buffers on freelist has been added, some
>> old one's like maxwritten_clean can be removed as new logic for
>> syncing buffers and moving them to free list doesn't use them.
>> However I think it's better to remove them once the new logic is
>> accepted. Added some new logs for info related to free list under
>> BGW_DEBUG
>
>
> If I'm reading this right, the new statistic is an incrementing counter
where, every time you update it, you add the number of buffers currently on
the freelist. That makes no sense.

I think using 'number of buffers currently on the freelist' and
'number of recently allocated buffers' for consecutive cycles,
we can figure out approximately how many buffer allocations
needs clock sweep assuming low and high threshold water
marks are fixed. However there can be cases where it is not
easy to estimate that number.

> I think what you should be counting is the number of allocations that are
being satisfied from the free-list. Then, by comparing the rate at which
that value is incrementing to the rate at which buffers_alloc is
incrementing, somebody can figure out what percentage of allocations are
requiring a clock-sweep run. Actually, I think it's better to flip it
around: count the number of allocations that require an individual backend
to run the clock sweep (vs. being satisfied from the free-list); call it,
say, buffers_backend_clocksweep. We can then try to tune the patch to make
that number as small as possible under varying workloads.

This can give us clear idea to tune the patch, however we need
to maintain 3 counters for it in code(recent_alloc (needed for
current bgwriter logic) and other 2 suggested by you). Do you
want to retain such counters in code or it's for kind of debug info
for patch?

>
>>
>> d. Autotune the low and high threshold for freelist for various
>> configurations.
>
> I think we need to come up with some kind of formula here rather than
just a list of hard-coded constants.

That was my initial intention as well and I have tried based
on number of shared buffers like keeping threshold values as
percentage of shared buffers but nothing could satisfy different
kind of workloads. The current values I have choosen are based
on experiments for various workloads at different thresholds. I have
shown the lwlock_stats data for various loads based on current
thresholds upthread. Another way could be to make them as config
knobs and use the values as given by user incase it is provided by
user else go with fixed values.

There are other instances in code as well (one of them I remember
offhand is in pglz_compress) where we use fixed values based on
different sizes.

>And it definitely needs some comments explaining the logic behind the
choices.

Agreed, I shall improve them in next version of patch.

>
> Aside from those specific remarks, I think the elephant in the room is
the question of whether it really makes sense to have one process which is
responsible both for populating the free list and for writing buffers to
disk. One problem, which I alluded to above under point (1), is that we
might sometimes want to ensure that dirty buffers are written out to disk
without decrementing usage counts or adding anything to the free list.
This is a potentially solvable problem, though, because we can figure out
the number of buffers that we need to scan for freelist population and the
number that we need to scan for minimum buffer pool cleaning (one cycle
every 2 minutes). Once we've met the first goal, any further buffers we
run into under the second goal get cleaned if appropriate but their usage
counts don't get pushed down nor do they get added to the freelist. Once
we meet the second goal, we can go back to sleep.
>
> But the other problem, which I think is likely unsolvable, is that
writing a dirty page can take a long time on a busy system (multiple
seconds) and the freelist can be emptied much, much quicker than that
(milliseconds). Although your benchmark results show great speed-ups on
read-only workloads, we're not really going to get the benefit consistently
on read-write workloads -- unless of course the background writer fails to
actually write anything, which should be viewed as a bug, not a feature --
because the freelist will often be empty while the background writer is
blocked on I/O.
>
> I'm wondering if it would be a whole lot simpler and better to introduce
a new background process, maybe with a name like bgreclaim.

That will certainly help in retaining the current behaviour of
bgwriter and make the idea cleaner. I will modify the patch
to have a new background process unless somebody thinks
otherwise.

>That process wouldn't write dirty buffers.

If we go with this approach, one thing which we need to decide
is what to do incase buf which has usage_count as zero is *dirty*,
as I don't think it is good idea to put it in freelist. Few options to
handle such a case are:

a. Skip such a buffer; the downside is if we have to skip lot
of buffers due to this reason then having separate process
such as bgreclaim will be less advantageous.
b. Skip the buffer and notify bgwriter to flush buffers, now this
notification can be sent either as soon as we encounter one
such buffer or after few such buffers (incase of few, we need to decide
some useful number). In this option, there is a chance that bgwriter
decide not to flush buffer/'s which ideally should not happen because
I think bgwriter considers the number of recent allocations for
performing scan to flush dirt buffers.
c. Have some mechanism where bgreclaim can notify bgwriter
to flush some specific buffers. I think if we have such a mechanism
that can be later even used by backends if required.
d. Keep the logic as per current patch and improve such that it can
retain the behaviour of one cycle per two minutes as suggested above
by you on the basis that in anycase it is better than the current code.

I don't think option (d) is best way to handle this scenario,however I
kept it incase nothing else sounds reasonable. Option (c) might have
lot of work which I am not sure is justifiable to handle the current
scenario,
though it can be useful for some other things. Option (a) should be okay
for most cases, but I think option (b) would be better.

> Instead, it would just run the clock sweep (i.e. the last loop inside
StrategyGetBuffer) and put the buffers onto the free list.

Don't we need to do more than just last loop inside StrategyGetBuffer(),
as clock sweep in strategy get buffer is responsible for getting one
buffer with usage_count = 0 where as we need to run the loop till it
finds and moves enough such buffers so that it can populate freelist
with number of buffers equal to high water mark of freelist.

>Then, we could leave the bgwriter logic more or less intact. It certainly
needs improvement, but that could be another patch.
>
> Incidentally, while I generally think your changes to the locking regimen
in StrategyGetBuffer() are going in the right direction, they need
significant cleanup. Your patch adds two new spinlocks, freelist_lck and
victimbuf_lck, that mostly but not-quite replace BufFreelistLock, and
you've now got StrategyGetBuffer() running with no lock at all when
accessing some things that used to be protected by BufFreelistLock;
specifically, you're doing StrategyControl->numBufferAllocs++ and
SetLatch(StrategyControl->bgwriterLatch) without any locking. That's not
OK.

I have kept them outside spinlock because as per patch the only
callsite for setting StrategyControl->bgwriterLatch is StrategyGetBuffer()
and StrategyControl->numBufferAllocs is used just for statistics purpose
(which I thought might be okay even if it is not accurate) whereas without
patch it is used by bgwriter for purpose other than stats as well.
However it certainly needs to be protected for separate bgreclaim process
idea or for retaining current bgwriter behaviour.

> I think you should get rid of BufFreelistLock completely and just decide
that freelist_lck will protect everything the freeNext links, plus
everything in StrategyControl except for nextVictimBuffer. victimbuf_lck
will protect nextVictimBuffer and nothing else.
>
> Then, in StrategyGetBuffer, acquire the freelist_lck at the point where
the LWLock is acquired today. Increment StrategyControl->numBufferAllocs;
save the values of StrategyControl->bgwriterLatch; pop a buffer off the
freelist if there is one, saving its identity. Release the spinlock.
Then, set the bgwriterLatch if needed. In the first loop, first check
whether the buffer we previously popped from the freelist is pinned or has
a non-zero usage count and return it if not, holding the buffer header
lock. Otherwise, reacquire the spinlock just long enough to pop a new
potential victim and then loop around.

I shall take care of doing this way in next version of patch.

>
> Under this locking strategy, StrategyNotifyBgWriter would use
freelist_lck. Right now, the patch removes the only caller, and should
therefore remove the function as well, but if we go with the new-process
idea listed above that part would get reverted, and then you'd need to make
it use the correct spinlock. You should also go through this patch and
remove all the commented-out bits and pieces that you haven't cleaned up;
those are distracting and unhelpful.

Sure.

Thank you for review.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Aug 6, 2014 at 6:12 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> If I'm reading this right, the new statistic is an incrementing counter
>> where, every time you update it, you add the number of buffers currently on
>> the freelist. That makes no sense.
>
> I think using 'number of buffers currently on the freelist' and
> 'number of recently allocated buffers' for consecutive cycles,
> we can figure out approximately how many buffer allocations
> needs clock sweep assuming low and high threshold water
> marks are fixed. However there can be cases where it is not
> easy to estimate that number.

Counters should be design in such a way that you can read it, and then
read it again later, and make sense of it - you should not need to
read the counter on *consecutive* cycles to interpret it.

>> I think what you should be counting is the number of allocations that are
>> being satisfied from the free-list. Then, by comparing the rate at which
>> that value is incrementing to the rate at which buffers_alloc is
>> incrementing, somebody can figure out what percentage of allocations are
>> requiring a clock-sweep run. Actually, I think it's better to flip it
>> around: count the number of allocations that require an individual backend
>> to run the clock sweep (vs. being satisfied from the free-list); call it,
>> say, buffers_backend_clocksweep. We can then try to tune the patch to make
>> that number as small as possible under varying workloads.
>
> This can give us clear idea to tune the patch, however we need
> to maintain 3 counters for it in code(recent_alloc (needed for
> current bgwriter logic) and other 2 suggested by you). Do you
> want to retain such counters in code or it's for kind of debug info
> for patch?

I only mean to propose one new counter, and I'd imagine including that
in the final patch. We already have a counter of total buffer
allocations; that's buffers_alloc. I'm proposing to add an additional
counter for the number of those allocations not satisfied from the
free list, with a name like buffers_alloc_clocksweep (I said
buffers_backend_clocksweep above, but that's probably not best, as the
existing buffers_backend counts buffer *writes*, not allocations). I
think we would definitely want to retain this counter in the final
patch, as an additional column in pg_stat_bgwriter.

>>> d. Autotune the low and high threshold for freelist for various
>>> configurations.
>>
>> I think we need to come up with some kind of formula here rather than just
>> a list of hard-coded constants.
>
> That was my initial intention as well and I have tried based
> on number of shared buffers like keeping threshold values as
> percentage of shared buffers but nothing could satisfy different
> kind of workloads. The current values I have choosen are based
> on experiments for various workloads at different thresholds. I have
> shown the lwlock_stats data for various loads based on current
> thresholds upthread. Another way could be to make them as config
> knobs and use the values as given by user incase it is provided by
> user else go with fixed values.

How did you go about determining the optimal value for a particular workload?

When the list is kept short, it's less likely that a value on the list
will be referenced or dirtied again before the page is actually
recycled. That's clearly good. But when the list is long, it's less
likely to become completely empty and thereby force individual
backends to run the clock-sweep. My suspicion is that, when the
number of buffers is small, the impact of the list being too short
isn't likely to be very significant, because running the clock-sweep
isn't all that expensive anyway - even if you have to scan through the
entire buffer pool multiple times, there aren't that many buffers.
But when the number of buffers is large, those repeated scans can
cause a major performance hit, so having an adequate pool of free
buffers becomes much more important.

I think your list of high-watermarks is far too generous for low
buffer counts. With more than 100k shared buffers, you've got a
high-watermark of 2k buffers, which means that 2% or less of the
buffers will be on the freelist, which seems a little on the high side
to me, but probably in the ballpark of what we should be aiming for.
But at 10001 shared buffers, you can have 1000 of them on the
freelist, which is 10% of the buffer pool; that seems high. At 101
shared buffers, 75% of the buffers in the system can be on the
freelist; that seems ridiculous. The chances of a buffer still being
unused by the time it reaches the head of the freelist seem very
small.

Based on your existing list of thresholds, and taking the above into
account, I'd suggest something like this: let the high-watermark for
the freelist be 0.5% of the total number of buffers, with a maximum of
2000 and a minimum of 5. Let the low-watermark be 20% of the
high-watermark. That might not be best, but I think some kind of
formula like that can likely be made to work. I would suggest
focusing your testing on configurations with *large* settings for
shared_buffers, say 1-64GB, rather than small configurations. Anyone
who cares greatly about performance isn't going to be running with
only 8MB of shared_buffers anyway. Arguably we shouldn't even run the
reclaim process on very small configurations; I think there should
probably a GUC (PGC_SIGHUP) to control whether it gets launched.

I think it would be a good idea to analyze how frequently the reclaim
process gets woken up. In the worst case, this happens once per (high
watermark - low watermark) allocations; that is, the system reaches
the low watermark and then does no further allocations until the
reclaim process brings the freelist back up to the high watermark.
But if more allocations occur between the time the reclaim process is
woken and the time it reaches the high watermark, then it should run
for longer, until the high watermark is reached. At least for
debugging purposes, I think it would be useful to have a counter of
reclaim wakeups. I'm not sure whether that's worth including in the
final patch, but it might be.

> That will certainly help in retaining the current behaviour of
> bgwriter and make the idea cleaner. I will modify the patch
> to have a new background process unless somebody thinks
> otherwise.
>
> If we go with this approach, one thing which we need to decide
> is what to do incase buf which has usage_count as zero is *dirty*,
> as I don't think it is good idea to put it in freelist.

I thought a bit about this yesterday. I think the problem is that we
might be in a situation where buffers are being dirtied faster than
they can be cleaned. In that case, if we only put clean buffers on the
freelist, then every backend in the system will be fighting over the
ever-dwindling supply of clean buffers until, in the worst case,
there's maybe only 1 clean buffer which is getting evicted repeatedly
at top speed - or maybe even no clean buffers, and the reclaim process
just spins in an infinite loop looking for clean buffers that aren't
there.

To put that another way, the rate at which buffers are being dirtied
can't exceed the rate at which they are being cleaned forever.
Eventually, somebody is going to have to wait. Having the backends
wait by being forced to write some dirty buffers does not seem like a
bad way to accomplish that. So I favor just putting the buffers on
freelist without regard to whether they are clean or dirty. If this
turns out not to work well we can look at other options (probably some
variant of (b) from your list).

>> Instead, it would just run the clock sweep (i.e. the last loop inside
>> StrategyGetBuffer) and put the buffers onto the free list.
>
> Don't we need to do more than just last loop inside StrategyGetBuffer(),
> as clock sweep in strategy get buffer is responsible for getting one
> buffer with usage_count = 0 where as we need to run the loop till it
> finds and moves enough such buffers so that it can populate freelist
> with number of buffers equal to high water mark of freelist.

Yeah, that's what I meant. Of course, it should add each buffer to
the freelist individually, not batch them up and add them all at once.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2014-08-06 15:42:08 +0530, Amit Kapila wrote:
> On Tue, Aug 5, 2014 at 9:21 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Thu, Jun 5, 2014 at 4:43 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> > This essentially removes BgWriterDelay, but it's still mentioned in
> BgBufferSync(). Looking further, I see that with the patch applied,
> BgBufferSync() is still present in the source code but is no longer called
> from anywhere. Please don't submit patches that render things unused
> without actually removing them; it makes it much harder to see what you've
> changed. I realize you probably left it that way for testing purposes, but
> you need to clean such things up before submitting. Likewise, if you've
> rendered GUCs or statistics counters removed, you need to rip them out, so
> that the scope of the changes you've made is clear to reviewers.

FWIW, I found this email amost unreadable because it misses quoting
signs after linebreaks in quoted content.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Aug 13, 2014 at 2:32 AM, Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:
> On 2014-08-06 15:42:08 +0530, Amit Kapila wrote:
> > On Tue, Aug 5, 2014 at 9:21 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
wrote:
> > > This essentially removes BgWriterDelay, but it's still mentioned in
> > BgBufferSync(). Looking further, I see that with the patch applied,
> > BgBufferSync() is still present in the source code but is no longer
called
> > from anywhere. Please don't submit patches that render things unused
> > without actually removing them; it makes it much harder to see what
you've
> > changed. I realize you probably left it that way for testing purposes,
but
> > you need to clean such things up before submitting. Likewise, if you've
> > rendered GUCs or statistics counters removed, you need to rip them out,
so
> > that the scope of the changes you've made is clear to reviewers.
>
> FWIW, I found this email amost unreadable because it misses quoting
> signs after linebreaks in quoted content.

I think I have done something wrong while replying to Robert's
mail, the main point in that mail was trying to see if there is any
major problem incase we have separate process (bgreclaim) to
populate freelist. One thing which I thought could be problematic
is to put a buf in freelist which has usage_count as zero and is *dirty*.
Please do let me know if you want clarification for something in
particular.

Overall, the main changes required in patch as per above feedback
are:
1. add an additional counter for the number of those
allocations not satisfied from the free list, with a
name like buffers_alloc_clocksweep.
2. Autotune the low and high threshold values for buffers
in freelist. In the patch, I have kept them as hard-coded
values.
3. For populating freelist, have a separate process (bgreclaim)
instead of doing it by bgwriter.

There are other things also which I need to take care as per
feedback like some change in locking strategy and code.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 2014-08-13 09:51:58 +0530, Amit Kapila wrote:
> Overall, the main changes required in patch as per above feedback
> are:
> 1. add an additional counter for the number of those
> allocations not satisfied from the free list, with a
> name like buffers_alloc_clocksweep.
> 2. Autotune the low and high threshold values for buffers
> in freelist. In the patch, I have kept them as hard-coded
> values.
> 3. For populating freelist, have a separate process (bgreclaim)
> instead of doing it by bgwriter.

I'm not convinced that 3) is the right way to go to be honest. Seems
like a huge bandaid to me.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Aug 13, 2014 at 4:25 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:
>
> On 2014-08-13 09:51:58 +0530, Amit Kapila wrote:
> > Overall, the main changes required in patch as per above feedback
> > are:
> > 1. add an additional counter for the number of those
> > allocations not satisfied from the free list, with a
> > name like buffers_alloc_clocksweep.
> > 2. Autotune the low and high threshold values for buffers
> > in freelist. In the patch, I have kept them as hard-coded
> > values.
> > 3. For populating freelist, have a separate process (bgreclaim)
> > instead of doing it by bgwriter.
>
> I'm not convinced that 3) is the right way to go to be honest. Seems
> like a huge bandaid to me.

Doing both (populating freelist and flushing dirty buffers) via bgwriter
isn't the best way either because it might not be able to perform
both the jobs as per need.
One example is it could take much longer time to flush a dirty buffer
than to move it into free list, so if there are few buffers which we need
to flush, then I think task of maintaining buffers in freelist will get hit
even though there are buffers in list which can be moved to
free list(non-dirty buffers).
Another is maintaining the current behaviour of bgwriter which is to scan
the entire buffer pool every few mins (assuming default configuration).
We can attempt to solve this problem as suggested by Robert upthread
but I am not completely sure if that can guarantee that the current
behaviour will be retained as it is.

I am not telling that having a separate process won't have any issues,
but I think we can tackle them without changing or complicating current
bgwriter behaviour.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Aug 5, 2014 at 9:21 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> Incidentally, while I generally think your changes to the locking regimen
in StrategyGetBuffer() are going in the right direction, they need
significant cleanup. Your patch adds two new spinlocks, freelist_lck and
victimbuf_lck, that mostly but not-quite replace BufFreelistLock, and
you've now got StrategyGetBuffer() running with no lock at all when
accessing some things that used to be protected by BufFreelistLock;
specifically, you're doing StrategyControl->numBufferAllocs++ and
SetLatch(StrategyControl->bgwriterLatch) without any locking. That's not
OK. I think you should get rid of BufFreelistLock completely and just
decide that freelist_lck will protect everything the freeNext links, plus
everything in StrategyControl except for nextVictimBuffer. victimbuf_lck
will protect nextVictimBuffer and nothing else.
>
> Then, in StrategyGetBuffer, acquire the freelist_lck at the point where
the LWLock is acquired today. Increment StrategyControl->numBufferAllocs;
save the values of StrategyControl->bgwriterLatch; pop a buffer off the
freelist if there is one, saving its identity. Release the spinlock.
Then, set the bgwriterLatch if needed. In the first loop, first check
whether the buffer we previously popped from the freelist is pinned or has
a non-zero usage count and return it if not, holding the buffer header
lock. Otherwise, reacquire the spinlock just long enough to pop a new
potential victim and then loop around.
>

Today, while working on updating the patch to improve locking
I found that as now we are going to have a new process, we need
a separate latch in StrategyControl to wakeup that process.
Another point is I think it will be better to protect
StrategyControl->completePasses with victimbuf_lck rather than
freelist_lck, as when we are going to update it we will already be
holding the victimbuf_lck and it doesn't make much sense to release
the victimbuf_lck and reacquire freelist_lck to update it.

I thought it is better to mention about above points so that if you have
any different thoughts about it, then it is better to discuss them now
rather than after I take performance data with this locking protocol.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> On Tue, Aug 5, 2014 at 9:21 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> I think you should get rid of BufFreelistLock completely and just
>>> decide that freelist_lck will protect everything the freeNext links, plus
>>> everything in StrategyControl except for nextVictimBuffer. victimbuf_lck
>>> will protect nextVictimBuffer and nothing else.

> Another point is I think it will be better to protect
> StrategyControl->completePasses with victimbuf_lck rather than
> freelist_lck, as when we are going to update it we will already be
> holding the victimbuf_lck and it doesn't make much sense to release
> the victimbuf_lck and reacquire freelist_lck to update it.

I'm rather concerned by this cavalier assumption that we can protect
fields a,b,c with one lock and fields x,y,z in the same struct with some
other lock.

A minimum requirement for that to work safely at all is that the fields
are of atomically fetchable/storable widths; which might be okay here
but it's a restriction that bears thinking about (and documenting).

But quite aside from safety, the fields are almost certainly going to
be in the same cache line which means contention between processes that
are trying to fetch or store them concurrently. For a patch whose sole
excuse for existence is to improve performance, that should be a very
scary concern.

(And yes, I realize these issues already affect the freelist. Perhaps
that's part of the reason we have performance issues with it.)

regards, tom lane

On Tue, Aug 26, 2014 at 8:40 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> > Another point is I think it will be better to protect
> > StrategyControl->completePasses with victimbuf_lck rather than
> > freelist_lck, as when we are going to update it we will already be
> > holding the victimbuf_lck and it doesn't make much sense to release
> > the victimbuf_lck and reacquire freelist_lck to update it.
>
> I'm rather concerned by this cavalier assumption that we can protect
> fields a,b,c with one lock and fields x,y,z in the same struct with some
> other lock.

In some cases, it could be beneficial especially when a,b,c are
going to be more frequently accessed as compare to x,y,z.

> A minimum requirement for that to work safely at all is that the fields
> are of atomically fetchable/storable widths; which might be okay here
> but it's a restriction that bears thinking about (and documenting).
>
> But quite aside from safety, the fields are almost certainly going to
> be in the same cache line which means contention between processes that
> are trying to fetch or store them concurrently.

I think patch will reduce the contention for some of such variables
(which are accessed during clock sweep) as it will minimize the need
to perform clock sweep by backends.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Aug 26, 2014 at 11:10 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>> On Tue, Aug 5, 2014 at 9:21 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>> I think you should get rid of BufFreelistLock completely and just
>>>> decide that freelist_lck will protect everything the freeNext links, plus
>>>> everything in StrategyControl except for nextVictimBuffer. victimbuf_lck
>>>> will protect nextVictimBuffer and nothing else.
>
>> Another point is I think it will be better to protect
>> StrategyControl->completePasses with victimbuf_lck rather than
>> freelist_lck, as when we are going to update it we will already be
>> holding the victimbuf_lck and it doesn't make much sense to release
>> the victimbuf_lck and reacquire freelist_lck to update it.
>
> I'm rather concerned by this cavalier assumption that we can protect
> fields a,b,c with one lock and fields x,y,z in the same struct with some
> other lock.
>
> A minimum requirement for that to work safely at all is that the fields
> are of atomically fetchable/storable widths; which might be okay here
> but it's a restriction that bears thinking about (and documenting).
>
> But quite aside from safety, the fields are almost certainly going to
> be in the same cache line which means contention between processes that
> are trying to fetch or store them concurrently. For a patch whose sole
> excuse for existence is to improve performance, that should be a very
> scary concern.
>
> (And yes, I realize these issues already affect the freelist. Perhaps
> that's part of the reason we have performance issues with it.)

False sharing is certainly a concern that has crossed my mine while
looking at Amit's work, but the performance numbers he's posted
upthread are stellar. Maybe we can squeeze some additional
performance out of this by padding out the cache lines, but it's
probably minor compared to the gains he's already seeing. I think we
should focus on trying to lock in those gains, and then we can
consider what further things we may want to do after that. If it
turns out that structure-padding is among those things, that's easy
enough to do as a separate patch.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Aug 26, 2014 at 10:53 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> Today, while working on updating the patch to improve locking
> I found that as now we are going to have a new process, we need
> a separate latch in StrategyControl to wakeup that process.
> Another point is I think it will be better to protect
> StrategyControl->completePasses with victimbuf_lck rather than
> freelist_lck, as when we are going to update it we will already be
> holding the victimbuf_lck and it doesn't make much sense to release
> the victimbuf_lck and reacquire freelist_lck to update it.

Sounds reasonable. I think the key thing at this point is to get a
new version of the patch with the background reclaim running in a
different process than the background writer. I don't see much point
in fine-tuning the locking regimen until that's done.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Aug 27, 2014 at 8:34 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, Aug 26, 2014 at 10:53 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> > Today, while working on updating the patch to improve locking
> > I found that as now we are going to have a new process, we need
> > a separate latch in StrategyControl to wakeup that process.
> > Another point is I think it will be better to protect
> > StrategyControl->completePasses with victimbuf_lck rather than
> > freelist_lck, as when we are going to update it we will already be
> > holding the victimbuf_lck and it doesn't make much sense to release
> > the victimbuf_lck and reacquire freelist_lck to update it.
>
> Sounds reasonable. I think the key thing at this point is to get a
> new version of the patch with the background reclaim running in a
> different process than the background writer. I don't see much point
> in fine-tuning the locking regimen until that's done.
>
>
I have updated the patch to address the feedback. Main changes are:

1. For populating freelist, have a separate process (bgreclaimer)
instead of doing it by bgwriter.
2. Autotune the low and high threshold values for buffers
in freelist. I have used the formula as suggested by you upthread.
3. Cleanup of locking regimen as discussed upthread (completely
eliminated BufFreelist Lock).
4. Improved comments and general code cleanup.

I have not yet added statistics (buffers_backend_clocksweep) as
for that we need to add one more variable in BufferStrategyControl
structure where I have already added few variables for this patch.
I think it is important to have such a stat available via
pg_stat_bgwriter, but not sure if it is worth to make the structure
bit more bulky.

Another minor point is about changes in lwlock.h
lwlock.h
* if you remove a lock, consider leaving a gap in the numbering
* sequence for the benefit of DTrace and other external debugging
* scripts.

As I have removed BufFreelist lock, I have adjusted the numbering
as well in lwlock.h. There is a meesage on top of lock definitions
which suggest to leave gap if we remove any lock, however I was not
sure whether this case (removing the first element) can effect anything,
so for now, I have adjusted the numbering.

I have yet to collect data under varying loads, however I have
collected performance data for 8GB shared buffers which shows
reasonably good performance and scalability.

I think the main part left for this patch is more data for various loads
which I will share in next few days, however I think patch is ready for
next round of review, so I will mark it as Needs Review.

Performance Data:
-------------------------------

Configuration and Db Details

IBM POWER-7 16 cores, 64 hardware threads

RAM = 64GB

Database Locale =C

checkpoint_segments=256

checkpoint_timeout =15min

shared_buffers=8GB

scale factor = 3000

Client Count = number of concurrent sessions and threads (ex. -c 8 -j 8)

Duration of each individual run = 5mins

All the data is in tps and taken using pgbench read-only load

Client Count/Patch_ver 8 16 32 64 128 HEAD 58614 107370 140717 104357
65010 Patch 60849 118701 165631 209226 213029

Note -
a. The numbers are slightly different than previously reported
numbers as earlier I was using debug mode of binaries to take
data and it seems some kind of trace was enabled on m/c.
However the improve in performance and scalability is almost
similar to previous.
b. Above data is median of 3 runs, for detailed data refer attached
document (perf_read_scalability_data_v5.ods)

CPU Usage
------------------
I have observed that CPU usage for new process (reclaimer) is
between 5~9%.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Aug 28, 2014 at 7:11 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> I have updated the patch to address the feedback. Main changes are:
>
> 1. For populating freelist, have a separate process (bgreclaimer)
> instead of doing it by bgwriter.
> 2. Autotune the low and high threshold values for buffers
> in freelist. I have used the formula as suggested by you upthread.
> 3. Cleanup of locking regimen as discussed upthread (completely
> eliminated BufFreelist Lock).
> 4. Improved comments and general code cleanup.

Overall this looks quite promising to me.

I had thought to call the new process just "bgreclaim" rather than
"bgreclaimer", but perhaps your name is better after all. At least,
it matches what we do elsewhere. But I don't care for the use
"Bgreclaimer"; let's do "BgReclaimer" if we really need mixed-case, or
else "bgreclaimer".

This is unclear:

+buffers for replacement. Earlier to protect freelist, we use LWLOCK as that
+is needed to perform clock sweep which is a longer operation, however now we
+are using two spinklocks freelist_lck and victimbuf_lck to perform operations
+on freelist and run clock sweep respectively.

I would drop the discussion of what was done before and say something
like this: The data structures relating to buffer eviction are
protected by two spinlocks. freelist_lck protects the freelist and
related data structures, while victimbuf_lck protects information
related to the current clock sweep condition.

+always in this list. We also throw buffers into this list if we consider
+their pages unlikely to be needed soon; this is done by background process
+reclaimer. The list is singly-linked using fields in the

I suggest: Allocating pages from this list is much cheaper than
running the "clock sweep" algorithm, which may encounter many buffers
that are poor candidates for eviction before finding a good candidate.
Therefore, we have a background process called bgreclaimer which works
to keep this list populated.

+Background Reclaimer's Processing
+---------------------------------

I suggest titling this section "Background Reclaim".

+The background reclaimer is designed to move buffers to freelist that are

I suggest replacing the first three words of this sentence with "bgreclaimer".

+and move the the unpinned and zero usage count buffers to freelist. It
+keep on doing this until the number of buffers in freelist become equal
+high threshold of freelist.

s/keep/keeps/
s/become equal/reaches the/
s/high threshold/high water mark/
s/of freelist//

Please change the other places that say threshold to use the "water
mark" terminology.

+ if (StrategyMoveBufferToFreeListEnd (bufHdr))

Extra space.

+ * buffers in consecutive cycles.

s/consecutive/later/

+ /* Execute the LRU scan */

s/LRU scan/clock sweep/ ?

+ while (tmp_num_to_free > 0)

I am not sure it's a good idea for this value to be fixed at loop
start and then just decremented. Shouldn't we loop and do the whole
thing over once we reach the high watermark, only stopping when
StrategySyncStartAndEnd() says num_to_free is 0?

+ /* choose next victim buffer to clean. */

This process doesn't clean buffers; it puts them on the freelist.

+ * high threshold of freelsit), we drasticaly reduce the odds for

Two typos.

+ * of buffers in freelist fall below low threshold of freelist.

s/fall/falls/

In freelist.c, it seems like a poor idea to have two spinlocks as
consecutive structure members; they'll be in the same cache line,
leading to false sharing. If we merge them into a single spinlock,
does that hurt performance? If we put them further apart, e.g. by
moving the freelist_lck to the start of the structure, followed by the
latches, and leaving victimbuf_lck where it is, does that help
performance?

+ /*
+ * If the buffer is pinned or has a nonzero usage_count,
we cannot use
+ * it; discard it and retry. (This can only happen if VACUUM put a
+ * valid buffer in the freelist and then someone else
used it before
+ * we got to it. It's probably impossible altogether as
of 8.3, but
+ * we'd better check anyway.)
+ */
+

This comment is clearly obsolete.

> I have not yet added statistics (buffers_backend_clocksweep) as
> for that we need to add one more variable in BufferStrategyControl
> structure where I have already added few variables for this patch.
> I think it is important to have such a stat available via
> pg_stat_bgwriter, but not sure if it is worth to make the structure
> bit more bulky.

I think it's worth it.

> Another minor point is about changes in lwlock.h
> lwlock.h
> * if you remove a lock, consider leaving a gap in the numbering
> * sequence for the benefit of DTrace and other external debugging
> * scripts.
>
> As I have removed BufFreelist lock, I have adjusted the numbering
> as well in lwlock.h. There is a meesage on top of lock definitions
> which suggest to leave gap if we remove any lock, however I was not
> sure whether this case (removing the first element) can effect anything,
> so for now, I have adjusted the numbering.

Let's leave slot 0 unused, instead.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Aug 28, 2014 at 4:41 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
> I have yet to collect data under varying loads, however I have
> collected performance data for 8GB shared buffers which shows
> reasonably good performance and scalability.
>
> I think the main part left for this patch is more data for various loads
> which I will share in next few days, however I think patch is ready for
> next round of review, so I will mark it as Needs Review.

I have collected more data with the patch. I understand that you
have given more review comments due to which patch require
changes, however I think it will not effect the performance data
to a great extent and I have anyway taken the data, so sharing the
same.

> Performance Data:
> -------------------------------
>
> Configuration and Db Details
> IBM POWER-7 16 cores, 64 hardware threads
> RAM = 64GB
> Database Locale =C
> checkpoint_segments=256
> checkpoint_timeout =15min
> scale factor = 3000
> Client Count = number of concurrent sessions and threads (ex. -c 8 -j 8)
> Duration of each individual run = 5mins
>
> All the data is in tps and taken using pgbench read-only load

Common configuration remains same as above.

Shared_Buffers = 500MB
Client Count/Patch_Ver 8 16 32 64 128 HEAD 56248 100112 121341 81128
56552 Patch 59389 112483 157034 185740 166725

Shared_Buffers = 1GB
Client Count/Patch_Ver 8 16 32 64 128 HEAD 56401 102557 121643 81686
57091 Patch 59361 114813 157528 188209 167752

Shared_Buffers = 14GB
Client Count/Patch_Ver 8 16 32 64 128 HEAD 60059 110582 152051 130718
97014 Patch 61462 117377 169767 225803 229083

Shared_Buffers = 15GB
Client Count/Patch_Ver 8 16 32 64 128 HEAD 60005 112928 153650 135203
36343 Patch 61345 115569 168767 226150 36985

Observations
---------------------
1. Performance improvement is upto 2~3 times for higher client
counts (64, 128).
2. For lower client count (8), we can see 2~5 % performance
improvement.
3. Overall, this improves the read scalability.
4. For lower number of shared buffers, we see that there is a minor
dip in tps even after patch (it might be that we can improve it by
tuning higher water mark for the number of buffers on freelist, I will
try this by varying high water mark).
5. For larger shared buffers (15GB), we can see that there is still a
dip at large client count, although situation is not bad as compare to
HEAD. The reason is that at such high shared buffers and client count,
I/O starts happening because all the data can't be contained in RAM.

I will try to take some data for tpc-b load as well. Kindly let me know
if you want to see data for some other configuration.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Sep 3, 2014 at 9:45 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Aug 28, 2014 at 4:41 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> >
> > I have yet to collect data under varying loads, however I have
> > collected performance data for 8GB shared buffers which shows
> > reasonably good performance and scalability.
> >
> > I think the main part left for this patch is more data for various loads
> > which I will share in next few days, however I think patch is ready for
> > next round of review, so I will mark it as Needs Review.
>
> I have collected more data with the patch. I understand that you
> have given more review comments due to which patch require
> changes, however I think it will not effect the performance data
> to a great extent and I have anyway taken the data, so sharing the
> same.
>
>
> > Performance Data:
> > -------------------------------
> >
> > Configuration and Db Details
> > IBM POWER-7 16 cores, 64 hardware threads
> > RAM = 64GB
> > Database Locale =C
> > checkpoint_segments=256
> > checkpoint_timeout =15min
> > scale factor = 3000
> > Client Count = number of concurrent sessions and threads (ex. -c 8 -j 8)
> > Duration of each individual run = 5mins
> >
> > All the data is in tps and taken using pgbench read-only load
>
> Common configuration remains same as above.

Forgot to mention that data is a median of 3 runs and attached
sheet contains data for individual runs.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Sep 3, 2014 at 1:45 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Aug 28, 2014 at 7:11 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > I have updated the patch to address the feedback. Main changes are:
> >
> > 1. For populating freelist, have a separate process (bgreclaimer)
> > instead of doing it by bgwriter.
> > 2. Autotune the low and high threshold values for buffers
> > in freelist. I have used the formula as suggested by you upthread.
> > 3. Cleanup of locking regimen as discussed upthread (completely
> > eliminated BufFreelist Lock).
> > 4. Improved comments and general code cleanup.
>
> +Background Reclaimer's Processing
> +---------------------------------
>
> I suggest titling this section "Background Reclaim".

I don't mind changing it, but currently used title is based on similar
title "Background Writer's Processing". It is used in previous
paragraph. Is there a reason to title this differently?

> +The background reclaimer is designed to move buffers to freelist that are
>
> I suggest replacing the first three words of this sentence with
"bgreclaimer".

Again what I have used is matching with BgWriter's explanation. I thought
it would be better if wording used is similar.

>
> + while (tmp_num_to_free > 0)
>
> I am not sure it's a good idea for this value to be fixed at loop
> start and then just decremented.

It is based on the idea what bgwriter does for num_to_scan and
calling it once has advantage that we need to take freelist_lck
just once.

> Shouldn't we loop and do the whole
> thing over once we reach the high watermark, only stopping when
> StrategySyncStartAndEnd() says num_to_free is 0?

Do you mean to say that for high water mark check, we should
always refer StrategySyncStartAndEnd() rather than getting the
value in begining?

Are you thinking that somebody else would have already put some
buffers onto freelist which would change initially identified high water
mark? I think that can be done only during very few and less used
operations. Do you think for that we start referring
StrategySyncStartAndEnd() in loop?
>
> In freelist.c, it seems like a poor idea to have two spinlocks as
> consecutive structure members; they'll be in the same cache line,
> leading to false sharing. If we merge them into a single spinlock,
> does that hurt performance?

I have kept them separate so that backends searching for a buffer
in freelist doesn't contend with bgreclaimer (while doing clock sweep)
or clock sweep being done by other backends. I think it will be bit
tricky to devise a test where this can hurt, however it doesn't seem
too bad to have two separate locks in this case.

> If we put them further apart, e.g. by
> moving the freelist_lck to the start of the structure, followed by the
> latches, and leaving victimbuf_lck where it is, does that help
> performance?

I can investigate.

> + /*
> + * If the buffer is pinned or has a nonzero usage_count,
> we cannot use
> + * it; discard it and retry. (This can only happen if
VACUUM put a
> + * valid buffer in the freelist and then someone else
> used it before
> + * we got to it. It's probably impossible altogether as
> of 8.3, but
> + * we'd better check anyway.)
> + */
> +
>
> This comment is clearly obsolete.

Okay, but this patch hasn't changed anything w.r.t above comment,
so I haven't changed it. Do you want me to remove second part of
comment starting with "(This can only happen"?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Sep 3, 2014 at 7:27 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> +Background Reclaimer's Processing
>> +---------------------------------
>>
>> I suggest titling this section "Background Reclaim".
>
> I don't mind changing it, but currently used title is based on similar
> title "Background Writer's Processing". It is used in previous
> paragraph. Is there a reason to title this differently?

Oh, I didn't see that. Seems like weird phrasing to me, but I guess
it's probably better to keep it consistent.

>> +The background reclaimer is designed to move buffers to freelist that are
>>
>> I suggest replacing the first three words of this sentence with
>> "bgreclaimer".
>
> Again what I have used is matching with BgWriter's explanation. I thought
> it would be better if wording used is similar.

OK.

>> + while (tmp_num_to_free > 0)
>>
>> I am not sure it's a good idea for this value to be fixed at loop
>> start and then just decremented.
>
> It is based on the idea what bgwriter does for num_to_scan and
> calling it once has advantage that we need to take freelist_lck
> just once.

Right, we shouldn't call it every loop iteration. However, consider
this scenario: there are no remaining buffers on the list and the high
watermark is 2000. We add 2000 buffers to the list. But by the time
we get done, other backends have already done 500 more allocations, so
now there are only 1500 buffers on the list. If this should occur, we
should add an additional 500 buffers to the list before we consider
sleeping. We want bgreclaimer to be able to run continuously if the
demand for buffers is high enough.

>> In freelist.c, it seems like a poor idea to have two spinlocks as
>> consecutive structure members; they'll be in the same cache line,
>> leading to false sharing. If we merge them into a single spinlock,
>> does that hurt performance?
>
> I have kept them separate so that backends searching for a buffer
> in freelist doesn't contend with bgreclaimer (while doing clock sweep)
> or clock sweep being done by other backends. I think it will be bit
> tricky to devise a test where this can hurt, however it doesn't seem
> too bad to have two separate locks in this case.

It's not. But if they are in the same cache line, they will behave
almost like one lock, because the CPU will lock the entire cache line
for each atomic op. See Tom's comments upthread.

> Okay, but this patch hasn't changed anything w.r.t above comment,
> so I haven't changed it. Do you want me to remove second part of
> comment starting with "(This can only happen"?

Right. Clearly it can happen again once we have this patch: that's
the entire point of the patch.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 03/09/14 16:22, Amit Kapila wrote:
> On Wed, Sep 3, 2014 at 9:45 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>
>> On Thu, Aug 28, 2014 at 4:41 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>>>
>>> I have yet to collect data under varying loads, however I have
>>> collected performance data for 8GB shared buffers which shows
>>> reasonably good performance and scalability.
>>>
>>> I think the main part left for this patch is more data for various loads
>>> which I will share in next few days, however I think patch is ready for
>>> next round of review, so I will mark it as Needs Review.
>>
>> I have collected more data with the patch. I understand that you
>> have given more review comments due to which patch require
>> changes, however I think it will not effect the performance data
>> to a great extent and I have anyway taken the data, so sharing the
>> same.
>>
>>
>>> Performance Data:
>>> -------------------------------
>>>
>>> Configuration and Db Details
>>> IBM POWER-7 16 cores, 64 hardware threads
>>> RAM = 64GB
>>> Database Locale =C
>>> checkpoint_segments=256
>>> checkpoint_timeout =15min
>>> scale factor = 3000
>>> Client Count = number of concurrent sessions and threads (ex. -c 8 -j 8)
>>> Duration of each individual run = 5mins
>>>
>>> All the data is in tps and taken using pgbench read-only load
>>
>> Common configuration remains same as above.
>
> Forgot to mention that data is a median of 3 runs and attached
> sheet contains data for individual runs.
>
>

Hi Amit,

Results look pretty good. Does it help in the read-write case too?

Cheers

Mark

On Thu, Sep 4, 2014 at 8:00 AM, Mark Kirkwood <mark(dot)kirkwood(at)catalyst(dot)net(dot)nz>
wrote:
>
>
> Hi Amit,
>
> Results look pretty good. Does it help in the read-write case too?

Last time I ran the tpc-b test of pgbench (results of which are
posted earlier in this thread), there doesn't seem to be any major
gain for that, however for cases where read is predominant, you
might see better gains.

I am again planing to take that data in next few days.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Sep 3, 2014 at 8:03 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Sep 3, 2014 at 7:27 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
> >> + while (tmp_num_to_free > 0)
> >>
> >> I am not sure it's a good idea for this value to be fixed at loop
> >> start and then just decremented.
> >
> > It is based on the idea what bgwriter does for num_to_scan and
> > calling it once has advantage that we need to take freelist_lck
> > just once.
>
> Right, we shouldn't call it every loop iteration. However, consider
> this scenario: there are no remaining buffers on the list and the high
> watermark is 2000. We add 2000 buffers to the list. But by the time
> we get done, other backends have already done 500 more allocations, so
> now there are only 1500 buffers on the list. If this should occur, we
> should add an additional 500 buffers to the list before we consider
> sleeping. We want bgreclaimer to be able to run continuously if the
> demand for buffers is high enough.

Its not difficult to handle such cases, but it can have downside also
for the cases where demand from backends is not high.
Consider in above case if instead of 500 more allocations, it just
does 5 more allocations, then bgreclaimer will again have to go through
the list and move 5 buffers and same can happen again by the time
it moves 5 buffers. Another point to keep in mind here is that in this
loop we are reducing the usage_count of buffers as well incase we don't
find buffer with usage_count=0. OTOH if we let bgreclaimer to go for
sleep after it moves initially identified buffers, then the backend which
first finds that the buffers in freelist falls below low water mark can
wake bgreclaimer.

> >> In freelist.c, it seems like a poor idea to have two spinlocks as
> >> consecutive structure members; they'll be in the same cache line,
> >> leading to false sharing. If we merge them into a single spinlock,
> >> does that hurt performance?
> >
> > I have kept them separate so that backends searching for a buffer
> > in freelist doesn't contend with bgreclaimer (while doing clock sweep)
> > or clock sweep being done by other backends. I think it will be bit
> > tricky to devise a test where this can hurt, however it doesn't seem
> > too bad to have two separate locks in this case.
>
> It's not. But if they are in the same cache line, they will behave
> almost like one lock, because the CPU will lock the entire cache line
> for each atomic op. See Tom's comments upthread.

I think to avoid having them in same cache line, we might need to
add some padding (at least 72 bytes) as the structure size including both
the spin locks is 56 bytes on PPC64 m/c and cache line size is 128 bytes.
I have taken performance data as well by keeping them further apart
as suggested by you upthread and by introducing padding, but the
difference in performance is less than 1.5% (on 64 and 128 client count)
which also might be due to variation of data across runs. So now to
proceed we have below options:

a. use two spinlocks as in patch, but keep them as far apart as possible.
This might not have an advantage as compare to what is used currently
in patch, but in future we can adding padding to take the advantage if
possible (currently on PPC64, it doesn't show any noticeable advantage,
however on some other m/c, it might show the advantage).

b. use only one spinlock, this can have disadvantage in certain cases
as mentioned upthread, however those might not be usual cases, so for
now we can consider them as lower priority and can choose this option.

Another point in this regard is that I have to make use of volatile
pointer to prevent code rearrangement in this case.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Sep 3, 2014 at 9:45 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

>
> > Performance Data:
> > -------------------------------
> >
> > Configuration and Db Details
> > IBM POWER-7 16 cores, 64 hardware threads
> > RAM = 64GB
> > Database Locale =C
> > checkpoint_segments=256
> > checkpoint_timeout =15min
> > scale factor = 3000
> > Client Count = number of concurrent sessions and threads (ex. -c 8 -j 8)
> > Duration of each individual run = 5mins
> >
> > All the data is in tps and taken using pgbench read-only load
>
> Common configuration remains same as above.
>
> Shared_Buffers = 500MB
> Client Count/Patch_Ver 8 16 32 64 128 HEAD 56248 100112 121341 81128
> 56552 Patch 59389 112483 157034 185740 166725
> ..
>

> Observations
> ---------------------
> 1. Performance improvement is upto 2~3 times for higher client
> counts (64, 128).
> 2. For lower client count (8), we can see 2~5 % performance
> improvement.
> 3. Overall, this improves the read scalability.
> 4. For lower number of shared buffers, we see that there is a minor
> dip in tps even after patch (it might be that we can improve it by
> tuning higher water mark for the number of buffers on freelist, I will
> try this by varying high water mark).
>

I have taken performance data by varying high and low mater marks
for lower value of shared buffers which is as below:

Shared_buffers = 500MB
Scale_factor = 3000

HM - High water mark, 0.5 means 0.5% of total shared buffers
LM - Low water mark, 20 means 20% of HM.

Client Count/Patch_Ver (Data in tps) 128 HM=0.5;LM=20 166725 HM=1;LM=20
166556 HM=2;LM=30 166463 HM=5;LM=30 166107 HM=10;LM=30 167231

Observation
--------------------
a. There is hardly any difference by varying High and Low water marks
as compared to default values currently used in patch.
b. I think this minor dip as compare to 64 client count is because one
this m/c has 64 hardware threads due which scaling beyond 64 client
count is difficult and second at relatively lower buffer count (500MB),
there is still minor contention around Buf Mapping locks.

In general, I think with patch the scaling is much better (2 times) than
HEAD, even when shared buffers are less and client count is high,
so this is not an issue.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Sep 4, 2014 at 7:25 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> Its not difficult to handle such cases, but it can have downside also
> for the cases where demand from backends is not high.
> Consider in above case if instead of 500 more allocations, it just
> does 5 more allocations, then bgreclaimer will again have to go through
> the list and move 5 buffers and same can happen again by the time
> it moves 5 buffers.

That's exactly the scenario in which we *want* the looping behavior.
If that's happening, then it means it's taking us exactly as long to
find 5 buffers as it takes the rest of the system to use 5 buffers.
We need to run continuously to keep up.

>> It's not. But if they are in the same cache line, they will behave
>> almost like one lock, because the CPU will lock the entire cache line
>> for each atomic op. See Tom's comments upthread.
>
> I think to avoid having them in same cache line, we might need to
> add some padding (at least 72 bytes) as the structure size including both
> the spin locks is 56 bytes on PPC64 m/c and cache line size is 128 bytes.
> I have taken performance data as well by keeping them further apart
> as suggested by you upthread and by introducing padding, but the
> difference in performance is less than 1.5% (on 64 and 128 client count)
> which also might be due to variation of data across runs. So now to
> proceed we have below options:
>
> a. use two spinlocks as in patch, but keep them as far apart as possible.
> This might not have an advantage as compare to what is used currently
> in patch, but in future we can adding padding to take the advantage if
> possible (currently on PPC64, it doesn't show any noticeable advantage,
> however on some other m/c, it might show the advantage).
>
> b. use only one spinlock, this can have disadvantage in certain cases
> as mentioned upthread, however those might not be usual cases, so for
> now we can consider them as lower priority and can choose this option.

I guess I don't care that much. I only mentioned it because Tom
brought it up; I don't really see a big problem with the way you're
doing it.

> Another point in this regard is that I have to make use of volatile
> pointer to prevent code rearrangement in this case.

Yep. Or we need to get off our duff and fix it so that's not necessary.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Sep 4, 2014 at 7:25 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> Its not difficult to handle such cases, but it can have downside also
>> for the cases where demand from backends is not high.
>> Consider in above case if instead of 500 more allocations, it just
>> does 5 more allocations, then bgreclaimer will again have to go through
>> the list and move 5 buffers and same can happen again by the time
>> it moves 5 buffers.
>
> That's exactly the scenario in which we *want* the looping behavior.
> If that's happening, then it means it's taking us exactly as long to
> find 5 buffers as it takes the rest of the system to use 5 buffers.
> We need to run continuously to keep up.

That's what I was thinking, as long as there isn't a lot of
overhead to starting and finishing a cycle. If there is, my
inclination would be to try to fix that rather than to sleep and
hope things don't get out of hand before it wakes up again.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Wed, Sep 3, 2014 at 7:27 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >> +Background Reclaimer's Processing
> >> +---------------------------------
> >>
> >> I suggest titling this section "Background Reclaim".
> >
> > I don't mind changing it, but currently used title is based on similar
> > title "Background Writer's Processing". It is used in previous
> > paragraph. Is there a reason to title this differently?
>
> Oh, I didn't see that. Seems like weird phrasing to me, but I guess
> it's probably better to keep it consistent.

... or you can also change the other one.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 04/09/14 14:42, Amit Kapila wrote:
> On Thu, Sep 4, 2014 at 8:00 AM, Mark Kirkwood <mark(dot)kirkwood(at)catalyst(dot)net(dot)nz>
> wrote:
>>
>>
>> Hi Amit,
>>
>> Results look pretty good. Does it help in the read-write case too?
>
> Last time I ran the tpc-b test of pgbench (results of which are
> posted earlier in this thread), there doesn't seem to be any major
> gain for that, however for cases where read is predominant, you
> might see better gains.
>
> I am again planing to take that data in next few days.
>

FWIW below are some test results on the 60 core beast with this patch
applied to 9.4. I'll need to do more runs to iron out the variation, but
it looks like the patch helps the standard (write heavy) pgbench
workload a little, and clearly helps the read only case.

4x E7-4890 15 cores each.
1 TB ram
16x Toshiba PX02SS SATA SSD
4x Samsung NVMe XS1715 PCIe SSD

Ubuntu 14.04 (Linux 3.13)
Postgres 9.4 beta2
+ buffer eviction patch v5

Pgbench

scale 2000

Non default params:

max_connections = 400;
shared_buffers = "10GB";
maintenance_work_mem = "1GB";
effective_io_concurrency = 10;
wal_buffers = "256MB";
checkpoint_segments = 1920;
checkpoint_completion_target = 0.8;
ssl = 'off';
wal_sync_method = 'open_datasync';

read write

elapsed 600s

Clients | tps | tps (unpatched)
---------+-------+----------------
6 | 8279 | 8328
12 | 16260 | 16381
24 | 23639 | 23451
48 | 31430 | 31004
96 | 38516 | 34777
192 | 33535 | 32443
384 | 27978 | 25068
384 | 30589 | 28798

read only

elapsed 300s

Clients | tps | tps (unpatched)
---------+--------+----------------
6 | 57654 | 57255
12 | 111361 | 112360
24 | 220304 | 187967
48 | 384567 | 230961
96 | 380309 | 241947
192 | 330865 | 214570
384 | 315516 | 207548

Regards

Mark

On Wed, Sep 3, 2014 at 1:45 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Thu, Aug 28, 2014 at 7:11 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > I have updated the patch to address the feedback. Main changes are:
> >
> > 1. For populating freelist, have a separate process (bgreclaimer)
> > instead of doing it by bgwriter.
> > 2. Autotune the low and high threshold values for buffers
> > in freelist. I have used the formula as suggested by you upthread.
> > 3. Cleanup of locking regimen as discussed upthread (completely
> > eliminated BufFreelist Lock).
> > 4. Improved comments and general code cleanup.
>
> Overall this looks quite promising to me.
>
> I had thought to call the new process just "bgreclaim" rather than
> "bgreclaimer", but perhaps your name is better after all. At least,
> it matches what we do elsewhere. But I don't care for the use
> "Bgreclaimer"; let's do "BgReclaimer" if we really need mixed-case, or
> else "bgreclaimer".

Changed it to bgreclaimer.

> This is unclear:
>
> +buffers for replacement. Earlier to protect freelist, we use LWLOCK as
that
> +is needed to perform clock sweep which is a longer operation, however
now we
> +are using two spinklocks freelist_lck and victimbuf_lck to perform
operations
> +on freelist and run clock sweep respectively.
>
> I would drop the discussion of what was done before and say something
> like this: The data structures relating to buffer eviction are
> protected by two spinlocks. freelist_lck protects the freelist and
> related data structures, while victimbuf_lck protects information
> related to the current clock sweep condition.

Changed, but I have not used exact wording mentioned above, let me know
if new wording used is okay?

> +always in this list. We also throw buffers into this list if we consider
> +their pages unlikely to be needed soon; this is done by background
process
> +reclaimer. The list is singly-linked using fields in the
>
> I suggest: Allocating pages from this list is much cheaper than
> running the "clock sweep" algorithm, which may encounter many buffers
> that are poor candidates for eviction before finding a good candidate.
> Therefore, we have a background process called bgreclaimer which works
> to keep this list populated.

Changed as per your suggestion.

> +Background Reclaimer's Processing
> +---------------------------------
>
> I suggest titling this section "Background Reclaim".
>
> +The background reclaimer is designed to move buffers to freelist that are
>
> I suggest replacing the first three words of this sentence with
"bgreclaimer".

As per discussion in thread, I have kept it as it is.

> +and move the the unpinned and zero usage count buffers to freelist. It
> +keep on doing this until the number of buffers in freelist become equal
> +high threshold of freelist.
>
> s/keep/keeps/
> s/become equal/reaches the/
> s/high threshold/high water mark/
> s/of freelist//

Changed as per your suggestion.

> Please change the other places that say threshold to use the "water
> mark" terminology.
>
> + if (StrategyMoveBufferToFreeListEnd (bufHdr))
>
> Extra space.
>
> + * buffers in consecutive cycles.
>
> s/consecutive/later/
>
> + /* Execute the LRU scan */
>
> s/LRU scan/clock sweep/ ?

Changed as per your suggestion.

>
> + while (tmp_num_to_free > 0)
>
> I am not sure it's a good idea for this value to be fixed at loop
> start and then just decremented. Shouldn't we loop and do the whole
> thing over once we reach the high watermark, only stopping when
> StrategySyncStartAndEnd() says num_to_free is 0?

Okay, changed the loop as per your suggestion.

> + /* choose next victim buffer to clean. */
>
> This process doesn't clean buffers; it puts them on the freelist.

Right. Changed it to match what it does.

> + * high threshold of freelsit), we drasticaly reduce the odds for
>
> Two typos.

Fixed.

> + * of buffers in freelist fall below low threshold of freelist.
>
> s/fall/falls/

Changed as per your suggestion.

> In freelist.c, it seems like a poor idea to have two spinlocks as
> consecutive structure members; they'll be in the same cache line,
> leading to false sharing. If we merge them into a single spinlock,
> does that hurt performance? If we put them further apart, e.g. by
> moving the freelist_lck to the start of the structure, followed by the
> latches, and leaving victimbuf_lck where it is, does that help
> performance?

As per discussion, I have kept them as it is and added a comment
indicating that we can consider having both locks in separate
cache lines.

> + /*
> + * If the buffer is pinned or has a nonzero usage_count,
> we cannot use
> + * it; discard it and retry. (This can only happen if
VACUUM put a
> + * valid buffer in the freelist and then someone else
> used it before
> + * we got to it. It's probably impossible altogether as
> of 8.3, but
> + * we'd better check anyway.)
> + */
> +
>
> This comment is clearly obsolete.

Removed.

> > I have not yet added statistics (buffers_backend_clocksweep) as
> > for that we need to add one more variable in BufferStrategyControl
> > structure where I have already added few variables for this patch.
> > I think it is important to have such a stat available via
> > pg_stat_bgwriter, but not sure if it is worth to make the structure
> > bit more bulky.
>
> I think it's worth it.

Okay added new statistic.

> > Another minor point is about changes in lwlock.h
> > lwlock.h
> > * if you remove a lock, consider leaving a gap in the numbering
> > * sequence for the benefit of DTrace and other external debugging
> > * scripts.
> >
> > As I have removed BufFreelist lock, I have adjusted the numbering
> > as well in lwlock.h. There is a meesage on top of lock definitions
> > which suggest to leave gap if we remove any lock, however I was not
> > sure whether this case (removing the first element) can effect anything,
> > so for now, I have adjusted the numbering.
>
> Let's leave slot 0 unused, instead.

Sure, that make sense.

Apart from above, I think for this patch, cat version bump is required
as I have modified system catalog. However I have not done the
same in patch as otherwise it will be bit difficult to take performance
data.

Performance Data with updated patch

Configuration and Db Details

IBM POWER-7 16 cores, 64 hardware threads
RAM = 64GB
Database Locale =C
checkpoint_segments=256
checkpoint_timeout =15min
shared_buffers=8GB
scale factor = 3000
Client Count = number of concurrent sessions and threads (ex. -c 8 -j 8)
Duration of each individual run = 5mins

All the data is in tps and taken using pgbench read-only load

Client Count/Patch_Ver (tps) 8 16 32 64 128 HEAD 58614 107370 140717
104357 65010 Patch 60092 113564 165014 213848 216065

This data is median of 3 runs, detailed report is attached with mail.
I have not repeated the test for all configurations, as there is no
major change in design/algorithm which can effect performance.
Mark has already taken tpc-b data which ensures that there is
no problem with it, however I will also take it once with latest version.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Sep 5, 2014 at 8:42 AM, Mark Kirkwood <mark(dot)kirkwood(at)catalyst(dot)net(dot)nz>
wrote:
>
> On 04/09/14 14:42, Amit Kapila wrote:
>>
>> On Thu, Sep 4, 2014 at 8:00 AM, Mark Kirkwood <
mark(dot)kirkwood(at)catalyst(dot)net(dot)nz>
>> wrote:
>>>
>>>
>>>
>>> Hi Amit,
>>>
>>> Results look pretty good. Does it help in the read-write case too?
>>
>>
>> Last time I ran the tpc-b test of pgbench (results of which are
>> posted earlier in this thread), there doesn't seem to be any major
>> gain for that, however for cases where read is predominant, you
>> might see better gains.
>>
>> I am again planing to take that data in next few days.
>>
>
> FWIW below are some test results on the 60 core beast with this patch
applied to 9.4. I'll need to do more runs to iron out the variation,
> but it looks like the patch helps the standard (write heavy) pgbench
workload a little, and clearly helps the read only case.
>

Thanks for doing the test. I think if possible you can take
the performance data with higher scale factor (4000) as it
seems your m/c has 1TB of RAM. You might want to use
latest patch I have posted today.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Sep 5, 2014 at 5:17 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> Apart from above, I think for this patch, cat version bump is required
> as I have modified system catalog. However I have not done the
> same in patch as otherwise it will be bit difficult to take performance
> data.

One regression failed on linux due to spacing issue which is
fixed in attached patch.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Sep 5, 2014 at 6:47 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> Client Count/Patch_Ver (tps) 8 16 32 64 128
> HEAD 58614 107370 140717 104357 65010
> Patch 60092 113564 165014 213848 216065
>
> This data is median of 3 runs, detailed report is attached with mail.
> I have not repeated the test for all configurations, as there is no
> major change in design/algorithm which can effect performance.
> Mark has already taken tpc-b data which ensures that there is
> no problem with it, however I will also take it once with latest version.

Well, these numbers are pretty much amazing. Question: It seems
there's obviously quite a bit of contention left; do you think
there's still a significant amount of time in the clock sweep, or is
the primary bottleneck the buffer mapping locks?

merlin

On Fri, Sep 5, 2014 at 9:19 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Sep 5, 2014 at 5:17 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> Apart from above, I think for this patch, cat version bump is required
>> as I have modified system catalog. However I have not done the
>> same in patch as otherwise it will be bit difficult to take performance
>> data.
>
> One regression failed on linux due to spacing issue which is
> fixed in attached patch.

I took another read through this patch. Here are some further review comments:

1. In BgMoveBuffersToFreelist(), it seems quite superfluous to have
both num_to_free and tmp_num_to_free. I'd get rid of tmp_num_to_free
and move the declaration of num_to_free inside the outer loop. I'd
also move the definitions of tmp_next_to_clean, tmp_recent_alloc,
tmp_recent_backend_clocksweep into the innermost scope in which they
are used.

2. Also in that function, I think the innermost bit of logic could be
rewritten more compactly, and in such a way as to make it clearer for
what set of instructions the buffer header will be locked.
LockBufHdr(bufHdr); if (bufHdr->refcount == 0) { if
(bufHdr->usage_count > 0) bufHdr->usage_count--; else add_to_freelist
= true; } UnlockBufHdr(bufHdr); if (add_to_freelist &&
StrategyMoveBufferToFreeListEnd(bufHdr)) num_to_free--;

3. This comment is now obsolete:

+ /*
+ * If bgwriterLatch is set, we need to waken the bgwriter, but we should
+ * not do so while holding freelist_lck; so set it after releasing the
+ * freelist_lck. This is annoyingly tedious, but it happens
at most once
+ * per bgwriter cycle, so the performance hit is minimal.
+ */
+

We're not actually holding any lock in need of releasing at that point
in the code, so this can be shortened to "If bgwriterLatch is set, we
need to waken the bgwriter."

* Ideally numFreeListBuffers should get called under freelist spinlock,

That doesn't make any sense. numFreeListBuffers is a variable, so you
can't "call" it. The value should be *read* under the spinlock, but
it is. I think this whole comment can be deleted and replaced with
"If the number of free buffers has fallen below the low water mark,
awaken the bgreclaimer to repopulate it."

4. StrategySyncStartAndEnd() is kind of a mess. One, it can return
the same victim buffer that's being handed out - at almost the same
time - to a backend running the clock sweep; if it does, they'll fight
over the buffer. Two, the *end out parameter actually returns a
count, not an endpoint. I think we should have
BgMoveBuffersToFreelist() call StrategySyncNextVictimBuffer() at the
top of the inner loop rather than the bottom, and change
StrategySyncStartAndEnd() so that it knows nothing about
victimbuf_lck. Let's also change StrategyGetBuffer() to call
StrategySyncNextVictimBuffer() so that the logic is centralized in one
place, and rename StrategySyncStartAndEnd() to something that better
matches its revised purpose. Maybe StrategyGetReclaimInfo().

5. Have you tested that this new bgwriter statistic is actually
working? Because it looks to me like BgMoveBuffersToFreelist is
changing BgWriterStats but never calling pgstat_send_bgwriter(), which
I'm thinking will mean the counters accumulate forever inside the
reclaimer but never get forwarded to the stats collector.

6. StrategyInitialize() uses #defines for
HIGH_WATER_MARK_FREELIST_BUFFERS_PERCENT and
LOW_WATER_MARK_FREELIST_BUFFERS_PERCENT but inline constants (5, 2000)
for clamping. Let's have constants for all of those (and omit mention
of the specific values in the comments).

7. The line you've added to the definition of view pg_stat_bgwriter
doesn't seem to be indented the same way as all the others. Tab vs.
space problem?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 5 September 2014 14:19, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Fri, Sep 5, 2014 at 5:17 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> >
> > Apart from above, I think for this patch, cat version bump is required
> > as I have modified system catalog. However I have not done the
> > same in patch as otherwise it will be bit difficult to take performance
> > data.
>
> One regression failed on linux due to spacing issue which is
> fixed in attached patch.
>

Here's a set of test results against this patch:

8-core AMD FX-9590, 32GB RAM

Config:
checkpoint_segments = 256
checkpoint_timeout = 15min
shared_buffers = 8GB

pgbench scale factor = 3000
run duration time = 5min

HEAD
Client Count/No. Of Runs (tps) 8 16 32 64 128
Run-1 31568 41890 48638 49266 41845
Run-2 31613 41879 48597 49332 41736
Run-3 31674 41987 48647 49320 41745

Patch=scalable_buffer_eviction_v7.patch
Client Count/No. Of Runs (tps) 8 16 32 64 128
Run-1 31880 42943 49359 49901 43779
Run-2 32150 43078 48934 49828 43769
Run-3 32323 42894 49481 49600 43529

--
Thom

On Mon, Sep 8, 2014 at 10:12 PM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
>
> On Fri, Sep 5, 2014 at 6:47 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > Client Count/Patch_Ver (tps) 8 16 32 64 128
> > HEAD 58614 107370 140717 104357 65010
> > Patch 60092 113564 165014 213848 216065
> >
> > This data is median of 3 runs, detailed report is attached with mail.
> > I have not repeated the test for all configurations, as there is no
> > major change in design/algorithm which can effect performance.
> > Mark has already taken tpc-b data which ensures that there is
> > no problem with it, however I will also take it once with latest
version.
>
> Well, these numbers are pretty much amazing. Question: It seems
> there's obviously quite a bit of contention left; do you think
> there's still a significant amount of time in the clock sweep, or is
> the primary bottleneck the buffer mapping locks?

I think contention around clock sweep is very minimal and for buffer
mapping locks it has reduced significantly (you can refer upthread
some of LWLOCK stat data, I have posted after this patch), but
might be we can get more out of it by improving hash table
concurrency. However apart from both of the above, the next thing
I have seen in profiles was palloc (at least that is what I remember
when I had done some profiling few months back during the
development of this patch). It seems to me at that time a totally
different optimisation, so I left it for another patch.
Another point is that the m/c on which I am doing performance
test has 16 cores (64 hardware threads), so not sure how much
scaling we can expect.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Sep 9, 2014 at 3:11 AM, Thom Brown <thom(at)linux(dot)com> wrote:
> On 5 September 2014 14:19, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Fri, Sep 5, 2014 at 5:17 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>> >
>> > Apart from above, I think for this patch, cat version bump is required
>> > as I have modified system catalog. However I have not done the
>> > same in patch as otherwise it will be bit difficult to take performance
>> > data.
>>
>> One regression failed on linux due to spacing issue which is
>> fixed in attached patch.
>
>
> Here's a set of test results against this patch:

Many thanks for taking the performance data. This data clearly shows
that there is a performance improvement at even lower configuration,
however the real benefit of the patch can be seen with higher core
m/c and with larger RAM (can contain all the data).

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 05/09/14 23:50, Amit Kapila wrote:
> On Fri, Sep 5, 2014 at 8:42 AM, Mark Kirkwood
> <mark(dot)kirkwood(at)catalyst(dot)net(dot)nz <mailto:mark(dot)kirkwood(at)catalyst(dot)net(dot)nz>>
> wrote:
> >
> > On 04/09/14 14:42, Amit Kapila wrote:
> >>
> >> On Thu, Sep 4, 2014 at 8:00 AM, Mark Kirkwood
> <mark(dot)kirkwood(at)catalyst(dot)net(dot)nz <mailto:mark(dot)kirkwood(at)catalyst(dot)net(dot)nz>>
> >> wrote:
> >>>
> >>>
> >>>
> >>> Hi Amit,
> >>>
> >>> Results look pretty good. Does it help in the read-write case too?
> >>
> >>
> >> Last time I ran the tpc-b test of pgbench (results of which are
> >> posted earlier in this thread), there doesn't seem to be any major
> >> gain for that, however for cases where read is predominant, you
> >> might see better gains.
> >>
> >> I am again planing to take that data in next few days.
> >>
> >
> > FWIW below are some test results on the 60 core beast with this patch
> applied to 9.4. I'll need to do more runs to iron out the variation,
> > but it looks like the patch helps the standard (write heavy) pgbench
> workload a little, and clearly helps the read only case.
> >
>
> Thanks for doing the test. I think if possible you can take
> the performance data with higher scale factor (4000) as it
> seems your m/c has 1TB of RAM. You might want to use
> latest patch I have posted today.
>

Here's some fairly typical data from read-write and read-only runs at
scale 4000 for 9.4 beta2 with and without the v7 patch (below). I'm not
seeing much variation between repeated read-write runs with the same
config (which is nice - sleep 30 and explicit checkpoint call between
each one seem to help there).

Interestingly, I note anecdotally that (unpatched) 9.4 beta2 seems to be
better at higher client counts than beta1 was...

In terms of the effect of the patch - looks pretty similar to the scale
2000 results for read-write, but read-only is a different and more
interesting story - unpatched 9.4 is noticeably impacted in the higher
client counts, whereas the patched version scales as well (or even
better perhaps) than in the scale 2000 case.

read write (600s)

Clients | tps | tps (unpatched)
---------+--------+----------------
6 | 9395 | 9334
12 | 16605 | 16525
24 | 24634 | 24910
48 | 32170 | 31275
96 | 35675 | 36533
192 | 35579 | 31137
384 | 30528 | 28308

read only (300s)

Clients | tps | tps (unpatched)
---------+--------+----------------
6 | 35743 | 35362
12 | 111019 | 106579
24 | 199746 | 160305
48 | 327026 | 198407
96 | 379184 | 171863
192 | 356623 | 152224
384 | 340878 | 128308

regards

Mark

On Tue, Sep 9, 2014 at 3:46 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Sep 5, 2014 at 9:19 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> One regression failed on linux due to spacing issue which is
>> fixed in attached patch.
I just read the latest patch by curiosity, wouldn't it make more sense
to split the patch into two different patches for clarity: one for the
reclaimer worker centralized around BgMoveBuffersToFreelist and a
second for the pg_stat portion? Those seem two different features.
Regards,
--
Michael

On Tue, Sep 9, 2014 at 12:16 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Sep 5, 2014 at 9:19 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > On Fri, Sep 5, 2014 at 5:17 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> >> Apart from above, I think for this patch, cat version bump is required
> >> as I have modified system catalog. However I have not done the
> >> same in patch as otherwise it will be bit difficult to take performance
> >> data.
> >
> > One regression failed on linux due to spacing issue which is
> > fixed in attached patch.
>
> I took another read through this patch. Here are some further review
comments:
>
> 1. In BgMoveBuffersToFreelist(), it seems quite superfluous to have
> both num_to_free and tmp_num_to_free.

num_to_free is used to accumulate total number of buffers that are
freed in one cycle of BgMoveBuffersToFreelist() which is reported
for debug info (BGW_DEBUG) and tmp_num_to_free is used as a temporary
number which is a count of number of buffers to be freed in one sub-cycle
(inner while loop)

> I'd get rid of tmp_num_to_free
> and move the declaration of num_to_free inside the outer loop. I'd
> also move the definitions of tmp_next_to_clean, tmp_recent_alloc,
> tmp_recent_backend_clocksweep into the innermost scope in which they
> are used.

okay, I have moved the tmp_* variables in innermost scope.

> 2. Also in that function, I think the innermost bit of logic could be
> rewritten more compactly, and in such a way as to make it clearer for
> what set of instructions the buffer header will be locked.
> LockBufHdr(bufHdr); if (bufHdr->refcount == 0) { if
> (bufHdr->usage_count > 0) bufHdr->usage_count--; else add_to_freelist
> = true; } UnlockBufHdr(bufHdr); if (add_to_freelist &&
> StrategyMoveBufferToFreeListEnd(bufHdr)) num_to_free--;

Changed as per suggestion.

> 3. This comment is now obsolete:
>
> + /*
> + * If bgwriterLatch is set, we need to waken the bgwriter, but we
should
> + * not do so while holding freelist_lck; so set it after
releasing the
> + * freelist_lck. This is annoyingly tedious, but it happens
> at most once
> + * per bgwriter cycle, so the performance hit is minimal.
> + */
> +
>
> We're not actually holding any lock in need of releasing at that point
> in the code, so this can be shortened to "If bgwriterLatch is set, we
> need to waken the bgwriter."

Changed as per suggestion.

> * Ideally numFreeListBuffers should get called under freelist
spinlock,
>
> That doesn't make any sense. numFreeListBuffers is a variable, so you
> can't "call" it. The value should be *read* under the spinlock, but
> it is. I think this whole comment can be deleted and replaced with
> "If the number of free buffers has fallen below the low water mark,
> awaken the bgreclaimer to repopulate it."

Changed as per suggestion.

> 4. StrategySyncStartAndEnd() is kind of a mess. One, it can return
> the same victim buffer that's being handed out - at almost the same
> time - to a backend running the clock sweep; if it does, they'll fight
> over the buffer. Two, the *end out parameter actually returns a
> count, not an endpoint. I think we should have
> BgMoveBuffersToFreelist() call StrategySyncNextVictimBuffer() at the
> top of the inner loop rather than the bottom, and change
> StrategySyncStartAndEnd() so that it knows nothing about
> victimbuf_lck. Let's also change StrategyGetBuffer() to call
> StrategySyncNextVictimBuffer() so that the logic is centralized in one
> place, and rename StrategySyncStartAndEnd() to something that better
> matches its revised purpose.

Changed as per suggestion. I have also updated
StrategySyncNextVictimBuffer() such that it increments completePasses
on completion of cycle as I think it is appropriate to update it, even when
clock sweep is done by bgreclaimer.

> Maybe StrategyGetReclaimInfo().

I have changed it to StrategyGetFreelistAccessInfo() as it seems most
other functions in freelist.c uses the names to sound something related
to buffers.

> 5. Have you tested that this new bgwriter statistic is actually
> working? Because it looks to me like BgMoveBuffersToFreelist is
> changing BgWriterStats but never calling pgstat_send_bgwriter(), which
> I'm thinking will mean the counters accumulate forever inside the
> reclaimer but never get forwarded to the stats collector.

pgstat_send_bgwriter() is called in bgreclaimer loop (caller of
BgMoveBuffersToFreelist, this is similar to how bgwriter does).
I have done few tests with it before sending the previous patch.

> 6. StrategyInitialize() uses #defines for
> HIGH_WATER_MARK_FREELIST_BUFFERS_PERCENT and
> LOW_WATER_MARK_FREELIST_BUFFERS_PERCENT but inline constants (5, 2000)
> for clamping. Let's have constants for all of those (and omit mention
> of the specific values in the comments).

Changed as per suggestion.

> 7. The line you've added to the definition of view pg_stat_bgwriter
> doesn't seem to be indented the same way as all the others. Tab vs.
> space problem?

Fixed.

Performance Data:
-------------------------------
Configuration and Db Details
IBM POWER-7 16 cores, 64 hardware threads
RAM = 64GB
Database Locale =C
checkpoint_segments=256
checkpoint_timeout =15min
shared_buffers=8GB
scale factor = 3000
Client Count = number of concurrent sessions and threads (ex. -c 8 -j 8)
Duration of each individual run = 5mins

All the data is in tps and taken using pgbench read-only load

Client Count/Patch_ver 8 16 32 64 128 HEAD 58614 107370 140717 104357
65010 Patch 61825 115152 170952 217389 220994

Observation
--------------------
1. The scalability/performance is similar to previous patch, slightly
better at higher client count.
2. I have taken the performance data just for one set of configuration,
as there doesn't seem to be any fundamental change which can impact
performance.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Sep 10, 2014 at 5:46 AM, Mark Kirkwood <
mark(dot)kirkwood(at)catalyst(dot)net(dot)nz> wrote:
>
> On 05/09/14 23:50, Amit Kapila wrote:
>>
>> On Fri, Sep 5, 2014 at 8:42 AM, Mark Kirkwood
>> > FWIW below are some test results on the 60 core beast with this patch
>> applied to 9.4. I'll need to do more runs to iron out the variation,
>> > but it looks like the patch helps the standard (write heavy) pgbench
>> workload a little, and clearly helps the read only case.
>> >
>>
>> Thanks for doing the test. I think if possible you can take
>> the performance data with higher scale factor (4000) as it
>> seems your m/c has 1TB of RAM. You might want to use
>> latest patch I have posted today.
>>
>
> Here's some fairly typical data from read-write and read-only runs at
scale 4000 for 9.4 beta2 with and without the v7 patch (below). I'm not
seeing much variation between repeated read-write runs with the same config
(which is nice - sleep 30 and explicit checkpoint call between each one
seem to help there).
>
> Interestingly, I note anecdotally that (unpatched) 9.4 beta2 seems to be
better at higher client counts than beta1 was...
>
> In terms of the effect of the patch - looks pretty similar to the scale
2000 results for read-write, but read-only is a different and more
interesting story - unpatched 9.4 is noticeably impacted in the higher
client counts, whereas the patched version scales as well (or even better
perhaps) than in the scale 2000 case.

Yeah, that's what I was expecting, the benefit of this patch
will be more at higher client count when there is large data
and all the data can fit in RAM .

Many thanks for doing the performance test for patch.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 10/09/14 18:54, Amit Kapila wrote:
> On Wed, Sep 10, 2014 at 5:46 AM, Mark Kirkwood
> <mark(dot)kirkwood(at)catalyst(dot)net(dot)nz <mailto:mark(dot)kirkwood(at)catalyst(dot)net(dot)nz>>
> wrote:
> >
> > In terms of the effect of the patch - looks pretty similar to the
> scale 2000 results for read-write, but read-only is a different and more
> interesting story - unpatched 9.4 is noticeably impacted in the higher
> client counts, whereas the patched version scales as well (or even
> better perhaps) than in the scale 2000 case.
>
> Yeah, that's what I was expecting, the benefit of this patch
> will be more at higher client count when there is large data
> and all the data can fit in RAM .
>
> Many thanks for doing the performance test for patch.
>
>

No worries, this is looking like a patch we're going to apply to 9.4 to
make the 60 core beast scale a bit better, so thanks very much for your
work in this area.

If you would like more tests run at higher scales let me know (we have
two of these machines at pre-production state currently so I can run
benchmarks as reqd)!

Regards

Mark

Hi,

On 2014-09-10 12:17:34 +0530, Amit Kapila wrote:
> include $(top_srcdir)/src/backend/common.mk
> diff --git a/src/backend/postmaster/bgreclaimer.c b/src/backend/postmaster/bgreclaimer.c
> new file mode 100644
> index 0000000..3df2337
> --- /dev/null
> +++ b/src/backend/postmaster/bgreclaimer.c

A fair number of comments in that file refer to bgwriter...

> @@ -0,0 +1,302 @@
> +/*-------------------------------------------------------------------------
> + *
> + * bgreclaimer.c
> + *
> + * The background reclaimer (bgreclaimer) is new as of Postgres 9.5. It
> + * attempts to keep regular backends from having to run clock sweep (which
> + * they would only do when they don't find a usable shared buffer from
> + * freelist to read in another page).

That's not really accurate. Freelist pages are often also needed to
write new pages, without reading anything in. I'd phrase it as "which
they only need to do if they don't find a victim buffer from the
freelist"

> In the best scenario all requests
> + * for shared buffers will be fulfilled from freelist as the background
> + * reclaimer process always tries to maintain buffers on freelist. However,
> + * regular backends are still empowered to run clock sweep to find a usable
> + * buffer if the bgreclaimer fails to maintain enough buffers on freelist.

"empowered" sounds strange to me. 'still can run the clock sweep'?

> + * The bgwriter is started by the postmaster as soon as the startup subprocess
> + * finishes, or as soon as recovery begins if we are doing archive recovery.

Why only archive recovery? I guess (only read this far...) it's not just
during InArchiveRecoveryb recovery but also StandbyMode? But I don't see
why we shouldn't use it during normal crash recovery. That's also often
painfully slow and the reclaimer could help? Less, but still.

> +static void bgreclaim_quickdie(SIGNAL_ARGS);
> +static void BgreclaimSigHupHandler(SIGNAL_ARGS);
> +static void ReqShutdownHandler(SIGNAL_ARGS);
> +static void bgreclaim_sigusr1_handler(SIGNAL_ARGS);

This looks inconsistent.

> + /*
> + * If an exception is encountered, processing resumes here.
> + *
> + * See notes in postgres.c about the design of this coding.
> + */
> + if (sigsetjmp(local_sigjmp_buf, 1) != 0)
> + {
> + /* Since not using PG_TRY, must reset error stack by hand */
> + error_context_stack = NULL;
> +
> + /* Prevent interrupts while cleaning up */
> + HOLD_INTERRUPTS();
> +
> + /* Report the error to the server log */
> + EmitErrorReport();
> +
> + /*
> + * These operations are really just a minimal subset of
> + * AbortTransaction(). We don't have very many resources to worry
> + * about in bgreclaim, but we do have buffers and file descriptors.
> + */
> + UnlockBuffers();
> + AtEOXact_Buffers(false);
> + AtEOXact_Files();

No LWLockReleaseAll(), AbortBufferIO(), ...? Unconvinced that that's a
good idea, regardless of it possibly being true today (which I'm not
sure about yet).

> + /*
> + * Now return to normal top-level context and clear ErrorContext for
> + * next time.
> + */
> + MemoryContextSwitchTo(bgreclaim_context);
> + FlushErrorState();
> +
> + /* Flush any leaked data in the top-level context */
> + MemoryContextResetAndDeleteChildren(bgreclaim_context);
> +
> + /* Now we can allow interrupts again */
> + RESUME_INTERRUPTS();

Other processes sleep for a second here, I think that's a good
idea. E.g. that bit:
/*
* Sleep at least 1 second after any error. A write error is likely
* to be repeated, and we don't want to be filling the error logs as
* fast as we can.
*/
pg_usleep(1000000L);

> + /*
> + * Loop forever
> + */
> + for (;;)
> + {
> + int rc;
> +
> +
> + /*
> + * Backend will signal bgreclaimer when the number of buffers in
> + * freelist falls below than low water mark of freelist.
> + */
> + rc = WaitLatch(&MyProc->procLatch,
> + WL_LATCH_SET | WL_POSTMASTER_DEATH,
> + -1);

That's probably not going to work well directly after a (re)start of
bgreclaim (depending on how you handle the water mark, I'll see in a
bit). Maybe it should rather be

ResetLatch();
BgMoveBuffersToFreelist();
pgstat_send_bgwriter();
rc = WaitLatch()
if (rc & WL_POSTMASTER_DEATH)
exit(1)

> +Background Reclaimer's Processing
> +---------------------------------
> +
> +The background reclaimer is designed to move buffers to freelist that are
> +likely to be recycled soon, thereby offloading the need to perform
> +clock sweep work from active backends. To do this, it runs the clock sweep
> +and move the the unpinned and zero usage count buffers to freelist. It
> +keeps on doing this until the number of buffers in freelist reaches the
> +high water mark.
> +
> +Two water mark indicators are used to maintain sufficient number of buffers
> +on freelist. Low water mark indicator is used by backends to wake bgreclaimer
> +when the number of buffers in freelist falls below it. High water mark
> +indicator is used by bgreclaimer to move buffers to freelist.

For me the description of the high water as stated here doesn't seem to
explain anything.

This section should have a description of how the reclaimer interacts
with the bgwriter logic. Do we put dirty buffers on the freelist that
are then cleaned by the bgwriter? Which buffers does the bgwriter write
out?

> /*
> + * Move buffers with reference and usage_count as zero to freelist.
> + * By maintaining enough number of buffers on freelist (equal to
> + * high water mark of freelist), we drastically reduce the odds for
> + * backend's to perform clock sweep.

Move buffers with reference and a usage_count *of* zero to freelist. By
maintaining enough buffers in the freelist (up to the list's high water
mark), we drastically reduce the likelihood of individual backends
having to perform the clock sweep themselves.

> + * This is called by the background reclaim process when the number
> + * of buffers in freelist falls below low water mark of freelist.
> + */

The logic used here *definitely* needs to be documented in another form
somewhere in the source.

> +void
> +BgMoveBuffersToFreelist(void)
> +{
> + uint32 num_to_free = 0;
> + uint32 recent_alloc = 0;
> + uint32 recent_backend_clocksweep = 0;
> + volatile uint32 next_victim = 0;
> +
> + /* Execute the clock sweep */
> + for (;;)
> + {
> + uint32 tmp_num_to_free;
> + uint32 tmp_recent_alloc;
> + uint32 tmp_recent_backend_clocksweep;
> +
> + StrategyGetFreelistAccessInfo(&tmp_num_to_free,
> + &tmp_recent_alloc,
> + &tmp_recent_backend_clocksweep);
> +
> + num_to_free += tmp_num_to_free;
> + recent_alloc += tmp_recent_alloc;
> + recent_backend_clocksweep += tmp_recent_backend_clocksweep;
> +
> + if (tmp_num_to_free == 0)
> + break;

num_to_free isn't a convincing name if I understand what this is doing
correctly. Maybe 'move_to_freelist', 'freelist_needed',
'needed_on_freelist' or something like that?

I wonder if we don't want to increase the high watermark when
tmp_recent_backend_clocksweep > 0?

> + while (tmp_num_to_free > 0)
> + {
> + volatile BufferDesc *bufHdr;
> + bool add_to_freelist = false;
> +
> + /*
> + * Choose next victim buffer to look if that can be moved
> + * to freelist.
> + */
> + StrategySyncNextVictimBuffer(&next_victim);
> +
> + bufHdr = &BufferDescriptors[next_victim];
> +
> + /*
> + * If the buffer is pinned or has a nonzero usage_count, we cannot
> + * move it to freelist; decrement the usage_count (unless pinned)
> + * and keep scanning.
> + */
> + LockBufHdr(bufHdr);

Hm. Perhaps we should do a bufHdr->refcount != zero check without
locking here? The atomic op will transfer the cacheline exclusively to
the reclaimer's CPU. Even though it very shortly afterwards will be
touched afterwards by the pinning backend.

> +/*
> + * Water mark indicators for maintaining buffers on freelist. When the
> + * number of buffers on freelist drops below the low water mark, the
> + * allocating backend sets the latch and bgreclaimer wakesup and begin
> + * adding buffer's to freelist until it reaches high water mark and then
> + * again goes back to sleep.
> + */

s/wakesup/wakes up/; s/begin adding/begins adding/; s/buffer's/buffers/;
/to freelist/to the freelist/; s/reaches high water/reaches the high water/

> +int freelistLowWaterMark;
> +int freelistHighWaterMark;
> +
> +/* Percentage indicators for maintaining buffers on freelist */
> +#define HIGH_WATER_MARK_FREELIST_BUFFERS_PERCENT 0.005
> +#define LOW_WATER_MARK_FREELIST_BUFFERS_PERCENT 0.2
> +#define MIN_HIGH_WATER_MARK 5
> +#define MAX_HIGH_WATER_MARK 2000

I'm confused. The high water mark percentage is smaller than the low
water mark?

What's the reasoning for these numbers? What's the justification for the
max of 2k buffers for the high watermark? That's not much on a busy
database with large s_b?

> - *lock_held = true;
> - LWLockAcquire(BufFreelistLock, LW_EXCLUSIVE);
> + SpinLockAcquire(&StrategyControl->freelist_lck);
>
> /*
> - * We count buffer allocation requests so that the bgwriter can estimate
> - * the rate of buffer consumption. Note that buffers recycled by a
> - * strategy object are intentionally not counted here.
> + * We count buffer allocation requests so that the bgwriter or bgreclaimer
> + * can know the rate of buffer consumption and report it as stats. Note
> + * that buffers recycled by a strategy object are intentionally not counted
> + * here.
> */
> StrategyControl->numBufferAllocs++;
>
> /*
> - * If bgwriterLatch is set, we need to waken the bgwriter, but we should
> - * not do so while holding BufFreelistLock; so release and re-grab. This
> - * is annoyingly tedious, but it happens at most once per bgwriter cycle,
> - * so the performance hit is minimal.
> + * Remember the values of bgwriter and bgreclaimer latch so that they can
> + * be set outside spin lock and try to get a buffer from the freelist.
> */
> + bgreclaimerLatch = StrategyControl->bgreclaimerLatch;
> bgwriterLatch = StrategyControl->bgwriterLatch;

I don't understand why these need to be grabbed under the spinlock?

> if (bgwriterLatch)
> - {
> StrategyControl->bgwriterLatch = NULL;
> - LWLockRelease(BufFreelistLock);
> - SetLatch(bgwriterLatch);
> - LWLockAcquire(BufFreelistLock, LW_EXCLUSIVE);
> - }
>
> - /*
> - * Try to get a buffer from the freelist. Note that the freeNext fields
> - * are considered to be protected by the BufFreelistLock not the
> - * individual buffer spinlocks, so it's OK to manipulate them without
> - * holding the spinlock.
> - */
> - while (StrategyControl->firstFreeBuffer >= 0)
> + numFreeListBuffers = StrategyControl->numFreeListBuffers;
> +
> + if (StrategyControl->firstFreeBuffer >= 0)
> {
> buf = &BufferDescriptors[StrategyControl->firstFreeBuffer];
> Assert(buf->freeNext != FREENEXT_NOT_IN_LIST);
> @@ -169,35 +198,82 @@ StrategyGetBuffer(BufferAccessStrategy strategy, bool *lock_held)
> /* Unconditionally remove buffer from freelist */
> StrategyControl->firstFreeBuffer = buf->freeNext;
> buf->freeNext = FREENEXT_NOT_IN_LIST;
> + --StrategyControl->numFreeListBuffers;
> + }
> + else
> + StrategyControl->numBufferBackendClocksweep++;
> +
> + SpinLockRelease(&StrategyControl->freelist_lck);

> + /* If bgwriterLatch is set, we need to waken the bgwriter */
> + if (bgwriterLatch)
> + SetLatch(bgwriterLatch);
> +
> + /*
> + * If the number of free buffers has fallen below the low water mark,
> + * awaken the bgreclaimer to repopulate it. bgreclaimerLatch is initialized in
> + * early phase of BgReclaimer startup, however we still check before using
> + * it to avoid any problem incase we reach here before its initializion.
> + */
> + if (numFreeListBuffers < freelistLowWaterMark && bgreclaimerLatch)
> + SetLatch(StrategyControl->bgreclaimerLatch);
> +
> + if (buf != NULL)
> + {
> /*
> - * If the buffer is pinned or has a nonzero usage_count, we cannot use
> - * it; discard it and retry. (This can only happen if VACUUM put a
> - * valid buffer in the freelist and then someone else used it before
> - * we got to it. It's probably impossible altogether as of 8.3, but
> - * we'd better check anyway.)
> + * Try to get a buffer from the freelist. Note that the freeNext fields
> + * are considered to be protected by the freelist_lck not the
> + * individual buffer spinlocks, so it's OK to manipulate them without
> + * holding the buffer spinlock.
> */
> - LockBufHdr(buf);
> - if (buf->refcount == 0 && buf->usage_count == 0)
> + for(;;)
> {
> - if (strategy != NULL)
> - AddBufferToRing(strategy, buf);
> - return buf;
> + /*
> + * If the buffer is pinned or has a nonzero usage_count, we cannot use
> + * it; discard it and retry.
> + */
> + LockBufHdr(buf);
> + if (buf->refcount == 0 && buf->usage_count == 0)
> + {
> + if (strategy != NULL)
> + AddBufferToRing(strategy, buf);
> + return buf;
> + }
> + UnlockBufHdr(buf);
> +
> + SpinLockAcquire(&StrategyControl->freelist_lck);
> +
> + if (StrategyControl->firstFreeBuffer >= 0)
> + {
> + buf = &BufferDescriptors[StrategyControl->firstFreeBuffer];
> + Assert(buf->freeNext != FREENEXT_NOT_IN_LIST);
> +
> + /* Unconditionally remove buffer from freelist */
> + StrategyControl->firstFreeBuffer = buf->freeNext;
> + buf->freeNext = FREENEXT_NOT_IN_LIST;
> + --StrategyControl->numFreeListBuffers;
> +
> + SpinLockRelease(&StrategyControl->freelist_lck);
> + }
> + else
> + {
> + StrategyControl->numBufferBackendClocksweep++;
> + SpinLockRelease(&StrategyControl->freelist_lck);
> + break;
> + }
> }
> - UnlockBufHdr(buf);
> }

I think it makes sense to break out this bit into its own
function. That'll make StrategyGetBuffer() a good bit easier to read.

> /* Nothing on the freelist, so run the "clock sweep" algorithm */
> trycounter = NBuffers;
> +
> for (;;)
> {
> - buf = &BufferDescriptors[StrategyControl->nextVictimBuffer];
> + volatile uint32 next_victim;
>
> - if (++StrategyControl->nextVictimBuffer >= NBuffers)
> - {
> - StrategyControl->nextVictimBuffer = 0;
> - StrategyControl->completePasses++;
> - }
> + StrategySyncNextVictimBuffer(&next_victim);
> +
> + buf = &BufferDescriptors[next_victim];

I'd also move this into its own function, but thats's more debatable.

> /*
> * If the buffer is pinned or has a nonzero usage_count, we cannot use
> @@ -241,7 +317,7 @@ StrategyGetBuffer(BufferAccessStrategy strategy, bool *lock_held)
> void
> StrategyFreeBuffer(volatile BufferDesc *buf)
> {
> - LWLockAcquire(BufFreelistLock, LW_EXCLUSIVE);
> + SpinLockAcquire(&StrategyControl->freelist_lck);
>
> /*
> * It is possible that we are told to put something in the freelist that
> @@ -253,11 +329,50 @@ StrategyFreeBuffer(volatile BufferDesc *buf)
> if (buf->freeNext < 0)
> StrategyControl->lastFreeBuffer = buf->buf_id;
> StrategyControl->firstFreeBuffer = buf->buf_id;
> + ++StrategyControl->numFreeListBuffers;
> + }
> +
> + SpinLockRelease(&StrategyControl->freelist_lck);
> +}
> +
> +/*
> + * StrategyMoveBufferToFreeListEnd: put a buffer on the end of freelist
> + */
> +bool
> +StrategyMoveBufferToFreeListEnd(volatile BufferDesc *buf)
> +{

Should maybe rather be named *Tail?

> + bool freed = false;
> + SpinLockAcquire(&StrategyControl->freelist_lck);
> +
> + /*
> + * It is possible that we are told to put something in the freelist that
> + * is already in it; don't screw up the list if so.
> + */

When/Why is that possible?

> /*
> + * StrategyGetFreelistAccessInfo -- get information required by bgreclaimer
> + * to move unused buffers to freelist.
> + *
> + * The result is the number of buffers that are required to be moved to
> + * freelist and count of recent buffer allocs and buffer allocs not
> + * satisfied from freelist.
> + */
> +void
> +StrategyGetFreelistAccessInfo(uint32 *num_buf_to_free, uint32 *num_buf_alloc,
> + uint32 *num_buf_backend_clocksweep)
> +{
> + int curfreebuffers;
> +
> + SpinLockAcquire(&StrategyControl->freelist_lck);
> + curfreebuffers = StrategyControl->numFreeListBuffers;
> + if (curfreebuffers < freelistHighWaterMark)
> + *num_buf_to_free = freelistHighWaterMark - curfreebuffers;
> + else
> + *num_buf_to_free = 0;
> +
> + if (num_buf_alloc)
> + {
> + *num_buf_alloc = StrategyControl->numBufferAllocs;
> + StrategyControl->numBufferAllocs = 0;
> + }
> + if (num_buf_backend_clocksweep)
> + {
> + *num_buf_backend_clocksweep = StrategyControl->numBufferBackendClocksweep;
> + StrategyControl->numBufferBackendClocksweep = 0;
> + }
> + SpinLockRelease(&StrategyControl->freelist_lck);
> +
> + return;
> +}

Do we need the if (num_buf_alloc) bits? Can't we make it unconditional?

Some more general remarks:
* I think there's a fair amount of unexplained heuristics in here
* Are we sure that the freelist_lck spinlock won't cause pain? Right now
there will possibly be dozens of processes busily spinning on it... I
think it's a acceptable risk, but we should think about it.
* I'm not convinced that these changes can be made without also changing
the bgwriter logic. Have you measured whether there are differences in
how effective the bgwriter is? Not that it's very effective right now :)

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Thanks for reviewing, Andres.

On Thu, Sep 11, 2014 at 7:01 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> +static void bgreclaim_quickdie(SIGNAL_ARGS);
>> +static void BgreclaimSigHupHandler(SIGNAL_ARGS);
>> +static void ReqShutdownHandler(SIGNAL_ARGS);
>> +static void bgreclaim_sigusr1_handler(SIGNAL_ARGS);
>
> This looks inconsistent.

It's exactly the same as what bgwriter.c does.

> No LWLockReleaseAll(), AbortBufferIO(), ...? Unconvinced that that's a
> good idea, regardless of it possibly being true today (which I'm not
> sure about yet).

We really need a more centralized way to handle error cleanup in
auxiliary processes. The current state of affairs is really pretty
helter-skelter. But for this patch, I think we should aim to mimic
the existing style, as ugly as it is. I'm not sure whether Amit's got
the logic correct, though: I'd agree LWLockReleaseAll(), at a minimum,
is probably a good idea.

>> +Two water mark indicators are used to maintain sufficient number of buffers
>> +on freelist. Low water mark indicator is used by backends to wake bgreclaimer
>> +when the number of buffers in freelist falls below it. High water mark
>> +indicator is used by bgreclaimer to move buffers to freelist.
>
> For me the description of the high water as stated here doesn't seem to
> explain anything.

Yeah, let me try to revise and expand on that a bit:

Background Reclaimer's Processing
---------------------------------

The background reclaimer runs the clock sweep to identify buffers that
are good candidates for eviction and puts them on the freelist. This
makes buffer allocation much faster, since removing a buffer from the
head of a linked list is much cheaper than linearly scanning the whole
buffer pool until a promising candidate is found. It's possible that
a buffer we add to the freelist may be accessed or even pinned before
it's evicted; if that happens, the backend that would have evicted it
will simply disregard it and take the next buffer instead (or run the
clock sweep itself, if necessary). However, to make sure that doesn't
happen too often, we need to keep the freelist as short as possible,
so that there won't be many other buffer accesses between when the
time a buffer is added to the freelist and the time when it's actually
evicted.

We use two water marks to control the activity of the bgreclaimer
process. Each time bgreclaimer is awoken, it will move buffers to the
freelist until the length of the free list reaches the high water
mark. It will then sleep. When the number of buffers on the freelist
reaches the low water mark, backends attempting to allocate new
buffers will set the bgreclaimer's latch, waking it up again. While
it's important for the high water mark to be small (for the reasons
described above), we also need to ensure adequate separation between
the low and high water marks, so that the bgreclaimer isn't constantly
being awoken to find just a handful of additional candidate buffers,
and we need to ensure that the low watermark is adequate to keep the
freelist from becoming completely empty before bgreclaimer has time to
wake up and beginning filling it again.

> This section should have a description of how the reclaimer interacts
> with the bgwriter logic. Do we put dirty buffers on the freelist that
> are then cleaned by the bgwriter? Which buffers does the bgwriter write
> out?

The bgwriter is cleaning ahead of the strategy point, and the
bgreclaimer is advancing the strategy point. I think we should
consider having the bgreclaimer wake the bgwriter if it comes across a
dirty buffer, because while the bgwriter only estimates the rate of
buffer allocation, bgreclaimer *knows* the rate of allocation, because
its own activity is tied to the allocation rate. I think there's the
potential for this kind of thing to make the background writer
significantly more effective than it is today, but I'm heavily in
favor of leaving it for a separate patch.

> I wonder if we don't want to increase the high watermark when
> tmp_recent_backend_clocksweep > 0?

That doesn't really work unless there's some countervailing force to
eventually reduce it again; otherwise, it'd just converge to infinity.
And it doesn't really seem necessary at the moment.

> Hm. Perhaps we should do a bufHdr->refcount != zero check without
> locking here? The atomic op will transfer the cacheline exclusively to
> the reclaimer's CPU. Even though it very shortly afterwards will be
> touched afterwards by the pinning backend.

Meh. I'm not in favor of adding more funny games with locking unless
we can prove they're necessary for performance.

> * Are we sure that the freelist_lck spinlock won't cause pain? Right now
> there will possibly be dozens of processes busily spinning on it... I
> think it's a acceptable risk, but we should think about it.

As you and I have talked about before, we could reduce contention here
by partitioning the freelist, or by using a CAS loop to pop items off
of it. But I am not convinced either is necessary; I think it's hard
for the system to accumulate enough people hitting the freelist
simultaneously to matter, because the stuff they've got to do between
one freelist access and the next is generally going to be something
much more expensive, like reading 8kB from the OS.

One question in my mind is whether we ought to separate this patch
into two - one for the changes to the locking regime, and another for
the addition of the bgreclaimer process. Those things are really two
different features, although they are tightly enough coupled that
maybe it's OK to keep them together. I also think it would be good to
get some statistics on how often regular backends are running the
clocksweep vs. how often bgreclaimer is satisfying their needs.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2014-09-11 09:02:34 -0400, Robert Haas wrote:
> Thanks for reviewing, Andres.
>
> On Thu, Sep 11, 2014 at 7:01 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> +static void bgreclaim_quickdie(SIGNAL_ARGS);
> >> +static void BgreclaimSigHupHandler(SIGNAL_ARGS);
> >> +static void ReqShutdownHandler(SIGNAL_ARGS);
> >> +static void bgreclaim_sigusr1_handler(SIGNAL_ARGS);
> >
> > This looks inconsistent.
>
> It's exactly the same as what bgwriter.c does.

So what? There's no code in common, so I see no reason to have one
signal handler using underscores and the next one camelcase names.

> > No LWLockReleaseAll(), AbortBufferIO(), ...? Unconvinced that that's a
> > good idea, regardless of it possibly being true today (which I'm not
> > sure about yet).
>
> We really need a more centralized way to handle error cleanup in
> auxiliary processes. The current state of affairs is really pretty
> helter-skelter.

Agreed. There really should be three variants:
* full abort including support for transactions
* full abort without transactions being used (most background processes)
* abort without shared memory interactions

> But for this patch, I think we should aim to mimic
> the existing style, as ugly as it is.

Agreed.

> Background Reclaimer's Processing
> ---------------------------------
>
> The background reclaimer runs the clock sweep to identify buffers that
> are good candidates for eviction and puts them on the freelist. This
> makes buffer allocation much faster, since removing a buffer from the
> head of a linked list is much cheaper than linearly scanning the whole
> buffer pool until a promising candidate is found. It's possible that
> a buffer we add to the freelist may be accessed or even pinned before
> it's evicted; if that happens, the backend that would have evicted it
> will simply disregard it and take the next buffer instead (or run the
> clock sweep itself, if necessary). However, to make sure that doesn't
> happen too often, we need to keep the freelist as short as possible,
> so that there won't be many other buffer accesses between when the
> time a buffer is added to the freelist and the time when it's actually
> evicted.
>
> We use two water marks to control the activity of the bgreclaimer
> process. Each time bgreclaimer is awoken, it will move buffers to the
> freelist until the length of the free list reaches the high water
> mark. It will then sleep.

I wonder if we should recheck the number of freelist items before
sleeping. As the latch currently is reset before sleeping (IIRC) we
might miss being woken up soon. It very well might be that bgreclaim
needs to run for more than one cycle in a row to keep up...

> > This section should have a description of how the reclaimer interacts
> > with the bgwriter logic. Do we put dirty buffers on the freelist that
> > are then cleaned by the bgwriter? Which buffers does the bgwriter write
> > out?
>
> The bgwriter is cleaning ahead of the strategy point, and the
> bgreclaimer is advancing the strategy point.

That sentence, in some form, should be in the above paragraph.

> I think we should
> consider having the bgreclaimer wake the bgwriter if it comes across a
> dirty buffer, because while the bgwriter only estimates the rate of
> buffer allocation, bgreclaimer *knows* the rate of allocation, because
> its own activity is tied to the allocation rate. I think there's the
> potential for this kind of thing to make the background writer
> significantly more effective than it is today, but I'm heavily in
> favor of leaving it for a separate patch.

Yes, doing that sounds like a good plan. I'm happy with that being done
in a separate patch.

> > I wonder if we don't want to increase the high watermark when
> > tmp_recent_backend_clocksweep > 0?
>
> That doesn't really work unless there's some countervailing force to
> eventually reduce it again; otherwise, it'd just converge to infinity.
> And it doesn't really seem necessary at the moment.

Right, it obviously needs to go both ways. I'm a bit sceptic about
untunable, fixed, numbers proving to be accurate for widely varied
workloads.

> > Hm. Perhaps we should do a bufHdr->refcount != zero check without
> > locking here? The atomic op will transfer the cacheline exclusively to
> > the reclaimer's CPU. Even though it very shortly afterwards will be
> > touched afterwards by the pinning backend.
>
> Meh. I'm not in favor of adding more funny games with locking unless
> we can prove they're necessary for performance.

Well, this in theory increases the number of processes touching buffer
headers regularly. Currently, if you have one read IO intensive backend,
there's pretty much only process touching the cachelines. This will make
it two. I don't think it's unreasonable to try to reduce the cacheline
pingpong caused by that...

> > * Are we sure that the freelist_lck spinlock won't cause pain? Right now
> > there will possibly be dozens of processes busily spinning on it... I
> > think it's a acceptable risk, but we should think about it.
>
> As you and I have talked about before, we could reduce contention here
> by partitioning the freelist, or by using a CAS loop to pop items off
> of it. But I am not convinced either is necessary; I think it's hard
> for the system to accumulate enough people hitting the freelist
> simultaneously to matter, because the stuff they've got to do between
> one freelist access and the next is generally going to be something
> much more expensive, like reading 8kB from the OS.
>
> One question in my mind is whether we ought to separate this patch
> into two - one for the changes to the locking regime, and another for
> the addition of the bgreclaimer process. Those things are really two
> different features, although they are tightly enough coupled that
> maybe it's OK to keep them together.

I think it's ok to commit them together. Hm, although: It'd actually be
highly interesting to see the effect of replacing the freelist lock with
a spinlock without the rest of these changes. I think that's really a
number we want to see at least once.

> I also think it would be good to
> get some statistics on how often regular backends are running the
> clocksweep vs. how often bgreclaimer is satisfying their needs.

I think that's necessary. The patch added buf_backend_clocksweep. Maybe
we just also need buf_backend_from_freelist?

Greetings,

Andres Freund

On Thu, Sep 11, 2014 at 6:32 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> Thanks for reviewing, Andres.
>
> On Thu, Sep 11, 2014 at 7:01 AM, Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:
> >> +static void bgreclaim_quickdie(SIGNAL_ARGS);
> >> +static void BgreclaimSigHupHandler(SIGNAL_ARGS);
> >> +static void ReqShutdownHandler(SIGNAL_ARGS);
> >> +static void bgreclaim_sigusr1_handler(SIGNAL_ARGS);
> >
> > This looks inconsistent.
>
> It's exactly the same as what bgwriter.c does.
>
> > No LWLockReleaseAll(), AbortBufferIO(), ...? Unconvinced that that's a
> > good idea, regardless of it possibly being true today (which I'm not
> > sure about yet).
>
> We really need a more centralized way to handle error cleanup in
> auxiliary processes. The current state of affairs is really pretty
> helter-skelter. But for this patch, I think we should aim to mimic
> the existing style, as ugly as it is. I'm not sure whether Amit's got
> the logic correct, though: I'd agree LWLockReleaseAll(), at a minimum,
> is probably a good idea.

Code related to bgreclaimer logic itself doesn't take any LWLock, do
you suspect the same might be required due to some Signal/Interrupt
handling?

From myside, I have thought about what to keep for error cleanup based
on the working of bgreclaimer. However there is a chance that I have
missed something.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

> > We really need a more centralized way to handle error cleanup in
> > auxiliary processes. The current state of affairs is really pretty
> > helter-skelter. But for this patch, I think we should aim to mimic
> > the existing style, as ugly as it is. I'm not sure whether Amit's got
> > the logic correct, though: I'd agree LWLockReleaseAll(), at a minimum,
> > is probably a good idea.
>
> Code related to bgreclaimer logic itself doesn't take any LWLock, do
> you suspect the same might be required due to some Signal/Interrupt
> handling?

I suspect it might creep in at some point at some unrelated place. Which
will only ever break in production scenarios. Say, a lwlock in in config
file processing. I seem to recall somebody seing a version of a patching
adding a lwlock there... :). Or a logging hook. Or ...

The savings from not doing LWLockReleaseAll() are nonexistant, so ...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Sep 11, 2014 at 6:59 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:
>
> > > We really need a more centralized way to handle error cleanup in
> > > auxiliary processes. The current state of affairs is really pretty
> > > helter-skelter. But for this patch, I think we should aim to mimic
> > > the existing style, as ugly as it is. I'm not sure whether Amit's got
> > > the logic correct, though: I'd agree LWLockReleaseAll(), at a minimum,
> > > is probably a good idea.
> >
> > Code related to bgreclaimer logic itself doesn't take any LWLock, do
> > you suspect the same might be required due to some Signal/Interrupt
> > handling?
>
> I suspect it might creep in at some point at some unrelated place. Which
> will only ever break in production scenarios. Say, a lwlock in in config
> file processing.

Yeah, I suspected the same and checked that path, but couldn't find but
may be in some path it is there as the code has many flows.

> I seem to recall somebody seing a version of a patching
> adding a lwlock there... :). Or a logging hook. Or ...
>
> The savings from not doing LWLockReleaseAll() are nonexistant, so ...

Okay, I shall add it in next version of patch and mention in comments
the reasons quoted by you.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Sep 11, 2014 at 9:22 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> It's exactly the same as what bgwriter.c does.
>
> So what? There's no code in common, so I see no reason to have one
> signal handler using underscores and the next one camelcase names.

/me shrugs.

It's not always possible to have things be consistent with each other
within a file and also with what gets done in other files. I'm not
sure we should fault patch authors for choosing a different one than
we would have chosen. FWIW, I probably would have done it the way
Amit did it. I don't actually care, though.

> I wonder if we should recheck the number of freelist items before
> sleeping. As the latch currently is reset before sleeping (IIRC) we
> might miss being woken up soon. It very well might be that bgreclaim
> needs to run for more than one cycle in a row to keep up...

The outer loop in BgMoveBuffersToFreelist() was added to address
precisely this point, which I raised in a previous review.

>> > I wonder if we don't want to increase the high watermark when
>> > tmp_recent_backend_clocksweep > 0?
>>
>> That doesn't really work unless there's some countervailing force to
>> eventually reduce it again; otherwise, it'd just converge to infinity.
>> And it doesn't really seem necessary at the moment.
>
> Right, it obviously needs to go both ways. I'm a bit sceptic about
> untunable, fixed, numbers proving to be accurate for widely varied
> workloads.

Me, too, but I'm *even more* skeptical about making things complicated
on the pure theory that a simple solution can't be correct. I'm not
blind to the possibility that the current logic is inadequate, but
testing proves that it works well enough to produce a massive
performance boost over where we are now. When, and if, we develop a
theory about specifically how it falls short then, sure, let's adjust
it. But I think it would be a serious error to try to engineer a
perfect algorithm here based on the amount of testing that we can
reasonably do pre-commit. We have no chance of getting that right,
and I'd rather have an algorithm that is simple and imperfect than an
algorithm that is complex and still imperfect. No matter what, it's
better than what we have now.

>> > Hm. Perhaps we should do a bufHdr->refcount != zero check without
>> > locking here? The atomic op will transfer the cacheline exclusively to
>> > the reclaimer's CPU. Even though it very shortly afterwards will be
>> > touched afterwards by the pinning backend.
>>
>> Meh. I'm not in favor of adding more funny games with locking unless
>> we can prove they're necessary for performance.
>
> Well, this in theory increases the number of processes touching buffer
> headers regularly. Currently, if you have one read IO intensive backend,
> there's pretty much only process touching the cachelines. This will make
> it two. I don't think it's unreasonable to try to reduce the cacheline
> pingpong caused by that...

It's not unreasonable, but this is a good place to apply Knuth's first
law of optimization. There's no proof we need to do this, so let's
not until there is.

>> I also think it would be good to
>> get some statistics on how often regular backends are running the
>> clocksweep vs. how often bgreclaimer is satisfying their needs.
>
> I think that's necessary. The patch added buf_backend_clocksweep. Maybe
> we just also need buf_backend_from_freelist?

That's just (or should be just) buf_alloc - buf_backend_clocksweep.

I think buf_backend_clocksweep should really be called
buf_alloc_clocksweep, and should be added (in all relevant places)
right next to buf_alloc.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2014-09-11 09:48:10 -0400, Robert Haas wrote:
> On Thu, Sep 11, 2014 at 9:22 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > I wonder if we should recheck the number of freelist items before
> > sleeping. As the latch currently is reset before sleeping (IIRC) we
> > might miss being woken up soon. It very well might be that bgreclaim
> > needs to run for more than one cycle in a row to keep up...
>
> The outer loop in BgMoveBuffersToFreelist() was added to address
> precisely this point, which I raised in a previous review.

Hm, right. But then let's move BgWriterStats.m_buf_alloc =+,
... pgstat_send_bgwriter(); into that loop. Otherwise it'd possibly end
up being continously busy without being visible.

> >> > I wonder if we don't want to increase the high watermark when
> >> > tmp_recent_backend_clocksweep > 0?
> >>
> >> That doesn't really work unless there's some countervailing force to
> >> eventually reduce it again; otherwise, it'd just converge to infinity.
> >> And it doesn't really seem necessary at the moment.
> >
> > Right, it obviously needs to go both ways. I'm a bit sceptic about
> > untunable, fixed, numbers proving to be accurate for widely varied
> > workloads.
>
> Me, too, but I'm *even more* skeptical about making things complicated
> on the pure theory that a simple solution can't be correct.

Fair enough.

> I'm not
> blind to the possibility that the current logic is inadequate, but
> testing proves that it works well enough to produce a massive
> performance boost over where we are now.

But, to be honest, the testing so far was pretty "narrow" in the kind of
workloads that were run if I crossread things accurately. Don't get me
wrong, I'm *really* happy about having this patch, that just doesn't
mean every detail is right ;)

> >> > Hm. Perhaps we should do a bufHdr->refcount != zero check without
> >> > locking here? The atomic op will transfer the cacheline exclusively to
> >> > the reclaimer's CPU. Even though it very shortly afterwards will be
> >> > touched afterwards by the pinning backend.
> >>
> >> Meh. I'm not in favor of adding more funny games with locking unless
> >> we can prove they're necessary for performance.
> >
> > Well, this in theory increases the number of processes touching buffer
> > headers regularly. Currently, if you have one read IO intensive backend,
> > there's pretty much only process touching the cachelines. This will make
> > it two. I don't think it's unreasonable to try to reduce the cacheline
> > pingpong caused by that...
>
> It's not unreasonable, but this is a good place to apply Knuth's first
> law of optimization. There's no proof we need to do this, so let's
> not until there is.

That's true for new (pieces of) software; less so, when working with a
installed base that you might regress... But whatever.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Sep 11, 2014 at 10:03 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-09-11 09:48:10 -0400, Robert Haas wrote:
>> On Thu, Sep 11, 2014 at 9:22 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > I wonder if we should recheck the number of freelist items before
>> > sleeping. As the latch currently is reset before sleeping (IIRC) we
>> > might miss being woken up soon. It very well might be that bgreclaim
>> > needs to run for more than one cycle in a row to keep up...
>>
>> The outer loop in BgMoveBuffersToFreelist() was added to address
>> precisely this point, which I raised in a previous review.
>
> Hm, right. But then let's move BgWriterStats.m_buf_alloc =+,
> ... pgstat_send_bgwriter(); into that loop. Otherwise it'd possibly end
> up being continously busy without being visible.

Good idea.

>> I'm not
>> blind to the possibility that the current logic is inadequate, but
>> testing proves that it works well enough to produce a massive
>> performance boost over where we are now.
>
> But, to be honest, the testing so far was pretty "narrow" in the kind of
> workloads that were run if I crossread things accurately. Don't get me
> wrong, I'm *really* happy about having this patch, that just doesn't
> mean every detail is right ;)

Oh, sure. Totally agreed. And, to the extent that we're improving
things based on actual testing, I'm A-OK with that. I just don't want
to start speculating, or we'll never get this thing off the ground.

Some possibly-interesting test cases would be:

(1) A read-only pgbench workload that is just a tiny bit larger than
shared_buffers, say size of shared_buffers plus 0.01%. Such workloads
tend to stress buffer eviction heavily.

(2) A workload that maximizes the rate of concurrent buffer eviction
relative to other tasks. Read-only pgbench is not bad for this, but
maybe somebody's got a better idea.

As I sort of mentioned in what I was writing for the bufmgr README,
there are, more or less, three ways this can fall down, at least that
I can see: (1) if the high water mark is too high, then we'll start
finding buffers in the freelist that have already been touched since
we added them: (2) if the low water mark is too low, the freelist will
run dry; and (3) if the low and high water marks are too close
together, the bgreclaimer will be constantly getting woken up and
going to sleep again. I can't personally think of a workload that
will enable us to get a better handle on those cases than
high-concurrency pgbench, but you're known to be ingenious at coming
up with destruction workloads, so if you have an idea, by all means
fire away.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Sep 11, 2014 at 4:31 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:
> On 2014-09-10 12:17:34 +0530, Amit Kapila wrote:
> > +++ b/src/backend/postmaster/bgreclaimer.c
>
> A fair number of comments in that file refer to bgwriter...

Will fix.

> > @@ -0,0 +1,302 @@
> >
+/*-------------------------------------------------------------------------
> > + *
> > + * bgreclaimer.c
> > + *
> > + * The background reclaimer (bgreclaimer) is new as of Postgres 9.5.
It
> > + * attempts to keep regular backends from having to run clock sweep
(which
> > + * they would only do when they don't find a usable shared buffer from
> > + * freelist to read in another page).
>
> That's not really accurate. Freelist pages are often also needed to
> write new pages, without reading anything in.

Agreed, but the same is used in bgwriter file as well; so if we
change here, we might want to change bgwriter file header as well.

> I'd phrase it as "which
> they only need to do if they don't find a victim buffer from the
> freelist"

victim buffer sounds more like a buffer which it will get from
clock sweep, how about next candidate (same is used in function
header of StrategyGetBuffer()).

> > In the best scenario all requests
> > + * for shared buffers will be fulfilled from freelist as the background
> > + * reclaimer process always tries to maintain buffers on freelist.
However,
> > + * regular backends are still empowered to run clock sweep to find a
usable
> > + * buffer if the bgreclaimer fails to maintain enough buffers on
freelist.
>
> "empowered" sounds strange to me. 'still can run the clock sweep'?

No harm in changing like what you are suggesting, however the same is
used in file header of bgwriter.c as well, so I think lets keep this usage
as
it is because there is no correctness issue here.

> > + * The bgwriter is started by the postmaster as soon as the startup
subprocess
> > + * finishes, or as soon as recovery begins if we are doing archive
recovery.
>
> Why only archive recovery? I guess (only read this far...) it's not just
> during InArchiveRecoveryb recovery but also StandbyMode?

It will be for both.

> But I don't see
> why we shouldn't use it during normal crash recovery. That's also often
> painfully slow and the reclaimer could help? Less, but still.

Yes, it might improve a bit, however the main benefit with this patch is
under heavy load which means that it mainly addresses contention and
in case of crash recovery there will not be any contention because there
will be no backend processes.

Also I think to enable bgreclaimer during crash recovery, we need to have
one new signal which is not a problem, but it will make more sense to enable
it later based on benefit if it is there.

>
> > +static void bgreclaim_quickdie(SIGNAL_ARGS);
> > +static void BgreclaimSigHupHandler(SIGNAL_ARGS);
> > +static void ReqShutdownHandler(SIGNAL_ARGS);
> > +static void bgreclaim_sigusr1_handler(SIGNAL_ARGS);
>
> This looks inconsistent.

I have kept based on bgwriter, so not sure if it's good to change.
However I we want consistent in naming, I would like to keep
something like:

ReclaimShutdownHandler
ReclaimQuickDieHandler
..
..

> > + /*
> > + * If an exception is encountered, processing resumes here.
> > + *
> > + * See notes in postgres.c about the design of this coding.
> > + */
> > + if (sigsetjmp(local_sigjmp_buf, 1) != 0)
> > + {
> > + /* Since not using PG_TRY, must reset error stack by hand
*/
> > + error_context_stack = NULL;
..
>
> No LWLockReleaseAll(), AbortBufferIO(), ...? Unconvinced that that's a
> good idea, regardless of it possibly being true today (which I'm not
> sure about yet).

I will add LWLockReleaseAll() in exception handling as discussed
elsewhere in thread.

> > +
> > + /* Now we can allow interrupts again */
> > + RESUME_INTERRUPTS();
>
> Other processes sleep for a second here, I think that's a good
> idea. E.g. that bit:

Agreed, will make change as per suggestion.

>
> > + /*
> > + * Loop forever
> > + */
> > + for (;;)
> > + {
> > + int rc;
> > +
> > +
> > + /*
> > + * Backend will signal bgreclaimer when the number of
buffers in
> > + * freelist falls below than low water mark of freelist.
> > + */
> > + rc = WaitLatch(&MyProc->procLatch,
> > + WL_LATCH_SET |
WL_POSTMASTER_DEATH,
> > + -1);
>
> That's probably not going to work well directly after a (re)start of
> bgreclaim (depending on how you handle the water mark, I'll see in a
> bit).

Could you please be more specific here?

>
> > +Background Reclaimer's Processing
> > +---------------------------------
..
> > +Two water mark indicators are used to maintain sufficient number of
buffers
> > +on freelist. Low water mark indicator is used by backends to wake
bgreclaimer
> > +when the number of buffers in freelist falls below it. High water mark
> > +indicator is used by bgreclaimer to move buffers to freelist.
>
> For me the description of the high water as stated here doesn't seem to
> explain anything.
>
> This section should have a description of how the reclaimer interacts
> with the bgwriter logic. Do we put dirty buffers on the freelist that
> are then cleaned by the bgwriter? Which buffers does the bgwriter write
> out?

As discussed in thread, I will change this accordingly.

> > /*
> > + * Move buffers with reference and usage_count as zero to freelist.
> > + * By maintaining enough number of buffers on freelist (equal to
> > + * high water mark of freelist), we drastically reduce the odds for
> > + * backend's to perform clock sweep.
>
> Move buffers with reference and a usage_count *of* zero to freelist. By
> maintaining enough buffers in the freelist (up to the list's high water
> mark), we drastically reduce the likelihood of individual backends
> having to perform the clock sweep themselves.

Okay will rephrase it as per your suggestion.

> > + * This is called by the background reclaim process when the number
> > + * of buffers in freelist falls below low water mark of freelist.
> > + */
>
> The logic used here *definitely* needs to be documented in another form
> somewhere in the source.

I think the way Robert has suggested to modify Readme adresses
this to an extent, however if you think it is better to go in more
detail, then I can expand function header of function
BgMoveBuffersToFreelist() or on top of bgreclaimer.c, what do you prefer?

> > +
> > + if (tmp_num_to_free == 0)
> > + break;
>
> num_to_free isn't a convincing name if I understand what this is doing
> correctly. Maybe 'move_to_freelist', 'freelist_needed',
> 'needed_on_freelist' or something like that?

I think keeping num or count in name could be more helpful as it is used
to loop for finding usable buffers. How about 'num_needed_on_freelist',
without any count or num it sounds more like a boolean variable.

>
> I wonder if we don't want to increase the high watermark when
> tmp_recent_backend_clocksweep > 0?
>
> > + while (tmp_num_to_free > 0)
> > + {
..
> > + /*
> > + * If the buffer is pinned or has a nonzero
usage_count, we cannot
> > + * move it to freelist; decrement the usage_count
(unless pinned)
> > + * and keep scanning.
> > + */
> > + LockBufHdr(bufHdr);
>
> Hm. Perhaps we should do a bufHdr->refcount != zero check without
> locking here? The atomic op will transfer the cacheline exclusively to
> the reclaimer's CPU. Even though it very shortly afterwards will be
> touched afterwards by the pinning backend.

As per discussion, the conclusion seems to be that we can do some
more tests to see if we need such a change, I will do more tests on
the lines suggested by Robert in below mails and then we can decide
if any thing is required.

> > +/*
> > + * Water mark indicators for maintaining buffers on freelist. When the
> > + * number of buffers on freelist drops below the low water mark, the
> > + * allocating backend sets the latch and bgreclaimer wakesup and begin
> > + * adding buffer's to freelist until it reaches high water mark and
then
> > + * again goes back to sleep.
> > + */
>
> s/wakesup/wakes up/; s/begin adding/begins adding/; s/buffer's/buffers/;
> /to freelist/to the freelist/; s/reaches high water/reaches the high
water/

Will change as per suggestions.

> > +int freelistLowWaterMark;
> > +int freelistHighWaterMark;
> > +
> > +/* Percentage indicators for maintaining buffers on freelist */
> > +#define HIGH_WATER_MARK_FREELIST_BUFFERS_PERCENT 0.005
> > +#define LOW_WATER_MARK_FREELIST_BUFFERS_PERCENT 0.2
> > +#define MIN_HIGH_WATER_MARK 5
> > +#define MAX_HIGH_WATER_MARK 2000
>
> I'm confused. The high water mark percentage is smaller than the low
> water mark?

High water mark is percentage of NBuffers (shared_buffers).
Low water mark is percentage of High water mark.

I will add a comment.

> What's the reasoning for these numbers?

Based on experiments with different amount of shared buffers

> What's the justification for the
> max of 2k buffers for the high watermark? That's not much on a busy
> database with large s_b?

I have done tests at various loads upto 15GB of shared_buffers
and having data size as 44GB, 2000 turns out to be good enough
number of buffers on freelist(based on performance data).

Another thing I have mentioned in begining was to have guc for
these thresholds, but I think it will be much more difficult for
user to decide these values.

I think for now it is okay to fix these numbers as in patch
and we can anyway go back to add guc's or change them based
on more tests if anything turns out that needs further tuning.

> > - *lock_held = true;
> > - LWLockAcquire(BufFreelistLock, LW_EXCLUSIVE);
> > + SpinLockAcquire(&StrategyControl->freelist_lck);
> >
..
> > + bgreclaimerLatch = StrategyControl->bgreclaimerLatch;
> > bgwriterLatch = StrategyControl->bgwriterLatch;
>
> I don't understand why these need to be grabbed under the spinlock?

In earlier version of patch, it was done without spinklock,
however Robert has given comment that as it was previously done
with BufFreelist lock, these should be under spinlock (atleast
that is what I understood) and I tested the patch again by having
them under spinlock and didn't find any difference, so I have moved
them under spinlock.

..
> > + if (numFreeListBuffers < freelistLowWaterMark &&
bgreclaimerLatch)
> > + SetLatch(StrategyControl->bgreclaimerLatch);
> > +
..
> > }
> > - UnlockBufHdr(buf);
> > }
>
> I think it makes sense to break out this bit into its own
> function. That'll make StrategyGetBuffer() a good bit easier to read.

I will try to move it into a new function GetBufferFromFreelist().

> > /* Nothing on the freelist, so run the "clock sweep" algorithm */
> > trycounter = NBuffers;
> > +
...
> > + buf = &BufferDescriptors[next_victim];
>
> I'd also move this into its own function, but thats's more debatable.

As we have not touched much of this part of code, so lets refactor
this code separately if required.

> > +/*
> > + * StrategyMoveBufferToFreeListEnd: put a buffer on the end of freelist
> > + */
> > +bool
> > +StrategyMoveBufferToFreeListEnd(volatile BufferDesc *buf)
> > +{
>
> Should maybe rather be named *Tail?

Tail is better suited than End, I will change it.

> > + bool freed = false;
> > + SpinLockAcquire(&StrategyControl->freelist_lck);
> > +
> > + /*
> > + * It is possible that we are told to put something in the
freelist that
> > + * is already in it; don't screw up the list if so.
> > + */
>
> When/Why is that possible?

As we are doing clocksweep which can come across the same buffer
again incase it didn't find sufficient buffers to put on freelist in one
cycle, so this function needs to ensure the same.

> > +void
> > +StrategyGetFreelistAccessInfo(uint32 *num_buf_to_free, uint32
*num_buf_alloc,
> > + uint32
*num_buf_backend_clocksweep)
> > +
> > + if (num_buf_alloc)
> > + {
> > + *num_buf_alloc = StrategyControl->numBufferAllocs;
> > + StrategyControl->numBufferAllocs = 0;
> > + }
> > + if (num_buf_backend_clocksweep)
> > + {
> > + *num_buf_backend_clocksweep =
StrategyControl->numBufferBackendClocksweep;
> > + StrategyControl->numBufferBackendClocksweep = 0;
> > + }
> > + SpinLockRelease(&StrategyControl->freelist_lck);
> > +
> > + return;
> > +}
>
> Do we need the if (num_buf_alloc) bits? Can't we make it unconditional?

This API is more or less designed on lines of StrategySyncStart().
Currently there is no case where we can't do it unconditional (same is
true for num_buf_backend_clocksweep), however there is some merit
to keep it in sync with existing API. Having said that if you feel we
should
go about doing it unconditionally, I will change it in next patch.

>
> Some more general remarks:
> * I think there's a fair amount of unexplained heuristics in here

I think after addressing the comments given by you above,
the situation will be better.

> * Are we sure that the freelist_lck spinlock won't cause pain? Right now
> there will possibly be dozens of processes busily spinning on it... I
> think it's a acceptable risk, but we should think about it.

The situation seems to be better by having spinlock, rather than
by using LWLock, however if there is anything that causes pain,
I think we might want to consider partitioning the free list, but lets
keep that for other day.

> * I'm not convinced that these changes can be made without also changing
> the bgwriter logic. Have you measured whether there are differences in
> how effective the bgwriter is? Not that it's very effective right now :)

Current changes doesn't do anything to make bgwriter more or
less effective, however as discussed elsewhere in thread that it
makes sense for bgreclaimer to notify bgwriter in some situtions,
that can make bgwriter more effective than now, however as concluded
there lets do it in separate patch.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Sep 11, 2014 at 4:22 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > Hm. Perhaps we should do a bufHdr->refcount != zero check without
>> > locking here? The atomic op will transfer the cacheline exclusively to
>> > the reclaimer's CPU. Even though it very shortly afterwards will be
>> > touched afterwards by the pinning backend.
>>
>> Meh. I'm not in favor of adding more funny games with locking unless
>> we can prove they're necessary for performance.
>
> Well, this in theory increases the number of processes touching buffer
> headers regularly. Currently, if you have one read IO intensive backend,
> there's pretty much only process touching the cachelines. This will make
> it two. I don't think it's unreasonable to try to reduce the cacheline
> pingpong caused by that...

I don't think it will help much. A pinned buffer is pretty likely to
be in modified state in the cache of the cpu of the pinning backend.
Even taking a look at the refcount will trigger a writeback and
demotion to shared state. When time comes to unpin the buffer the
cacheline must again be promoted to exclusive state introducing
coherency traffic. Not locking the buffer only saves transfering the
cacheline back to the pinning backend, not a huge amount of savings.

Regards,
Ants Aasma
--
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de

On 2014-09-12 12:38:48 +0300, Ants Aasma wrote:
> On Thu, Sep 11, 2014 at 4:22 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> > Hm. Perhaps we should do a bufHdr->refcount != zero check without
> >> > locking here? The atomic op will transfer the cacheline exclusively to
> >> > the reclaimer's CPU. Even though it very shortly afterwards will be
> >> > touched afterwards by the pinning backend.
> >>
> >> Meh. I'm not in favor of adding more funny games with locking unless
> >> we can prove they're necessary for performance.
> >
> > Well, this in theory increases the number of processes touching buffer
> > headers regularly. Currently, if you have one read IO intensive backend,
> > there's pretty much only process touching the cachelines. This will make
> > it two. I don't think it's unreasonable to try to reduce the cacheline
> > pingpong caused by that...
>
> I don't think it will help much. A pinned buffer is pretty likely to
> be in modified state in the cache of the cpu of the pinning backend.

Right. Unless you're on a MOESI platforms. I'd really like to know why
that's not more widely used.

> Even taking a look at the refcount will trigger a writeback and
> demotion to shared state. When time comes to unpin the buffer the
> cacheline must again be promoted to exclusive state introducing
> coherency traffic. Not locking the buffer only saves transfering the
> cacheline back to the pinning backend, not a huge amount of savings.

Yes. But: In many, if not most, cases the cacheline will be read a
couple times before modifying it via the spinlock.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 9/11/14, 7:01 AM, Andres Freund wrote:
> I'm not convinced that these changes can be made without also changing
> the bgwriter logic. Have you measured whether there are differences in
> how effective the bgwriter is? Not that it's very effective right now :)

The current background writer tuning went out of its way to do nothing
when it wasn't clear there was something that always worked. What
happened with all of the really clever schemes was that they worked on
some workloads, and just trashed others. Most of the gain from the 8.3
rewrite came from looking at well theorized ideas for how to handle
things like pre-emptive LRU scanning for writes, and just throwing them
out altogether in favor of ignoring the problem. The magic numbers left
in or added to the code were tuned to do very little work,
intentionally. If anything, since then the pressure to do nothing has
gone up in the default install, because now people are very concerned
about extra wakeups using power.

To find bad cases before, I was running about 30 different test
combinations by the end, Heikki was running another set in the EDB lab,
I believe there was a lab at NTT running their own set too. What went in
was the combination that didn't break any of them badly--not the one
that performed best on the good workloads.

This looks like it's squashed one of the very fundamental buffer scaling
issues though; well done Amit. What I'd like to see is preserving the
heart of that while touching as little as possible. When in doubt, do
nothing; let the backends suck it up and do the work themselves.

I had to take a health break from community development for a while, and
I'm hoping to jump back into review again for the rest of the current
development cycle. I'll go back to my notes and try to recreate the
pathological cases that plagued both the 8.3 BGW rewrite and the aborted
9.2 fsync spreading effort I did; get those running again and see how
they do on this new approach. I have a decent sized 24 core server that
should be good enough for this job. I'll see what I can do.

--
Greg Smith greg(dot)smith(at)crunchydatasolutions(dot)com
Chief PostgreSQL Evangelist - http://crunchydatasolutions.com/

On Fri, Sep 12, 2014 at 11:55 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> On Thu, Sep 11, 2014 at 4:31 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:
> > On 2014-09-10 12:17:34 +0530, Amit Kapila wrote:
> > > +++ b/src/backend/postmaster/bgreclaimer.c
> >
> > A fair number of comments in that file refer to bgwriter...
>
> Will fix.

Fixed.

> > > @@ -0,0 +1,302 @@
> > >
+/*-------------------------------------------------------------------------
> > > + *
> > > + * bgreclaimer.c
> > > + *
> > > + * The background reclaimer (bgreclaimer) is new as of Postgres 9.5.
It
> > > + * attempts to keep regular backends from having to run clock sweep
(which
> > > + * they would only do when they don't find a usable shared buffer
from
> > > + * freelist to read in another page).
> >
> > That's not really accurate. Freelist pages are often also needed to
> > write new pages, without reading anything in.
>
> Agreed, but the same is used in bgwriter file as well; so if we
> change here, we might want to change bgwriter file header as well.

I have just changed bgreclaimer for this comment, if the same
is required for bgwriter, I can create a separate patch as that
change is not related to this patch, so I thought it is better
to keep it separate.

> > I'd phrase it as "which
> > they only need to do if they don't find a victim buffer from the
> > freelist"
>
> victim buffer sounds more like a buffer which it will get from
> clock sweep, how about next candidate (same is used in function
> header of StrategyGetBuffer()).

Fixed.

> > > In the best scenario all requests
> > > + * for shared buffers will be fulfilled from freelist as the
background
> > > + * reclaimer process always tries to maintain buffers on freelist.
However,
> > > + * regular backends are still empowered to run clock sweep to find a
usable
> > > + * buffer if the bgreclaimer fails to maintain enough buffers on
freelist.
> >
> > "empowered" sounds strange to me. 'still can run the clock sweep'?
>
> No harm in changing like what you are suggesting, however the same is
> used in file header of bgwriter.c as well, so I think lets keep this
usage as
> it is because there is no correctness issue here.

Not changed anything for this comment.

> >
> > > +static void bgreclaim_quickdie(SIGNAL_ARGS);
> > > +static void BgreclaimSigHupHandler(SIGNAL_ARGS);
> > > +static void ReqShutdownHandler(SIGNAL_ARGS);
> > > +static void bgreclaim_sigusr1_handler(SIGNAL_ARGS);
> >
> > This looks inconsistent.
>
> I have kept based on bgwriter, so not sure if it's good to change.
> However I we want consistent in naming, I would like to keep
> something like:
>
> ReclaimShutdownHandler
> ReclaimQuickDieHandler
> ..
> ..

Changed function names to make them consistent.

> > > + /*
> > > + * If an exception is encountered, processing resumes here.
> > > + *
> > > + * See notes in postgres.c about the design of this coding.
> > > + */
> > > + if (sigsetjmp(local_sigjmp_buf, 1) != 0)
> > > + {
> > > + /* Since not using PG_TRY, must reset error stack by
hand */
> > > + error_context_stack = NULL;
> ..
> >
> > No LWLockReleaseAll(), AbortBufferIO(), ...? Unconvinced that that's a
> > good idea, regardless of it possibly being true today (which I'm not
> > sure about yet).
>
> I will add LWLockReleaseAll() in exception handling as discussed
> elsewhere in thread.

Done.

>
> > > +
> > > + /* Now we can allow interrupts again */
> > > + RESUME_INTERRUPTS();
> >
> > Other processes sleep for a second here, I think that's a good
> > idea. E.g. that bit:
>
> Agreed, will make change as per suggestion.

Done.

> >
> > > + /*
> > > + * Loop forever
> > > + */
> > > + for (;;)
> > > + {
> > > + int rc;
> > > +
> > > +
> > > + /*
> > > + * Backend will signal bgreclaimer when the number of
buffers in
> > > + * freelist falls below than low water mark of freelist.
> > > + */
> > > + rc = WaitLatch(&MyProc->procLatch,
> > > + WL_LATCH_SET |
WL_POSTMASTER_DEATH,
> > > + -1);
> >
> > That's probably not going to work well directly after a (re)start of
> > bgreclaim (depending on how you handle the water mark, I'll see in a
> > bit).
>
> Could you please be more specific here?

I wasn't sure if any change is required here, so kept the code
as it is.

> >
> > > +Background Reclaimer's Processing
> > > +---------------------------------
> ..
> > > +Two water mark indicators are used to maintain sufficient number of
buffers
> > > +on freelist. Low water mark indicator is used by backends to wake
bgreclaimer
> > > +when the number of buffers in freelist falls below it. High water
mark
> > > +indicator is used by bgreclaimer to move buffers to freelist.
> >
> > For me the description of the high water as stated here doesn't seem to
> > explain anything.
> >
> > This section should have a description of how the reclaimer interacts
> > with the bgwriter logic. Do we put dirty buffers on the freelist that
> > are then cleaned by the bgwriter? Which buffers does the bgwriter write
> > out?
>
> As discussed in thread, I will change this accordingly.

Done

> > Move buffers with reference and a usage_count *of* zero to freelist. By
> > maintaining enough buffers in the freelist (up to the list's high water
> > mark), we drastically reduce the likelihood of individual backends
> > having to perform the clock sweep themselves.
>
> Okay will rephrase it as per your suggestion.

Done

> > > + * This is called by the background reclaim process when the number
> > > + * of buffers in freelist falls below low water mark of freelist.
> > > + */
> >
> > The logic used here *definitely* needs to be documented in another form
> > somewhere in the source.
>
> I think the way Robert has suggested to modify Readme adresses
> this to an extent, however if you think it is better to go in more
> detail, then I can expand function header of function
> BgMoveBuffersToFreelist() or on top of bgreclaimer.c, what do you prefer?
>
> > > +
> > > + if (tmp_num_to_free == 0)
> > > + break;
> >
> > num_to_free isn't a convincing name if I understand what this is doing
> > correctly. Maybe 'move_to_freelist', 'freelist_needed',
> > 'needed_on_freelist' or something like that?
>
> I think keeping num or count in name could be more helpful as it is used
> to loop for finding usable buffers. How about 'num_needed_on_freelist',
> without any count or num it sounds more like a boolean variable.

Changed.

> >
> > I wonder if we don't want to increase the high watermark when
> > tmp_recent_backend_clocksweep > 0?
> >
> > > + while (tmp_num_to_free > 0)
> > > + {
> ..
> > > + /*
> > > + * If the buffer is pinned or has a nonzero
usage_count, we cannot
> > > + * move it to freelist; decrement the
usage_count (unless pinned)
> > > + * and keep scanning.
> > > + */
> > > + LockBufHdr(bufHdr);
> >
> > Hm. Perhaps we should do a bufHdr->refcount != zero check without
> > locking here? The atomic op will transfer the cacheline exclusively to
> > the reclaimer's CPU. Even though it very shortly afterwards will be
> > touched afterwards by the pinning backend.
>
> As per discussion, the conclusion seems to be that we can do some
> more tests to see if we need such a change, I will do more tests on
> the lines suggested by Robert in below mails and then we can decide
> if any thing is required.

Still performance data related to this needs to be collected.

> > > +/*
> > > + * Water mark indicators for maintaining buffers on freelist. When
the
> > > + * number of buffers on freelist drops below the low water mark, the
> > > + * allocating backend sets the latch and bgreclaimer wakesup and
begin
> > > + * adding buffer's to freelist until it reaches high water mark and
then
> > > + * again goes back to sleep.
> > > + */
> >
> > s/wakesup/wakes up/; s/begin adding/begins adding/; s/buffer's/buffers/;
> > /to freelist/to the freelist/; s/reaches high water/reaches the high
water/
>
> Will change as per suggestions.

Done

> > > +int freelistLowWaterMark;
> > > +int freelistHighWaterMark;
> > > +
> > > +/* Percentage indicators for maintaining buffers on freelist */
> > > +#define HIGH_WATER_MARK_FREELIST_BUFFERS_PERCENT 0.005
> > > +#define LOW_WATER_MARK_FREELIST_BUFFERS_PERCENT 0.2
> > > +#define MIN_HIGH_WATER_MARK 5
> > > +#define MAX_HIGH_WATER_MARK 2000
> >
> > I'm confused. The high water mark percentage is smaller than the low
> > water mark?
>
> High water mark is percentage of NBuffers (shared_buffers).
> Low water mark is percentage of High water mark.
>
> I will add a comment.

Added a comment.

>
>
> > > - *lock_held = true;
> > > - LWLockAcquire(BufFreelistLock, LW_EXCLUSIVE);
> > > + SpinLockAcquire(&StrategyControl->freelist_lck);
> > >
> ..
> > > + bgreclaimerLatch = StrategyControl->bgreclaimerLatch;
> > > bgwriterLatch = StrategyControl->bgwriterLatch;
> >
> > I don't understand why these need to be grabbed under the spinlock?
>
> In earlier version of patch, it was done without spinklock,
> however Robert has given comment that as it was previously done
> with BufFreelist lock, these should be under spinlock (atleast
> that is what I understood) and I tested the patch again by having
> them under spinlock and didn't find any difference, so I have moved
> them under spinlock.

Not changed anything related to this.

> ..
> > > + if (numFreeListBuffers < freelistLowWaterMark &&
bgreclaimerLatch)
> > > + SetLatch(StrategyControl->bgreclaimerLatch);
> > > +
> ..
> > > }
> > > - UnlockBufHdr(buf);
> > > }
> >
> > I think it makes sense to break out this bit into its own
> > function. That'll make StrategyGetBuffer() a good bit easier to read.
>
> I will try to move it into a new function GetBufferFromFreelist().

Moved this part of code into new function.

>
> > > +/*
> > > + * StrategyMoveBufferToFreeListEnd: put a buffer on the end of
freelist
> > > + */
> > > +bool
> > > +StrategyMoveBufferToFreeListEnd(volatile BufferDesc *buf)
> > > +{
> >
> > Should maybe rather be named *Tail?
>
> Tail is better suited than End, I will change it.

Changed.

> > > +void
> > > +StrategyGetFreelistAccessInfo(uint32 *num_buf_to_free, uint32
*num_buf_alloc,
> > > + uint32
*num_buf_backend_clocksweep)
> > > +
> > > + if (num_buf_alloc)
> > > + {
> > > + *num_buf_alloc = StrategyControl->numBufferAllocs;
> > > + StrategyControl->numBufferAllocs = 0;
> > > + }
> > > + if (num_buf_backend_clocksweep)
> > > + {
> > > + *num_buf_backend_clocksweep =
StrategyControl->numBufferBackendClocksweep;
> > > + StrategyControl->numBufferBackendClocksweep = 0;
> > > + }
> > > + SpinLockRelease(&StrategyControl->freelist_lck);
> > > +
> > > + return;
> > > +}
> >
> > Do we need the if (num_buf_alloc) bits? Can't we make it unconditional?
>
> This API is more or less designed on lines of StrategySyncStart().
> Currently there is no case where we can't do it unconditional (same is
> true for num_buf_backend_clocksweep), however there is some merit
> to keep it in sync with existing API. Having said that if you feel we
should
> go about doing it unconditionally, I will change it in next patch.

I have made it unconditional.

> Hm, right. But then let's move BgWriterStats.m_buf_alloc =+,
> ... pgstat_send_bgwriter(); into that loop. Otherwise it'd possibly end
> up being continously busy without being visible.

Done and I have removed pgstat_send_bgwriter() call from
bgreclaimer loop, as after this change calling it there becomes
redundant.

Apart from this, I have changed kid of newly added function as
due to recent commit, the oid I was using is no longer available.

I have taken tpc-b data as well, it is with previous version of patch,
however the recent version hasn't changed much to impact performance
data. It takes long time to get tpc-b data, thats why I could not report
it previously.

I will post the data with the latest patch separately (where I will focus
on new cases discussed between Robert and Andres).

TPC-B Performance Data
----------------------------------------
Configuration and Db Details
IBM POWER-7 16 cores, 64 hardware threads
RAM = 64GB
Database Locale =C
checkpoint_segments=256
checkpoint_timeout =15min
wal_buffers = 256MB
shared_buffers=8GB
scale factor = 3000
Client Count = number of concurrent sessions and threads (ex. -c 8 -j 8)
Duration of each individual run = 30mins

Client_count/Patch_ver 32 64 128 HEAD 420 535 556 Patch (sbe_v8) 431 537
568

About performance data
-------------------------------------
1. This data is a median of 3 runs, individual run data can
be found in attached document (perf_read_scalability_data_v8.ods)
2. There is not much difference in performance between Head and patch
which shows that this patch hasn't regressed tpc-b case.

Steps to take tpc-b data, for each run:
1. start server
2. drop db
3. create db
4. initialize db
5. run tpc-b pgbench (used prepared statement)
6. checkpoint
7. stop server

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Sep 12, 2014 at 11:09 PM, Gregory Smith <gregsmithpgsql(at)gmail(dot)com>
wrote:

> This looks like it's squashed one of the very fundamental buffer
> scaling issues though; well done Amit.

Thanks.

> I'll go back to my notes and try to recreate the pathological cases
> that plagued both the 8.3 BGW rewrite and the aborted 9.2 fsync
> spreading effort I did; get those running again and see how they
> do on this new approach. I have a decent sized 24 core server
> that should be good enough for this job. I'll see what I can do.

It will be really helpful if you can try out those cases.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 14/09/14 19:00, Amit Kapila wrote:
> On Fri, Sep 12, 2014 at 11:09 PM, Gregory Smith
> <gregsmithpgsql(at)gmail(dot)com <mailto:gregsmithpgsql(at)gmail(dot)com>> wrote:
>
> > This looks like it's squashed one of the very fundamental buffer
> > scaling issues though; well done Amit.
>
> Thanks.
>
> > I'll go back to my notes and try to recreate the pathological cases
> > that plagued both the 8.3 BGW rewrite and the aborted 9.2 fsync
> > spreading effort I did; get those running again and see how they
> > do on this new approach. I have a decent sized 24 core server
> > that should be good enough for this job. I'll see what I can do.
>
> It will be really helpful if you can try out those cases.
>
>

And if you want 'em run on the 60 core beast, just let me know the
details and I'll do some runs for you.

Cheers

Mark

On Sun, Sep 14, 2014 at 12:23 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

> On Fri, Sep 12, 2014 at 11:55 AM, Amit Chapel <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> > On Thu, Sep 11, 2014 at 4:31 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
> wrote:
> > > On 2014-09-10 12:17:34 +0530, Amit Kapila wrote:
>
> I will post the data with the latest patch separately (where I will focus
> on new cases discussed between Robert and Andres).
>
>
Performance Data with latest version of patch.
All the data shown below is a median of 3 runs, for each
individual run data, refer attached document
(perf_read_scalability_data_v9.ods)

Performance Data for Read-only test
-----------------------------------------------------
Configuration and Db Details
IBM POWER-7 16 cores, 64 hardware threads
RAM = 64GB
Database Locale =C
checkpoint_segments=256
checkpoint_timeout =15min
shared_buffers=8GB
scale factor = 3000
Client Count = number of concurrent sessions and threads (ex. -c 8 -j 8)
Duration of each individual run = 5mins

All the data is in tps and taken using pgbench read-only load

Client_Count/Patch_Ver 8 16 32 64 128 HEAD 58614 107370 140717 104357
65010 sbe_v9 62943 119064 172246 220174 220904

Observations
---------------------
1. It scales well as with previous versions of patch, but
it seems the performance is slightly better in few cases,
may be because I have removed a statement (if check)
or 2 in bgreclaimer (those were done under spinlock) or it
could be just run-to-run difference.

> (1) A read-only pgbench workload that is just a tiny bit larger than
> shared_buffers, say size of shared_buffers plus 0.01%. Such workloads
> tend to stress buffer eviction heavily.

When the data is just tiny bit larger than shared buffers, actually
there is no problem in scalability even in HEAD, because I think
most of the requests will be satisfied from existing buffer pool.
I have taken data for some of the loads where database size is
bit larger than shared buffers and it is as follows:

Scale Factor - 800
Shared_Buffers - 12286MB (Total db size is 12288MB)

Client_Count/Patch_Ver 1 8 16 32 64 128 HEAD 8406 68712 132222 198481
290340 289828 sbe_v9 8504 68546 131926 195789 289959 289021

Scale Factor - 800
Shared_Buffers - 12166MB (Total db size is 12288MB)

Client_Count/Patch_Ver 1 8 16 32 64 128 HEAD 8428 68609 128092 196596
292066 293812 sbe_v9 8386 68546 126926 197126 289959 287621

Observations
---------------------
In most cases performance with patch is slightly less as compare
to HEAD and the difference is generally less than 1% and in a case
or 2 close to 2%. I think the main reason for slight difference is that
when the size of shared buffers is almost same as data size, the number
of buffers it needs from clock sweep are very less, as an example in first
case (when size of shared buffers is 12286MB), it actually needs at most
256 additional buffers (2MB) via clock sweep, where as bgreclaimer
will put 2000 (high water mark) additional buffers (0.5% of shared buffers
is greater than 2000 ) in free list, so bgreclaimer does some extra work
when it is not required and it also leads to condition you mentioned
down (freelist will contain buffers that have already been touched since
we added them). Now for case 2 (12166MB), we need buffers more
than 2000 additional buffers, but not too many, so it can also have
similar effect.

I think we have below options related to this observation
a. Some further tuning in bgreclaimer, so that instead of putting
the buffers up to high water mark in freelist, it puts just 1/4th or
1/2 of high water mark and then check if the free list still contains
lesser than equal to low water mark, if yes it continues and if not
then it can wait (or may be some other way).
b. Instead of waking bgreclaimer when the number of buffers fall
below low water mark, wake when the number of times backends
does clock sweep crosses certain threshold
c. Give low and high water mark as config knobs, so that in some
rare cases users can use them to do tuning.
d. Lets not do anything as if user does such a configuration, he should
be educated to configure shared buffers in a better way and or the
performance hit doesn't seem to be justified to do any further
work.

Now if we do either of 'a' or 'b', then I think there is a chance
that the gain might not be same for cases where users can
easily get benefit from this patch and there is a chance that
it degrades the performance in some other case.

> (2) A workload that maximizes the rate of concurrent buffer eviction
> relative to other tasks. Read-only pgbench is not bad for this, but
> maybe somebody's got a better idea.

I think the first test of pgbench (scale_factor-3000;shared_buffers-8GB)
addresses this case.

> As I sort of mentioned in what I was writing for the bufmgr README,
> there are, more or less, three ways this can fall down, at least that
> I can see: (1) if the high water mark is too high, then we'll start
> finding buffers in the freelist that have already been touched since
> we added them:

I think I am able to see this effect (though mild) in one of above tests.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Sep 16, 2014 at 8:18 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

> In most cases performance with patch is slightly less as compare
> to HEAD and the difference is generally less than 1% and in a case
> or 2 close to 2%. I think the main reason for slight difference is that
> when the size of shared buffers is almost same as data size, the number
> of buffers it needs from clock sweep are very less, as an example in first
> case (when size of shared buffers is 12286MB), it actually needs at most
> 256 additional buffers (2MB) via clock sweep, where as bgreclaimer
> will put 2000 (high water mark) additional buffers (0.5% of shared buffers
> is greater than 2000 ) in free list, so bgreclaimer does some extra work
> when it is not required and it also leads to condition you mentioned
> down (freelist will contain buffers that have already been touched since
> we added them). Now for case 2 (12166MB), we need buffers more
> than 2000 additional buffers, but not too many, so it can also have
> similar effect.
>

So there are two suboptimal things that can happen and they pull in
opposite directions. I think you should instrument the server how often
each is happening. #1 is that we can pop a buffer from the freelist and
find that it's been touched. That means we wasted the effort of putting it
on the freelist in the first place. #2 is that we can want to pop a buffer
from the freelist and find it empty and thus be forced to run the clock
sweep ourselves. If we're having problem #1, we could improve things by
reducing the water marks. If we're having problem #2, we could improve
things by increasing the water marks. If we're having both problems, then
I dunno. But let's get some numbers on the frequency of these specific
things, rather than just overall tps numbers.

> I think we have below options related to this observation
> a. Some further tuning in bgreclaimer, so that instead of putting
> the buffers up to high water mark in freelist, it puts just 1/4th or
> 1/2 of high water mark and then check if the free list still contains
> lesser than equal to low water mark, if yes it continues and if not
> then it can wait (or may be some other way).
>

That sounds suspiciously like just reducing the high water mark.

> b. Instead of waking bgreclaimer when the number of buffers fall
> below low water mark, wake when the number of times backends
> does clock sweep crosses certain threshold
>

That doesn't sound helpful.

> c. Give low and high water mark as config knobs, so that in some
> rare cases users can use them to do tuning.
>

Yuck.

> d. Lets not do anything as if user does such a configuration, he should
> be educated to configure shared buffers in a better way and or the
> performance hit doesn't seem to be justified to do any further
> work.
>

At least worth entertaining.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Sep 16, 2014 at 10:21 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, Sep 16, 2014 at 8:18 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>
>> In most cases performance with patch is slightly less as compare
>> to HEAD and the difference is generally less than 1% and in a case
>> or 2 close to 2%. I think the main reason for slight difference is that
>> when the size of shared buffers is almost same as data size, the number
>> of buffers it needs from clock sweep are very less, as an example in first
>> case (when size of shared buffers is 12286MB), it actually needs at most
>> 256 additional buffers (2MB) via clock sweep, where as bgreclaimer
>> will put 2000 (high water mark) additional buffers (0.5% of shared buffers
>> is greater than 2000 ) in free list, so bgreclaimer does some extra work
>> when it is not required and it also leads to condition you mentioned
>> down (freelist will contain buffers that have already been touched since
>> we added them). Now for case 2 (12166MB), we need buffers more
>> than 2000 additional buffers, but not too many, so it can also have
>> similar effect.
>>
>
> So there are two suboptimal things that can happen and they pull in
> opposite directions. I think you should instrument the server how often
> each is happening. #1 is that we can pop a buffer from the freelist and
> find that it's been touched. That means we wasted the effort of putting it
> on the freelist in the first place. #2 is that we can want to pop a buffer
> from the freelist and find it empty and thus be forced to run the clock
> sweep ourselves. If we're having problem #1, we could improve things by
> reducing the water marks. If we're having problem #2, we could improve
> things by increasing the water marks. If we're having both problems, then
> I dunno. But let's get some numbers on the frequency of these specific
> things, rather than just overall tps numbers.
>

Specific numbers of both the configurations for which I have
posted data in previous mail are as follows:

Scale Factor - 800
Shared_Buffers - 12286MB (Total db size is 12288MB)
Client and Thread Count = 64
buffers_touched_freelist - count of buffers that backends found touched
after
popping from freelist.
buffers_backend_clocksweep - count of buffer allocations not satisfied from
freelist

buffers_alloc 1531023 buffers_backend_clocksweep 0
buffers_touched_freelist 0

Scale Factor - 800
Shared_Buffers - 12166MB (Total db size is 12288MB)
Client and Thread Count = 64

buffers_alloc 1531010 buffers_backend_clocksweep 0
buffers_touched_freelist 0

In both the above cases, I have taken data multiple times to ensure
correctness. From the above data, it is evident that in both the above
configurations all the requests are satisfied from the initial freelist.
Basically the amount of shared buffers configured
(12286MB = 1572608 buffers and 12166MB = 1557248 buffers) are
sufficient to contain all the work load for pgbench run.

So now the question is why we are seeing small variation (<1%) in data
in case all the data fits in shared buffers and the reason could be that
we have added few extra instructions (due to increase in StrategyControl
structure size, additional function call, one or two new assignments) in the
Buffer Allocation path (the extra instructions will also be only till all
the data
pages gets associated with buffers, after that the control won't even reach
StrategyGetBuffer()) or it may be due to variation across different runs
with
different binaries.

I have went ahead to take the data in cases shared buffers are tiny bit
(0.1%
and .05%) less than workload (based on buffer allocations done in above
cases).

Performance Data
-------------------------------

Scale Factor - 800
Shared_Buffers - 11950MB

Client_Count/Patch_Ver 8 16 32 64 128 HEAD 68424 132540 195496 279511
283280 sbe_v9 68565 132709 194631 284351 289333

Scale Factor - 800
Shared_Buffers - 11955MB

Client_Count/Patch_Ver 8 16 32 64 128 HEAD 68331 127752 196385 274387
281753 sbe_v9 68922 131314 194452 284292 287221

The above data indicates that performance is better with patch
in almost all cases and especially at high concurrency (64 and
128 client count).

The overall conclusion is that with patch
a. when the data can fit in RAM and not completely in shared buffers,
the performance/scalability is quite good even if shared buffers are just
tiny bit less that all the data.
b. when shared buffers are sufficient to contain all the data, then there is
a slight difference (<1%) in performance.

>
>> d. Lets not do anything as if user does such a configuration, he should
>> be educated to configure shared buffers in a better way and or the
>> performance hit doesn't seem to be justified to do any further
>> work.
>>
>
> At least worth entertaining.
>
> Based on further analysis, I think this is the way to go.

Attached find the patch for new stat (buffers_touched_freelist) just in
case you want to run the patch with it and detailed (individual run)
performance data.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sun, Sep 14, 2014 at 12:23 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
> Apart from this, I have changed kid of newly added function as
> due to recent commit, the oid I was using is no longer available.

After sending last patch I have realized that oid used in patch for
new function is wrong; fixed the same in attached patch.

I have fixed all the comments raised till now for patch and
provided the latest performance data as well, so I will mark it
as "Needs Review" to proceed further.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 9/16/14, 8:18 AM, Amit Kapila wrote:
> I think the main reason for slight difference is that
> when the size of shared buffers is almost same as data size, the number
> of buffers it needs from clock sweep are very less, as an example in first
> case (when size of shared buffers is 12286MB), it actually needs at most
> 256 additional buffers (2MB) via clock sweep, where as bgreclaimer
> will put 2000 (high water mark) additional buffers (0.5% of shared buffers
> is greater than 2000 ) in free list, so bgreclaimer does some extra work
> when it is not required
This is exactly what I was warning about, as the sort of lesson learned
from the last round of such tuning. There are going to be spots where
trying to tune the code to be aggressive on the hard cases will work
great. But you need to make that dynamic to some degree, such that the
code doesn't waste a lot of time sweeping buffers when the demand for
them is actually weak. That will make all sorts of cases that look like
this slower.

We should be able to tell these apart if there's enough instrumentation
and solid logic inside of the program itself though. The 8.3 era BGW
coped with a lot of these issues using a particular style of moving
average with fast reaction time, plus instrumenting the buffer
allocation rate as accurately as it could. So before getting into
high/low water note questions, are you comfortable that there's a clear,
accurate number that measures the activity level that's important here?
And have you considered ways it might be averaging over time or have a
history that's analyzed? The exact fast approach / slow decay weighted
moving average approach of the 8.3 BGW, the thing that tried to smooth
the erratic data set possible here, was a pretty critical part of
getting itself auto-tuning to workload size. It ended up being much
more important than the work of setting the arbitrary watermark levels.