Add statistics to pg_stat_wal view for wal related parameter tuning

On Tue, Oct 20, 2020 at 8:01 AM Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote:

Hi,

I think we need to add some statistics to pg_stat_wal view.

Although there are some parameter related WAL,
there are few statistics for tuning them.

I think it's better to provide the following statistics.
Please let me know your comments.

```
postgres=# SELECT * from pg_stat_wal;
-[ RECORD 1 ]-------+------------------------------
wal_records | 2000224
wal_fpi | 47
wal_bytes | 248216337
wal_buffers_full | 20954
wal_init_file | 8
wal_write_backend | 20960
wal_write_walwriter | 46
wal_write_time | 51
wal_sync_backend | 7
wal_sync_walwriter | 8
wal_sync_time | 0
stats_reset | 2020-10-20 11:04:51.307771+09
```

1. Basic statistics of WAL activity

- wal_records: Total number of WAL records generated
- wal_fpi: Total number of WAL full page images generated
- wal_bytes: Total amount of WAL bytes generated

To understand DB's performance, first, we will check the performance
trends for the entire database instance.
For example, if the number of wal_fpi becomes higher, users may tune
"wal_compression", "checkpoint_timeout" and so on.

Although users can check the above statistics via EXPLAIN, auto_explain,
autovacuum and pg_stat_statements now,
if users want to see the performance trends for the entire database,
they must recalculate the statistics.

Here, do you mean to say 'entire cluster' instead of 'entire database'
because it seems these stats are getting collected for the entire
cluster?

I think it is useful to add the sum of the basic statistics.

There is an argument that it is better to view these stats at the
statement-level so that one can know which statements are causing most
WAL and then try to rate-limit them if required in the application and
anyway they can get the aggregate of all the WAL if they want. We have
added these stats in PG-13, so do we have any evidence that the
already added stats don't provide enough information? I understand
that you are trying to display the accumulated stats here which if
required users/DBA need to compute with the currently provided stats.
OTOH, sometimes adding more ways to do some things causes difficulty
for users to understand and learn.

I see that we also need to add extra code to capture these stats (some
of which is in performance-critical path especially in
XLogInsertRecord) which again makes me a bit uncomfortable. It might
be that it is all fine as it is very important to collect these stats
at cluster-level in spite that the same information can be gathered at
statement-level to help customers but I don't see a very strong case
for that in your proposal.

--
With Regards,
Amit Kapila.

On 2020-10-20 12:46, Amit Kapila wrote:

On Tue, Oct 20, 2020 at 8:01 AM Masahiro Ikeda
<ikedamsh@oss.nttdata.com> wrote:

Hi,

I think we need to add some statistics to pg_stat_wal view.

Although there are some parameter related WAL,
there are few statistics for tuning them.

I think it's better to provide the following statistics.
Please let me know your comments.

```
postgres=# SELECT * from pg_stat_wal;
-[ RECORD 1 ]-------+------------------------------
wal_records | 2000224
wal_fpi | 47
wal_bytes | 248216337
wal_buffers_full | 20954
wal_init_file | 8
wal_write_backend | 20960
wal_write_walwriter | 46
wal_write_time | 51
wal_sync_backend | 7
wal_sync_walwriter | 8
wal_sync_time | 0
stats_reset | 2020-10-20 11:04:51.307771+09
```

1. Basic statistics of WAL activity

- wal_records: Total number of WAL records generated
- wal_fpi: Total number of WAL full page images generated
- wal_bytes: Total amount of WAL bytes generated

To understand DB's performance, first, we will check the performance
trends for the entire database instance.
For example, if the number of wal_fpi becomes higher, users may tune
"wal_compression", "checkpoint_timeout" and so on.

Although users can check the above statistics via EXPLAIN,
auto_explain,
autovacuum and pg_stat_statements now,
if users want to see the performance trends for the entire database,
they must recalculate the statistics.

Here, do you mean to say 'entire cluster' instead of 'entire database'
because it seems these stats are getting collected for the entire
cluster?

Thanks for your comments.
Yes, I wanted to say 'entire cluster'.

I think it is useful to add the sum of the basic statistics.

There is an argument that it is better to view these stats at the
statement-level so that one can know which statements are causing most
WAL and then try to rate-limit them if required in the application and
anyway they can get the aggregate of all the WAL if they want. We have
added these stats in PG-13, so do we have any evidence that the
already added stats don't provide enough information? I understand
that you are trying to display the accumulated stats here which if
required users/DBA need to compute with the currently provided stats.
OTOH, sometimes adding more ways to do some things causes difficulty
for users to understand and learn.

I agreed that the statement-level stat is important and I understood
that we can
know the aggregated WAL stats of pg_stat_statement view and autovacuum's
log.
But now, WAL stats generated by autovacuum can be output to logs and it
is not
easy to aggregate them. Since WAL writes impacts for the entire cluster,
I thought
it's natural to provide accumulated value.

I see that we also need to add extra code to capture these stats (some
of which is in performance-critical path especially in
XLogInsertRecord) which again makes me a bit uncomfortable. It might
be that it is all fine as it is very important to collect these stats
at cluster-level in spite that the same information can be gathered at
statement-level to help customers but I don't see a very strong case
for that in your proposal.

Also about performance, I thought there are few impacts because it
increments stats in memory. If I can implement to reuse pgWalUsage's
value which already collects these stats, there is no impact in
XLogInsertRecord.
For example, how about pg_stat_wal() calculates the accumulated
value of wal_records, wal_fpi, and wal_bytes to use pgWalUsage's value?

Regards
--
Masahiro Ikeda
NTT DATA CORPORATION

On Tue, Oct 20, 2020 at 12:41 PM Masahiro Ikeda
<ikedamsh@oss.nttdata.com> wrote:

On 2020-10-20 12:46, Amit Kapila wrote:

On Tue, Oct 20, 2020 at 8:01 AM Masahiro Ikeda

1. Basic statistics of WAL activity

- wal_records: Total number of WAL records generated
- wal_fpi: Total number of WAL full page images generated
- wal_bytes: Total amount of WAL bytes generated

To understand DB's performance, first, we will check the performance
trends for the entire database instance.
For example, if the number of wal_fpi becomes higher, users may tune
"wal_compression", "checkpoint_timeout" and so on.

Although users can check the above statistics via EXPLAIN,
auto_explain,
autovacuum and pg_stat_statements now,
if users want to see the performance trends for the entire database,
they must recalculate the statistics.

Here, do you mean to say 'entire cluster' instead of 'entire database'
because it seems these stats are getting collected for the entire
cluster?

Thanks for your comments.
Yes, I wanted to say 'entire cluster'.

I think it is useful to add the sum of the basic statistics.

There is an argument that it is better to view these stats at the
statement-level so that one can know which statements are causing most
WAL and then try to rate-limit them if required in the application and
anyway they can get the aggregate of all the WAL if they want. We have
added these stats in PG-13, so do we have any evidence that the
already added stats don't provide enough information? I understand
that you are trying to display the accumulated stats here which if
required users/DBA need to compute with the currently provided stats.
OTOH, sometimes adding more ways to do some things causes difficulty
for users to understand and learn.

I agreed that the statement-level stat is important and I understood
that we can
know the aggregated WAL stats of pg_stat_statement view and autovacuum's
log.
But now, WAL stats generated by autovacuum can be output to logs and it
is not
easy to aggregate them. Since WAL writes impacts for the entire cluster,
I thought
it's natural to provide accumulated value.

I think it is other way i.e if we would have accumulated stats then it
makes sense to provide those at statement-level because one would like
to know the exact cause of more WAL activity. Say it is due to an
autovacuum or due to the particular set of statements then it would
easier for users to do something about it.

--
With Regards,
Amit Kapila.

I think it's really a more convenient way to collect wal usage information,
with it we can query when I want. Several points on my side:

1. It will be nice If you provide a chance to reset the information in WalStats,
so that we can reset it without restart the database.

2. I think 'wal_write_backend' is mean wal write times happen in
backends. The describe in document is not so clear, suggest rewrite it.

3. I do not think it's a correct describe in document for 'wal_buffers_full'.

4. Quite strange to collect twice in XLogInsertRecord() for xl_tot_len,
m_wal_records, m_wal_fpi.

5. I notice some code in issue_xlog_fsync() function to collect sync info,
a standby may call the issue_xlog_fsync() too, which do not want to
to collect this info. I think this need some change avoid run by standby
side.

Regards,
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca

At Tue, 20 Oct 2020 16:11:29 +0900, Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote in

On 2020-10-20 12:46, Amit Kapila wrote:

I see that we also need to add extra code to capture these stats (some
of which is in performance-critical path especially in
XLogInsertRecord) which again makes me a bit uncomfortable. It might
be that it is all fine as it is very important to collect these stats
at cluster-level in spite that the same information can be gathered at
statement-level to help customers but I don't see a very strong case
for that in your proposal.

We should avoid that duplication as possible even if the both number
are important.

Also about performance, I thought there are few impacts because it
increments stats in memory. If I can implement to reuse pgWalUsage's
value which already collects these stats, there is no impact in
XLogInsertRecord.
For example, how about pg_stat_wal() calculates the accumulated
value of wal_records, wal_fpi, and wal_bytes to use pgWalUsage's
value?

I don't think that works, but it would work that pgstat_send_wal()
takes the difference of that values between two successive calls.

WalUsage prevWalUsage;
...
pgstat_send_wal()
{
..
/* fill in some values using pgWalUsage */
WalStats.m_wal_bytes = pgWalUsage.wal_bytes - prevWalUsage.wal_bytes;
WalStats.m_wal_records = pgWalUsage.wal_records - prevWalUsage.wal_records;
WalStats.m_wal_wal_fpi = pgWalUsage.wal_fpi - prevWalUsage.wal_fpi;
...
pgstat_send(&WalStats, sizeof(WalStats));

/* remember the current numbers */
prevWalUsage = pgWalUsage;

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 2020-10-21 13:41, Amit Kapila wrote:

On Tue, Oct 20, 2020 at 12:41 PM Masahiro Ikeda
<ikedamsh@oss.nttdata.com> wrote:

On 2020-10-20 12:46, Amit Kapila wrote:

On Tue, Oct 20, 2020 at 8:01 AM Masahiro Ikeda

1. Basic statistics of WAL activity

- wal_records: Total number of WAL records generated
- wal_fpi: Total number of WAL full page images generated
- wal_bytes: Total amount of WAL bytes generated

To understand DB's performance, first, we will check the performance
trends for the entire database instance.
For example, if the number of wal_fpi becomes higher, users may tune
"wal_compression", "checkpoint_timeout" and so on.

Although users can check the above statistics via EXPLAIN,
auto_explain,
autovacuum and pg_stat_statements now,
if users want to see the performance trends for the entire database,
they must recalculate the statistics.

Here, do you mean to say 'entire cluster' instead of 'entire database'
because it seems these stats are getting collected for the entire
cluster?

Thanks for your comments.
Yes, I wanted to say 'entire cluster'.

I think it is useful to add the sum of the basic statistics.

There is an argument that it is better to view these stats at the
statement-level so that one can know which statements are causing most
WAL and then try to rate-limit them if required in the application and
anyway they can get the aggregate of all the WAL if they want. We have
added these stats in PG-13, so do we have any evidence that the
already added stats don't provide enough information? I understand
that you are trying to display the accumulated stats here which if
required users/DBA need to compute with the currently provided stats.
OTOH, sometimes adding more ways to do some things causes difficulty
for users to understand and learn.

I agreed that the statement-level stat is important and I understood
that we can
know the aggregated WAL stats of pg_stat_statement view and
autovacuum's
log.
But now, WAL stats generated by autovacuum can be output to logs and
it
is not
easy to aggregate them. Since WAL writes impacts for the entire
cluster,
I thought
it's natural to provide accumulated value.

I think it is other way i.e if we would have accumulated stats then it
makes sense to provide those at statement-level because one would like
to know the exact cause of more WAL activity. Say it is due to an
autovacuum or due to the particular set of statements then it would
easier for users to do something about it.

OK, I'll remove them.
Do you have any comments for other statistics?

--
Masahiro Ikeda
NTT DATA CORPORATION

On 2020-10-21 15:54, lchch1990@sina.cn wrote:

I think it's really a more convenient way to collect wal usage
information,
with it we can query when I want. Several points on my side:

Thanks for your comments.

1. It will be nice If you provide a chance to reset the information in
WalStats,
so that we can reset it without restart the database.

I agree.

Now, pg_stat_wal supports reset all informantion in WalStats
using pg_stat_reset_shared('wal') function.

Isn't it enough?

2. I think 'wal_write_backend' is mean wal write times happen in
backends. The describe in document is not so clear, suggest rewrite
it.

OK, I'll rewrite to "Total number of times backends write WAL data to
the disk".

3. I do not think it's a correct describe in document for
'wal_buffers_full'.

Where should I rewrite the description? If my understanding is not
correct, please let me know.

4. Quite strange to collect twice in XLogInsertRecord() for
xl_tot_len,
m_wal_records, m_wal_fpi.

Yes, Amit-san pointed me that too.
I'll remove them from pg_stat_wal since pg_stat_statements and vacuum
log
already shows the related statistics and there is a comment it's enough.

Anyway, if you need to the accumulated above statistics in pg_stat_wal,
please let me know.

5. I notice some code in issue_xlog_fsync() function to collect sync
info,
a standby may call the issue_xlog_fsync() too, which do not want to
to collect this info. I think this need some change avoid run by
standby
side.

Thanks, I will fix it.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

On 2020-10-21 18:03, Kyotaro Horiguchi wrote:

At Tue, 20 Oct 2020 16:11:29 +0900, Masahiro Ikeda
<ikedamsh@oss.nttdata.com> wrote in

On 2020-10-20 12:46, Amit Kapila wrote:

I see that we also need to add extra code to capture these stats (some
of which is in performance-critical path especially in
XLogInsertRecord) which again makes me a bit uncomfortable. It might
be that it is all fine as it is very important to collect these stats
at cluster-level in spite that the same information can be gathered at
statement-level to help customers but I don't see a very strong case
for that in your proposal.

We should avoid that duplication as possible even if the both number
are important.

Also about performance, I thought there are few impacts because it
increments stats in memory. If I can implement to reuse pgWalUsage's
value which already collects these stats, there is no impact in
XLogInsertRecord.
For example, how about pg_stat_wal() calculates the accumulated
value of wal_records, wal_fpi, and wal_bytes to use pgWalUsage's
value?

I don't think that works, but it would work that pgstat_send_wal()
takes the difference of that values between two successive calls.

WalUsage prevWalUsage;
...
pgstat_send_wal()
{
..
/* fill in some values using pgWalUsage */
WalStats.m_wal_bytes = pgWalUsage.wal_bytes -
prevWalUsage.wal_bytes;
WalStats.m_wal_records = pgWalUsage.wal_records -
prevWalUsage.wal_records;
WalStats.m_wal_wal_fpi = pgWalUsage.wal_fpi -
prevWalUsage.wal_fpi;
...
pgstat_send(&WalStats, sizeof(WalStats));

/* remember the current numbers */
prevWalUsage = pgWalUsage;

Thanks for your advice. This code can avoid the performance impact of
critical code.

By the way, what do you think to add these statistics to the pg_stat_wal
view?
I thought to remove the above statistics because there is advice that
PG13's features,
for example, pg_stat_statement view, vacuum log, and so on can cover
use-cases.

regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

At Thu, 22 Oct 2020 10:44:53 +0900, Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote in

On 2020-10-21 18:03, Kyotaro Horiguchi wrote:

At Tue, 20 Oct 2020 16:11:29 +0900, Masahiro Ikeda
<ikedamsh@oss.nttdata.com> wrote in

On 2020-10-20 12:46, Amit Kapila wrote:

I see that we also need to add extra code to capture these stats (some
of which is in performance-critical path especially in
XLogInsertRecord) which again makes me a bit uncomfortable. It might
be that it is all fine as it is very important to collect these stats
at cluster-level in spite that the same information can be gathered at
statement-level to help customers but I don't see a very strong case
for that in your proposal.

We should avoid that duplication as possible even if the both number
are important.

Also about performance, I thought there are few impacts because it
increments stats in memory. If I can implement to reuse pgWalUsage's
value which already collects these stats, there is no impact in
XLogInsertRecord.
For example, how about pg_stat_wal() calculates the accumulated
value of wal_records, wal_fpi, and wal_bytes to use pgWalUsage's
value?

I don't think that works, but it would work that pgstat_send_wal()
takes the difference of that values between two successive calls.
WalUsage prevWalUsage;
...
pgstat_send_wal()
{
..
/* fill in some values using pgWalUsage */
WalStats.m_wal_bytes = pgWalUsage.wal_bytes - prevWalUsage.wal_bytes;
WalStats.m_wal_records = pgWalUsage.wal_records -
prevWalUsage.wal_records;
WalStats.m_wal_wal_fpi = pgWalUsage.wal_fpi - prevWalUsage.wal_fpi;
...
pgstat_send(&WalStats, sizeof(WalStats));
/* remember the current numbers */
prevWalUsage = pgWalUsage;

Thanks for your advice. This code can avoid the performance impact of
critical code.

By the way, what do you think to add these statistics to the
pg_stat_wal view?
I thought to remove the above statistics because there is advice that
PG13's features,
for example, pg_stat_statement view, vacuum log, and so on can cover
use-cases.

At Thu, 22 Oct 2020 10:09:21 +0900, Masahiro Ikeda <ikedamsh@oss.nttdata.com> wrote in

I agreed that the statement-level stat is important and I understood
that we can
know the aggregated WAL stats of pg_stat_statement view and
autovacuum's
log.
But now, WAL stats generated by autovacuum can be output to logs and
it
is not
easy to aggregate them. Since WAL writes impacts for the entire
cluster,
I thought
it's natural to provide accumulated value.

I think it is other way i.e if we would have accumulated stats then it
makes sense to provide those at statement-level because one would like
to know the exact cause of more WAL activity. Say it is due to an
autovacuum or due to the particular set of statements then it would
easier for users to do something about it.

OK, I'll remove them.
Do you have any comments for other statistics?

That discussion comes from the fact that the code adds duplicate code
in a hot path. If we that extra cost doesn't exist, we are free to
add the accumulated values in pg_stat_wal. I think they are useful for
stats-collecting tools as far as we can do that without such an extra
cost.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

Thanks for your comments and advice. I updated the patch.

On 2020-10-21 18:03, Kyotaro Horiguchi wrote:

At Tue, 20 Oct 2020 16:11:29 +0900, Masahiro Ikeda
<ikedamsh@oss.nttdata.com> wrote in

On 2020-10-20 12:46, Amit Kapila wrote:

I see that we also need to add extra code to capture these stats (some
of which is in performance-critical path especially in
XLogInsertRecord) which again makes me a bit uncomfortable. It might
be that it is all fine as it is very important to collect these stats
at cluster-level in spite that the same information can be gathered at
statement-level to help customers but I don't see a very strong case
for that in your proposal.

We should avoid that duplication as possible even if the both number
are important.

Also about performance, I thought there are few impacts because it
increments stats in memory. If I can implement to reuse pgWalUsage's
value which already collects these stats, there is no impact in
XLogInsertRecord.
For example, how about pg_stat_wal() calculates the accumulated
value of wal_records, wal_fpi, and wal_bytes to use pgWalUsage's
value?

I don't think that works, but it would work that pgstat_send_wal()
takes the difference of that values between two successive calls.

WalUsage prevWalUsage;
...
pgstat_send_wal()
{
..
/* fill in some values using pgWalUsage */
WalStats.m_wal_bytes = pgWalUsage.wal_bytes -
prevWalUsage.wal_bytes;
WalStats.m_wal_records = pgWalUsage.wal_records -
prevWalUsage.wal_records;
WalStats.m_wal_wal_fpi = pgWalUsage.wal_fpi -
prevWalUsage.wal_fpi;
...
pgstat_send(&WalStats, sizeof(WalStats));

/* remember the current numbers */
prevWalUsage = pgWalUsage;

Thanks for Horiguchi-san's advice, I changed to reuse pgWalUsage
which is already defined and eliminates the extra overhead.

5. I notice some code in issue_xlog_fsync() function to collect sync
info,
a standby may call the issue_xlog_fsync() too, which do not want to
to collect this info. I think this need some change avoid run by
standby side.

IIUC, issue_xlog_fsync is called by wal receiver on standby side.
But it doesn't send collected statistics to the stats collecter.
So, I think it's not necessary to change the code to avoid collecting
the stats on the standby side.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

On 2020/10/29 17:03, Masahiro Ikeda wrote:

Hi,

Thanks for your comments and advice. I updated the patch.

On 2020-10-21 18:03, Kyotaro Horiguchi wrote:

At Tue, 20 Oct 2020 16:11:29 +0900, Masahiro Ikeda
<ikedamsh@oss.nttdata.com> wrote in

On 2020-10-20 12:46, Amit Kapila wrote:

I see that we also need to add extra code to capture these stats (some
of which is in performance-critical path especially in
XLogInsertRecord) which again makes me a bit uncomfortable. It might
be that it is all fine as it is very important to collect these stats
at cluster-level in spite that the same information can be gathered at
statement-level to help customers but I don't see a very strong case
for that in your proposal.

We should avoid that duplication as possible even if the both number
are important.

Also about performance, I thought there are few impacts because it
increments stats in memory. If I can implement to reuse pgWalUsage's
value which already collects these stats, there is no impact in
XLogInsertRecord.
For example, how about pg_stat_wal() calculates the accumulated
value of wal_records, wal_fpi, and wal_bytes to use pgWalUsage's
value?

I don't think that works, but it would work that pgstat_send_wal()
takes the difference of that values between two successive calls.

WalUsage prevWalUsage;
...
pgstat_send_wal()
{
..
�� /* fill in some values using pgWalUsage */
�� WalStats.m_wal_bytes�� = pgWalUsage.wal_bytes�� - prevWalUsage.wal_bytes;
�� WalStats.m_wal_records = pgWalUsage.wal_records - prevWalUsage.wal_records;
�� WalStats.m_wal_wal_fpi = pgWalUsage.wal_fpi���� - prevWalUsage.wal_fpi;
...
�� pgstat_send(&WalStats, sizeof(WalStats));

�� /* remember the current numbers */
�� prevWalUsage = pgWalUsage;

Thanks for Horiguchi-san's advice, I changed to reuse pgWalUsage
which is already defined and eliminates the extra overhead.

+	/* fill in some values using pgWalUsage */
+	WalStats.m_wal_bytes = pgWalUsage.wal_bytes - prevWalUsage.wal_bytes;
+	WalStats.m_wal_records = pgWalUsage.wal_records - prevWalUsage.wal_records;
+	WalStats.m_wal_fpi = pgWalUsage.wal_fpi - prevWalUsage.wal_fpi;

It's better to use WalUsageAccumDiff() here?

prevWalUsage needs to be initialized with pgWalUsage?

+				if (AmWalWriterProcess()){
+					WalStats.m_wal_write_walwriter++;
+				}
+				else
+				{
+					WalStats.m_wal_write_backend++;
+				}

I think that it's better not to separate m_wal_write_xxx into two for
walwriter and other processes. Instead, we can use one m_wal_write_xxx
counter and make pgstat_send_wal() send also the process type to
the stats collector. Then the stats collector can accumulate the counters
per process type if necessary. If we adopt this approach, we can easily
extend pg_stat_wal so that any fields can be reported per process type.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2020/10/20 11:31, Masahiro Ikeda wrote:

Hi,

I think we need to add some statistics to pg_stat_wal view.

Although there are some parameter related WAL,
there are few statistics for tuning them.

I think it's better to provide the following statistics.
Please let me know your comments.

```
postgres=# SELECT * from pg_stat_wal;
-[ RECORD 1 ]-------+------------------------------
wal_records         | 2000224
wal_fpi             | 47
wal_bytes           | 248216337
wal_buffers_full    | 20954
wal_init_file       | 8
wal_write_backend   | 20960
wal_write_walwriter | 46
wal_write_time      | 51
wal_sync_backend    | 7
wal_sync_walwriter  | 8
wal_sync_time       | 0
stats_reset         | 2020-10-20 11:04:51.307771+09
```

1. Basic statistics of WAL activity

- wal_records: Total number of WAL records generated
- wal_fpi: Total number of WAL full page images generated
- wal_bytes: Total amount of WAL bytes generated

To understand DB's performance, first, we will check the performance
trends for the entire database instance.
For example, if the number of wal_fpi becomes higher, users may tune
"wal_compression", "checkpoint_timeout" and so on.

Although users can check the above statistics via EXPLAIN, auto_explain,
autovacuum and pg_stat_statements now,
if users want to see the performance trends  for the entire database,
they must recalculate the statistics.

I think it is useful to add the sum of the basic statistics.

2.  WAL segment file creation

- wal_init_file: Total number of WAL segment files created.

To create a new WAL file may have an impact on the performance of
a write-heavy workload generating lots of WAL. If this number is reported high,
to reduce the number of this initialization, we can tune WAL-related parameters
so that more "recycled" WAL files can be held.

3. Number of when WAL is flushed

- wal_write_backend : Total number of WAL data written to the disk by backends
- wal_write_walwriter : Total number of WAL data written to the disk by walwriter
- wal_sync_backend : Total number of WAL data synced to the disk by backends
- wal_sync_walwriter : Total number of WAL data synced to the disk by walwrite

I think it's useful for tuning "synchronous_commit" and "commit_delay" for query executions.
If the number of WAL is flushed is high, users can know "synchronous_commit" is useful for the workload.

I just wonder how useful these counters are. Even without these counters,
we already know synchronous_commit=off is likely to cause the better
performance (but has the risk of data loss). So ISTM that these counters are
not so useful when tuning synchronous_commit.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2020-10-30 11:50, Fujii Masao wrote:

On 2020/10/29 17:03, Masahiro Ikeda wrote:

Hi,

Thanks for your comments and advice. I updated the patch.

On 2020-10-21 18:03, Kyotaro Horiguchi wrote:

At Tue, 20 Oct 2020 16:11:29 +0900, Masahiro Ikeda
<ikedamsh@oss.nttdata.com> wrote in

On 2020-10-20 12:46, Amit Kapila wrote:

I see that we also need to add extra code to capture these stats (some
of which is in performance-critical path especially in
XLogInsertRecord) which again makes me a bit uncomfortable. It might
be that it is all fine as it is very important to collect these stats
at cluster-level in spite that the same information can be gathered at
statement-level to help customers but I don't see a very strong case
for that in your proposal.

We should avoid that duplication as possible even if the both number
are important.

Also about performance, I thought there are few impacts because it
increments stats in memory. If I can implement to reuse pgWalUsage's
value which already collects these stats, there is no impact in
XLogInsertRecord.
For example, how about pg_stat_wal() calculates the accumulated
value of wal_records, wal_fpi, and wal_bytes to use pgWalUsage's
value?

I don't think that works, but it would work that pgstat_send_wal()
takes the difference of that values between two successive calls.

WalUsage prevWalUsage;
...
pgstat_send_wal()
{
..
   /* fill in some values using pgWalUsage */
   WalStats.m_wal_bytes   = pgWalUsage.wal_bytes   -
prevWalUsage.wal_bytes;
   WalStats.m_wal_records = pgWalUsage.wal_records -
prevWalUsage.wal_records;
   WalStats.m_wal_wal_fpi = pgWalUsage.wal_fpi     -
prevWalUsage.wal_fpi;
...
   pgstat_send(&WalStats, sizeof(WalStats));

   /* remember the current numbers */
   prevWalUsage = pgWalUsage;

Thanks for Horiguchi-san's advice, I changed to reuse pgWalUsage
which is already defined and eliminates the extra overhead.

+	/* fill in some values using pgWalUsage */
+	WalStats.m_wal_bytes = pgWalUsage.wal_bytes - prevWalUsage.wal_bytes;
+	WalStats.m_wal_records = pgWalUsage.wal_records - 
prevWalUsage.wal_records;
+	WalStats.m_wal_fpi = pgWalUsage.wal_fpi - prevWalUsage.wal_fpi;

It's better to use WalUsageAccumDiff() here?

Yes, thanks. I fixed it.

prevWalUsage needs to be initialized with pgWalUsage?

+				if (AmWalWriterProcess()){
+					WalStats.m_wal_write_walwriter++;
+				}
+				else
+				{
+					WalStats.m_wal_write_backend++;
+				}

I think that it's better not to separate m_wal_write_xxx into two for
walwriter and other processes. Instead, we can use one m_wal_write_xxx
counter and make pgstat_send_wal() send also the process type to
the stats collector. Then the stats collector can accumulate the
counters
per process type if necessary. If we adopt this approach, we can easily
extend pg_stat_wal so that any fields can be reported per process type.

I'll remove the above source code because these counters are not useful.

On 2020-10-30 12:00, Fujii Masao wrote:

On 2020/10/20 11:31, Masahiro Ikeda wrote:

Hi,

I think we need to add some statistics to pg_stat_wal view.

Although there are some parameter related WAL,
there are few statistics for tuning them.

I think it's better to provide the following statistics.
Please let me know your comments.

```
postgres=# SELECT * from pg_stat_wal;
-[ RECORD 1 ]-------+------------------------------
wal_records         | 2000224
wal_fpi             | 47
wal_bytes           | 248216337
wal_buffers_full    | 20954
wal_init_file       | 8
wal_write_backend   | 20960
wal_write_walwriter | 46
wal_write_time      | 51
wal_sync_backend    | 7
wal_sync_walwriter  | 8
wal_sync_time       | 0
stats_reset         | 2020-10-20 11:04:51.307771+09
```

1. Basic statistics of WAL activity

- wal_records: Total number of WAL records generated
- wal_fpi: Total number of WAL full page images generated
- wal_bytes: Total amount of WAL bytes generated

To understand DB's performance, first, we will check the performance
trends for the entire database instance.
For example, if the number of wal_fpi becomes higher, users may tune
"wal_compression", "checkpoint_timeout" and so on.

Although users can check the above statistics via EXPLAIN,
auto_explain,
autovacuum and pg_stat_statements now,
if users want to see the performance trends  for the entire database,
they must recalculate the statistics.

I think it is useful to add the sum of the basic statistics.

2.  WAL segment file creation

- wal_init_file: Total number of WAL segment files created.

To create a new WAL file may have an impact on the performance of
a write-heavy workload generating lots of WAL. If this number is
reported high,
to reduce the number of this initialization, we can tune WAL-related
parameters
so that more "recycled" WAL files can be held.

3. Number of when WAL is flushed

- wal_write_backend : Total number of WAL data written to the disk by
backends
- wal_write_walwriter : Total number of WAL data written to the disk
by walwriter
- wal_sync_backend : Total number of WAL data synced to the disk by
backends
- wal_sync_walwriter : Total number of WAL data synced to the disk by
walwrite

I think it's useful for tuning "synchronous_commit" and "commit_delay"
for query executions.
If the number of WAL is flushed is high, users can know
"synchronous_commit" is useful for the workload.

I just wonder how useful these counters are. Even without these
counters,
we already know synchronous_commit=off is likely to cause the better
performance (but has the risk of data loss). So ISTM that these
counters are
not so useful when tuning synchronous_commit.

Thanks, my understanding was wrong.
I agreed that your comments.

I merged the statistics of *_backend and *_walwriter.
I think the sum of them is useful to calculate the average per
write/sync time.
For example, per write time is equals wal_write_time / wal_write.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

On 2020/11/06 10:25, Masahiro Ikeda wrote:

On 2020-10-30 11:50, Fujii Masao wrote:

On 2020/10/29 17:03, Masahiro Ikeda wrote:

Hi,

Thanks for your comments and advice. I updated the patch.

On 2020-10-21 18:03, Kyotaro Horiguchi wrote:

At Tue, 20 Oct 2020 16:11:29 +0900, Masahiro Ikeda
<ikedamsh@oss.nttdata.com> wrote in

On 2020-10-20 12:46, Amit Kapila wrote:

I see that we also need to add extra code to capture these stats (some
of which is in performance-critical path especially in
XLogInsertRecord) which again makes me a bit uncomfortable. It might
be that it is all fine as it is very important to collect these stats
at cluster-level in spite that the same information can be gathered at
statement-level to help customers but I don't see a very strong case
for that in your proposal.

We should avoid that duplication as possible even if the both number
are important.

Also about performance, I thought there are few impacts because it
increments stats in memory. If I can implement to reuse pgWalUsage's
value which already collects these stats, there is no impact in
XLogInsertRecord.
For example, how about pg_stat_wal() calculates the accumulated
value of wal_records, wal_fpi, and wal_bytes to use pgWalUsage's
value?

I don't think that works, but it would work that pgstat_send_wal()
takes the difference of that values between two successive calls.

WalUsage prevWalUsage;
...
pgstat_send_wal()
{
..
   /* fill in some values using pgWalUsage */
   WalStats.m_wal_bytes   = pgWalUsage.wal_bytes   - prevWalUsage.wal_bytes;
   WalStats.m_wal_records = pgWalUsage.wal_records - prevWalUsage.wal_records;
   WalStats.m_wal_wal_fpi = pgWalUsage.wal_fpi     - prevWalUsage.wal_fpi;
...
   pgstat_send(&WalStats, sizeof(WalStats));

   /* remember the current numbers */
   prevWalUsage = pgWalUsage;

Thanks for Horiguchi-san's advice, I changed to reuse pgWalUsage
which is already defined and eliminates the extra overhead.

+    /* fill in some values using pgWalUsage */
+    WalStats.m_wal_bytes = pgWalUsage.wal_bytes - prevWalUsage.wal_bytes;
+    WalStats.m_wal_records = pgWalUsage.wal_records - prevWalUsage.wal_records;
+    WalStats.m_wal_fpi = pgWalUsage.wal_fpi - prevWalUsage.wal_fpi;

It's better to use WalUsageAccumDiff() here?

Yes, thanks. I fixed it.

prevWalUsage needs to be initialized with pgWalUsage?

+                if (AmWalWriterProcess()){
+                    WalStats.m_wal_write_walwriter++;
+                }
+                else
+                {
+                    WalStats.m_wal_write_backend++;
+                }

I think that it's better not to separate m_wal_write_xxx into two for
walwriter and other processes. Instead, we can use one m_wal_write_xxx
counter and make pgstat_send_wal() send also the process type to
the stats collector. Then the stats collector can accumulate the counters
per process type if necessary. If we adopt this approach, we can easily
extend pg_stat_wal so that any fields can be reported per process type.

I'll remove the above source code because these counters are not useful.

On 2020-10-30 12:00, Fujii Masao wrote:

On 2020/10/20 11:31, Masahiro Ikeda wrote:

Hi,

I think we need to add some statistics to pg_stat_wal view.

Although there are some parameter related WAL,
there are few statistics for tuning them.

I think it's better to provide the following statistics.
Please let me know your comments.

```
postgres=# SELECT * from pg_stat_wal;
-[ RECORD 1 ]-------+------------------------------
wal_records         | 2000224
wal_fpi             | 47
wal_bytes           | 248216337
wal_buffers_full    | 20954
wal_init_file       | 8
wal_write_backend   | 20960
wal_write_walwriter | 46
wal_write_time      | 51
wal_sync_backend    | 7
wal_sync_walwriter  | 8
wal_sync_time       | 0
stats_reset         | 2020-10-20 11:04:51.307771+09
```

1. Basic statistics of WAL activity

- wal_records: Total number of WAL records generated
- wal_fpi: Total number of WAL full page images generated
- wal_bytes: Total amount of WAL bytes generated

To understand DB's performance, first, we will check the performance
trends for the entire database instance.
For example, if the number of wal_fpi becomes higher, users may tune
"wal_compression", "checkpoint_timeout" and so on.

Although users can check the above statistics via EXPLAIN, auto_explain,
autovacuum and pg_stat_statements now,
if users want to see the performance trends  for the entire database,
they must recalculate the statistics.

I think it is useful to add the sum of the basic statistics.

2.  WAL segment file creation

- wal_init_file: Total number of WAL segment files created.

To create a new WAL file may have an impact on the performance of
a write-heavy workload generating lots of WAL. If this number is reported high,
to reduce the number of this initialization, we can tune WAL-related parameters
so that more "recycled" WAL files can be held.

3. Number of when WAL is flushed

- wal_write_backend : Total number of WAL data written to the disk by backends
- wal_write_walwriter : Total number of WAL data written to the disk by walwriter
- wal_sync_backend : Total number of WAL data synced to the disk by backends
- wal_sync_walwriter : Total number of WAL data synced to the disk by walwrite

I think it's useful for tuning "synchronous_commit" and "commit_delay" for query executions.
If the number of WAL is flushed is high, users can know "synchronous_commit" is useful for the workload.

I just wonder how useful these counters are. Even without these counters,
we already know synchronous_commit=off is likely to cause the better
performance (but has the risk of data loss). So ISTM that these counters are
not so useful when tuning synchronous_commit.

Thanks, my understanding was wrong.
I agreed that your comments.

I merged the statistics of *_backend and *_walwriter.
I think the sum of them is useful to calculate the average per write/sync time.
For example, per write time is equals wal_write_time / wal_write.

Understood.

Thanks for updating the patch!

patching file src/include/catalog/pg_proc.dat
Hunk #1 FAILED at 5491.
1 out of 1 hunk FAILED -- saving rejects to file src/include/catalog/pg_proc.dat.rej

I got this failure when applying the patch. Could you update the patch?

-       Number of times WAL data was written to the disk because WAL buffers got full
+       Total number of times WAL data written to the disk because WAL buffers got full

Isn't "was" necessary between "data" and "written"?

+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>wal_bytes</structfield> <type>bigint</type>

Shouldn't the type of wal_bytes be numeric because the total number of
WAL bytes can exceed the range of bigint? I think that the type of
pg_stat_statements.wal_bytes is also numeric for the same reason.

+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>wal_write_time</structfield> <type>bigint</type>

Shouldn't the type of wal_xxx_time be double precision,
like pg_stat_database.blk_write_time?

Even when fsync is set to off or wal_sync_method is set to open_sync,
wal_sync is incremented. Isn't this behavior confusing?

+       Total amount of time that has been spent in the portion of
+       WAL data was written to disk by backend and walwriter, in milliseconds
+       (if <xref linkend="guc-track-io-timing"/> is enabled, otherwise zero)

With the patch, track_io_timing controls both IO for data files and
WAL files. But we may want to track only either of them. So it's better
to extend track_io_timing so that we can specify the tracking target
in the parameter? For example, we can make track_io_timing accept
data, wal and all. Or we should introduce new GUC for WAL, e.g.,
track_wal_io_timing? Thought?

I'm afraid that "by backend and walwriter" part can make us thinkg
incorrectly that WAL writes by other processes like autovacuum
are not tracked.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2020/11/12 14:58, Fujii Masao wrote:

On 2020/11/06 10:25, Masahiro Ikeda wrote:

On 2020-10-30 11:50, Fujii Masao wrote:

On 2020/10/29 17:03, Masahiro Ikeda wrote:

Hi,

Thanks for your comments and advice. I updated the patch.

On 2020-10-21 18:03, Kyotaro Horiguchi wrote:

At Tue, 20 Oct 2020 16:11:29 +0900, Masahiro Ikeda
<ikedamsh@oss.nttdata.com> wrote in

On 2020-10-20 12:46, Amit Kapila wrote:

I see that we also need to add extra code to capture these stats (some
of which is in performance-critical path especially in
XLogInsertRecord) which again makes me a bit uncomfortable. It might
be that it is all fine as it is very important to collect these stats
at cluster-level in spite that the same information can be gathered at
statement-level to help customers but I don't see a very strong case
for that in your proposal.

We should avoid that duplication as possible even if the both number
are important.

Also about performance, I thought there are few impacts because it
increments stats in memory. If I can implement to reuse pgWalUsage's
value which already collects these stats, there is no impact in
XLogInsertRecord.
For example, how about pg_stat_wal() calculates the accumulated
value of wal_records, wal_fpi, and wal_bytes to use pgWalUsage's
value?

I don't think that works, but it would work that pgstat_send_wal()
takes the difference of that values between two successive calls.

WalUsage prevWalUsage;
...
pgstat_send_wal()
{
..
   /* fill in some values using pgWalUsage */
   WalStats.m_wal_bytes   = pgWalUsage.wal_bytes   - prevWalUsage.wal_bytes;
   WalStats.m_wal_records = pgWalUsage.wal_records - prevWalUsage.wal_records;
   WalStats.m_wal_wal_fpi = pgWalUsage.wal_fpi     - prevWalUsage.wal_fpi;
...
   pgstat_send(&WalStats, sizeof(WalStats));

   /* remember the current numbers */
   prevWalUsage = pgWalUsage;

Thanks for Horiguchi-san's advice, I changed to reuse pgWalUsage
which is already defined and eliminates the extra overhead.

+    /* fill in some values using pgWalUsage */
+    WalStats.m_wal_bytes = pgWalUsage.wal_bytes - prevWalUsage.wal_bytes;
+    WalStats.m_wal_records = pgWalUsage.wal_records - prevWalUsage.wal_records;
+    WalStats.m_wal_fpi = pgWalUsage.wal_fpi - prevWalUsage.wal_fpi;

It's better to use WalUsageAccumDiff() here?

Yes, thanks. I fixed it.

prevWalUsage needs to be initialized with pgWalUsage?

+                if (AmWalWriterProcess()){
+                    WalStats.m_wal_write_walwriter++;
+                }
+                else
+                {
+                    WalStats.m_wal_write_backend++;
+                }

I think that it's better not to separate m_wal_write_xxx into two for
walwriter and other processes. Instead, we can use one m_wal_write_xxx
counter and make pgstat_send_wal() send also the process type to
the stats collector. Then the stats collector can accumulate the counters
per process type if necessary. If we adopt this approach, we can easily
extend pg_stat_wal so that any fields can be reported per process type.

I'll remove the above source code because these counters are not useful.

On 2020-10-30 12:00, Fujii Masao wrote:

On 2020/10/20 11:31, Masahiro Ikeda wrote:

Hi,

I think we need to add some statistics to pg_stat_wal view.

Although there are some parameter related WAL,
there are few statistics for tuning them.

I think it's better to provide the following statistics.
Please let me know your comments.

```
postgres=# SELECT * from pg_stat_wal;
-[ RECORD 1 ]-------+------------------------------
wal_records         | 2000224
wal_fpi             | 47
wal_bytes           | 248216337
wal_buffers_full    | 20954
wal_init_file       | 8
wal_write_backend   | 20960
wal_write_walwriter | 46
wal_write_time      | 51
wal_sync_backend    | 7
wal_sync_walwriter  | 8
wal_sync_time       | 0
stats_reset         | 2020-10-20 11:04:51.307771+09
```

1. Basic statistics of WAL activity

- wal_records: Total number of WAL records generated
- wal_fpi: Total number of WAL full page images generated
- wal_bytes: Total amount of WAL bytes generated

To understand DB's performance, first, we will check the performance
trends for the entire database instance.
For example, if the number of wal_fpi becomes higher, users may tune
"wal_compression", "checkpoint_timeout" and so on.

Although users can check the above statistics via EXPLAIN, auto_explain,
autovacuum and pg_stat_statements now,
if users want to see the performance trends  for the entire database,
they must recalculate the statistics.

I think it is useful to add the sum of the basic statistics.

2.  WAL segment file creation

- wal_init_file: Total number of WAL segment files created.

To create a new WAL file may have an impact on the performance of
a write-heavy workload generating lots of WAL. If this number is reported high,
to reduce the number of this initialization, we can tune WAL-related parameters
so that more "recycled" WAL files can be held.

3. Number of when WAL is flushed

- wal_write_backend : Total number of WAL data written to the disk by backends
- wal_write_walwriter : Total number of WAL data written to the disk by walwriter
- wal_sync_backend : Total number of WAL data synced to the disk by backends
- wal_sync_walwriter : Total number of WAL data synced to the disk by walwrite

I think it's useful for tuning "synchronous_commit" and "commit_delay" for query executions.
If the number of WAL is flushed is high, users can know "synchronous_commit" is useful for the workload.

I just wonder how useful these counters are. Even without these counters,
we already know synchronous_commit=off is likely to cause the better
performance (but has the risk of data loss). So ISTM that these counters are
not so useful when tuning synchronous_commit.

Thanks, my understanding was wrong.
I agreed that your comments.

I merged the statistics of *_backend and *_walwriter.
I think the sum of them is useful to calculate the average per write/sync time.
For example, per write time is equals wal_write_time / wal_write.

Understood.

Thanks for updating the patch!

patching file src/include/catalog/pg_proc.dat
Hunk #1 FAILED at 5491.
1 out of 1 hunk FAILED -- saving rejects to file src/include/catalog/pg_proc.dat.rej

I got this failure when applying the patch. Could you update the patch?

-       Number of times WAL data was written to the disk because WAL buffers got full
+       Total number of times WAL data written to the disk because WAL buffers got full

Isn't "was" necessary between "data" and "written"?

+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>wal_bytes</structfield> <type>bigint</type>

Shouldn't the type of wal_bytes be numeric because the total number of
WAL bytes can exceed the range of bigint? I think that the type of
pg_stat_statements.wal_bytes is also numeric for the same reason.

+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>wal_write_time</structfield> <type>bigint</type>

Shouldn't the type of wal_xxx_time be double precision,
like pg_stat_database.blk_write_time?

Even when fsync is set to off or wal_sync_method is set to open_sync,
wal_sync is incremented. Isn't this behavior confusing?

+       Total amount of time that has been spent in the portion of
+       WAL data was written to disk by backend and walwriter, in milliseconds
+       (if <xref linkend="guc-track-io-timing"/> is enabled, otherwise zero)

With the patch, track_io_timing controls both IO for data files and
WAL files. But we may want to track only either of them. So it's better
to extend track_io_timing so that we can specify the tracking target
in the parameter? For example, we can make track_io_timing accept
data, wal and all. Or we should introduce new GUC for WAL, e.g.,
track_wal_io_timing? Thought?

I'm afraid that "by backend and walwriter" part can make us thinkg
incorrectly that WAL writes by other processes like autovacuum
are not tracked.

  pgstat_send_wal(void)
  {
+	/* fill in some values using pgWalUsage */
+	WalUsage walusage;
+	memset(&walusage, 0, sizeof(WalUsage));
+	WalUsageAccumDiff(&walusage, &pgWalUsage, &prevWalUsage);

At the first call to pgstat_send_wal(), prevWalUsage has not been set to
the previous value of pgWalUsage. So the calculation result of
WalUsageAccumDiff() can be incorrect. To address this issue,
prevWalUsage should be set to pgWalUsage or both should be initialized
with 0 at the beginning of the process, for example?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Now, pg_stat_wal supports reset all informantion in WalStats
using pg_stat_reset_shared('wal') function.
Isn't it enough?

Yes it ok, sorry I miss this infomation.

3. I do not think it's a correct describe in document for
'wal_buffers_full'.

Where should I rewrite the description? If my understanding is not
correct, please let me know.

Sorry I have not described it clearly, because I can not understand the meaning of this
column after I read the describe in document.
And now I read the source code of walwrite and found the place where 'wal_buffers_full'
added is for a backend to wait a wal buffer which is occupied by other wal page, so the
backend flush the old page in the wal buffer(after wait it can).
So i think the origin decribe in document is not so in point, we can describe it such as
'Total number of times WAL data written to the disk because a backend yelled a wal buffer
for an advanced wal page.

Sorry if my understand is wrong.

On 2020-11-12 16:27, Fujii Masao wrote:

On 2020/11/12 14:58, Fujii Masao wrote:

On 2020/11/06 10:25, Masahiro Ikeda wrote:

On 2020-10-30 11:50, Fujii Masao wrote:

On 2020/10/29 17:03, Masahiro Ikeda wrote:

Hi,

Thanks for your comments and advice. I updated the patch.

On 2020-10-21 18:03, Kyotaro Horiguchi wrote:

At Tue, 20 Oct 2020 16:11:29 +0900, Masahiro Ikeda
<ikedamsh@oss.nttdata.com> wrote in

On 2020-10-20 12:46, Amit Kapila wrote:

I see that we also need to add extra code to capture these stats (some
of which is in performance-critical path especially in
XLogInsertRecord) which again makes me a bit uncomfortable. It might
be that it is all fine as it is very important to collect these stats
at cluster-level in spite that the same information can be gathered at
statement-level to help customers but I don't see a very strong case
for that in your proposal.

We should avoid that duplication as possible even if the both
number
are important.

Also about performance, I thought there are few impacts because
it
increments stats in memory. If I can implement to reuse
pgWalUsage's
value which already collects these stats, there is no impact in
XLogInsertRecord.
For example, how about pg_stat_wal() calculates the accumulated
value of wal_records, wal_fpi, and wal_bytes to use pgWalUsage's
value?

I don't think that works, but it would work that pgstat_send_wal()
takes the difference of that values between two successive calls.

WalUsage prevWalUsage;
...
pgstat_send_wal()
{
..
   /* fill in some values using pgWalUsage */
   WalStats.m_wal_bytes   = pgWalUsage.wal_bytes   -
prevWalUsage.wal_bytes;
   WalStats.m_wal_records = pgWalUsage.wal_records -
prevWalUsage.wal_records;
   WalStats.m_wal_wal_fpi = pgWalUsage.wal_fpi     -
prevWalUsage.wal_fpi;
...
   pgstat_send(&WalStats, sizeof(WalStats));

   /* remember the current numbers */
   prevWalUsage = pgWalUsage;

Thanks for Horiguchi-san's advice, I changed to reuse pgWalUsage
which is already defined and eliminates the extra overhead.

+    /* fill in some values using pgWalUsage */
+    WalStats.m_wal_bytes = pgWalUsage.wal_bytes - 
prevWalUsage.wal_bytes;
+    WalStats.m_wal_records = pgWalUsage.wal_records - 
prevWalUsage.wal_records;
+    WalStats.m_wal_fpi = pgWalUsage.wal_fpi - prevWalUsage.wal_fpi;

It's better to use WalUsageAccumDiff() here?

Yes, thanks. I fixed it.

prevWalUsage needs to be initialized with pgWalUsage?

+                if (AmWalWriterProcess()){
+                    WalStats.m_wal_write_walwriter++;
+                }
+                else
+                {
+                    WalStats.m_wal_write_backend++;
+                }

I think that it's better not to separate m_wal_write_xxx into two
for
walwriter and other processes. Instead, we can use one
m_wal_write_xxx
counter and make pgstat_send_wal() send also the process type to
the stats collector. Then the stats collector can accumulate the
counters
per process type if necessary. If we adopt this approach, we can
easily
extend pg_stat_wal so that any fields can be reported per process
type.

I'll remove the above source code because these counters are not
useful.

On 2020-10-30 12:00, Fujii Masao wrote:

On 2020/10/20 11:31, Masahiro Ikeda wrote:

Hi,

I think we need to add some statistics to pg_stat_wal view.

Although there are some parameter related WAL,
there are few statistics for tuning them.

I think it's better to provide the following statistics.
Please let me know your comments.

```
postgres=# SELECT * from pg_stat_wal;
-[ RECORD 1 ]-------+------------------------------
wal_records         | 2000224
wal_fpi             | 47
wal_bytes           | 248216337
wal_buffers_full    | 20954
wal_init_file       | 8
wal_write_backend   | 20960
wal_write_walwriter | 46
wal_write_time      | 51
wal_sync_backend    | 7
wal_sync_walwriter  | 8
wal_sync_time       | 0
stats_reset         | 2020-10-20 11:04:51.307771+09
```

1. Basic statistics of WAL activity

- wal_records: Total number of WAL records generated
- wal_fpi: Total number of WAL full page images generated
- wal_bytes: Total amount of WAL bytes generated

To understand DB's performance, first, we will check the
performance
trends for the entire database instance.
For example, if the number of wal_fpi becomes higher, users may
tune
"wal_compression", "checkpoint_timeout" and so on.

Although users can check the above statistics via EXPLAIN,
auto_explain,
autovacuum and pg_stat_statements now,
if users want to see the performance trends  for the entire
database,
they must recalculate the statistics.

I think it is useful to add the sum of the basic statistics.

2.  WAL segment file creation

- wal_init_file: Total number of WAL segment files created.

To create a new WAL file may have an impact on the performance of
a write-heavy workload generating lots of WAL. If this number is
reported high,
to reduce the number of this initialization, we can tune
WAL-related parameters
so that more "recycled" WAL files can be held.

3. Number of when WAL is flushed

- wal_write_backend : Total number of WAL data written to the disk
by backends
- wal_write_walwriter : Total number of WAL data written to the
disk by walwriter
- wal_sync_backend : Total number of WAL data synced to the disk by
backends
- wal_sync_walwriter : Total number of WAL data synced to the disk
by walwrite

I think it's useful for tuning "synchronous_commit" and
"commit_delay" for query executions.
If the number of WAL is flushed is high, users can know
"synchronous_commit" is useful for the workload.

I just wonder how useful these counters are. Even without these
counters,
we already know synchronous_commit=off is likely to cause the better
performance (but has the risk of data loss). So ISTM that these
counters are
not so useful when tuning synchronous_commit.

Thanks, my understanding was wrong.
I agreed that your comments.

I merged the statistics of *_backend and *_walwriter.
I think the sum of them is useful to calculate the average per
write/sync time.
For example, per write time is equals wal_write_time / wal_write.

Understood.

Thanks for updating the patch!

patching file src/include/catalog/pg_proc.dat
Hunk #1 FAILED at 5491.
1 out of 1 hunk FAILED -- saving rejects to file
src/include/catalog/pg_proc.dat.rej

I got this failure when applying the patch. Could you update the
patch?

-       Number of times WAL data was written to the disk because WAL 
buffers got full
+       Total number of times WAL data written to the disk because WAL 
buffers got full

Isn't "was" necessary between "data" and "written"?

+      <entry role="catalog_table_entry"><para 
role="column_definition">
+       <structfield>wal_bytes</structfield> <type>bigint</type>

Shouldn't the type of wal_bytes be numeric because the total number of
WAL bytes can exceed the range of bigint? I think that the type of
pg_stat_statements.wal_bytes is also numeric for the same reason.

+      <entry role="catalog_table_entry"><para 
role="column_definition">
+       <structfield>wal_write_time</structfield> <type>bigint</type>

Shouldn't the type of wal_xxx_time be double precision,
like pg_stat_database.blk_write_time?

Even when fsync is set to off or wal_sync_method is set to open_sync,
wal_sync is incremented. Isn't this behavior confusing?

+       Total amount of time that has been spent in the portion of
+       WAL data was written to disk by backend and walwriter, in 
milliseconds
+       (if <xref linkend="guc-track-io-timing"/> is enabled, 
otherwise zero)

With the patch, track_io_timing controls both IO for data files and
WAL files. But we may want to track only either of them. So it's
better
to extend track_io_timing so that we can specify the tracking target
in the parameter? For example, we can make track_io_timing accept
data, wal and all. Or we should introduce new GUC for WAL, e.g.,
track_wal_io_timing? Thought?

I'm afraid that "by backend and walwriter" part can make us thinkg
incorrectly that WAL writes by other processes like autovacuum
are not tracked.

pgstat_send_wal(void)
{
+	/* fill in some values using pgWalUsage */
+	WalUsage walusage;
+	memset(&walusage, 0, sizeof(WalUsage));
+	WalUsageAccumDiff(&walusage, &pgWalUsage, &prevWalUsage);

At the first call to pgstat_send_wal(), prevWalUsage has not been set
to
the previous value of pgWalUsage. So the calculation result of
WalUsageAccumDiff() can be incorrect. To address this issue,
prevWalUsage should be set to pgWalUsage or both should be initialized
with 0 at the beginning of the process, for example?

I forgot to handle it, thanks.
Although I initialized it in pgstat_initialize(),
if there is better way please let me know.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

On 2020-11-12 14:58, Fujii Masao wrote:

On 2020/11/06 10:25, Masahiro Ikeda wrote:

On 2020-10-30 11:50, Fujii Masao wrote:

On 2020/10/29 17:03, Masahiro Ikeda wrote:

Hi,

Thanks for your comments and advice. I updated the patch.

On 2020-10-21 18:03, Kyotaro Horiguchi wrote:

At Tue, 20 Oct 2020 16:11:29 +0900, Masahiro Ikeda
<ikedamsh@oss.nttdata.com> wrote in

On 2020-10-20 12:46, Amit Kapila wrote:

I see that we also need to add extra code to capture these stats (some
of which is in performance-critical path especially in
XLogInsertRecord) which again makes me a bit uncomfortable. It might
be that it is all fine as it is very important to collect these stats
at cluster-level in spite that the same information can be gathered at
statement-level to help customers but I don't see a very strong case
for that in your proposal.

We should avoid that duplication as possible even if the both
number
are important.

Also about performance, I thought there are few impacts because it
increments stats in memory. If I can implement to reuse
pgWalUsage's
value which already collects these stats, there is no impact in
XLogInsertRecord.
For example, how about pg_stat_wal() calculates the accumulated
value of wal_records, wal_fpi, and wal_bytes to use pgWalUsage's
value?

I don't think that works, but it would work that pgstat_send_wal()
takes the difference of that values between two successive calls.

WalUsage prevWalUsage;
...
pgstat_send_wal()
{
..
   /* fill in some values using pgWalUsage */
   WalStats.m_wal_bytes   = pgWalUsage.wal_bytes   -
prevWalUsage.wal_bytes;
   WalStats.m_wal_records = pgWalUsage.wal_records -
prevWalUsage.wal_records;
   WalStats.m_wal_wal_fpi = pgWalUsage.wal_fpi     -
prevWalUsage.wal_fpi;
...
   pgstat_send(&WalStats, sizeof(WalStats));

   /* remember the current numbers */
   prevWalUsage = pgWalUsage;

Thanks for Horiguchi-san's advice, I changed to reuse pgWalUsage
which is already defined and eliminates the extra overhead.

+    /* fill in some values using pgWalUsage */
+    WalStats.m_wal_bytes = pgWalUsage.wal_bytes - 
prevWalUsage.wal_bytes;
+    WalStats.m_wal_records = pgWalUsage.wal_records - 
prevWalUsage.wal_records;
+    WalStats.m_wal_fpi = pgWalUsage.wal_fpi - prevWalUsage.wal_fpi;

It's better to use WalUsageAccumDiff() here?

Yes, thanks. I fixed it.

prevWalUsage needs to be initialized with pgWalUsage?

+                if (AmWalWriterProcess()){
+                    WalStats.m_wal_write_walwriter++;
+                }
+                else
+                {
+                    WalStats.m_wal_write_backend++;
+                }

I think that it's better not to separate m_wal_write_xxx into two for
walwriter and other processes. Instead, we can use one
m_wal_write_xxx
counter and make pgstat_send_wal() send also the process type to
the stats collector. Then the stats collector can accumulate the
counters
per process type if necessary. If we adopt this approach, we can
easily
extend pg_stat_wal so that any fields can be reported per process
type.

I'll remove the above source code because these counters are not
useful.

On 2020-10-30 12:00, Fujii Masao wrote:

On 2020/10/20 11:31, Masahiro Ikeda wrote:

Hi,

I think we need to add some statistics to pg_stat_wal view.

Although there are some parameter related WAL,
there are few statistics for tuning them.

I think it's better to provide the following statistics.
Please let me know your comments.

```
postgres=# SELECT * from pg_stat_wal;
-[ RECORD 1 ]-------+------------------------------
wal_records         | 2000224
wal_fpi             | 47
wal_bytes           | 248216337
wal_buffers_full    | 20954
wal_init_file       | 8
wal_write_backend   | 20960
wal_write_walwriter | 46
wal_write_time      | 51
wal_sync_backend    | 7
wal_sync_walwriter  | 8
wal_sync_time       | 0
stats_reset         | 2020-10-20 11:04:51.307771+09
```

1. Basic statistics of WAL activity

- wal_records: Total number of WAL records generated
- wal_fpi: Total number of WAL full page images generated
- wal_bytes: Total amount of WAL bytes generated

To understand DB's performance, first, we will check the performance
trends for the entire database instance.
For example, if the number of wal_fpi becomes higher, users may tune
"wal_compression", "checkpoint_timeout" and so on.

Although users can check the above statistics via EXPLAIN,
auto_explain,
autovacuum and pg_stat_statements now,
if users want to see the performance trends  for the entire
database,
they must recalculate the statistics.

I think it is useful to add the sum of the basic statistics.

2.  WAL segment file creation

- wal_init_file: Total number of WAL segment files created.

To create a new WAL file may have an impact on the performance of
a write-heavy workload generating lots of WAL. If this number is
reported high,
to reduce the number of this initialization, we can tune WAL-related
parameters
so that more "recycled" WAL files can be held.

3. Number of when WAL is flushed

- wal_write_backend : Total number of WAL data written to the disk
by backends
- wal_write_walwriter : Total number of WAL data written to the disk
by walwriter
- wal_sync_backend : Total number of WAL data synced to the disk by
backends
- wal_sync_walwriter : Total number of WAL data synced to the disk
by walwrite

I think it's useful for tuning "synchronous_commit" and
"commit_delay" for query executions.
If the number of WAL is flushed is high, users can know
"synchronous_commit" is useful for the workload.

I just wonder how useful these counters are. Even without these
counters,
we already know synchronous_commit=off is likely to cause the better
performance (but has the risk of data loss). So ISTM that these
counters are
not so useful when tuning synchronous_commit.

Thanks, my understanding was wrong.
I agreed that your comments.

I merged the statistics of *_backend and *_walwriter.
I think the sum of them is useful to calculate the average per
write/sync time.
For example, per write time is equals wal_write_time / wal_write.

Understood.

Thanks for updating the patch!

Thanks for your comments.

patching file src/include/catalog/pg_proc.dat
Hunk #1 FAILED at 5491.
1 out of 1 hunk FAILED -- saving rejects to file
src/include/catalog/pg_proc.dat.rej

I got this failure when applying the patch. Could you update the patch?

Thanks, I updated the patch.

-       Number of times WAL data was written to the disk because WAL
buffers got full
+       Total number of times WAL data written to the disk because WAL
buffers got full

Isn't "was" necessary between "data" and "written"?

Yes, I fixed it.

+      <entry role="catalog_table_entry"><para 
role="column_definition">
+       <structfield>wal_bytes</structfield> <type>bigint</type>

Shouldn't the type of wal_bytes be numeric because the total number of
WAL bytes can exceed the range of bigint? I think that the type of
pg_stat_statements.wal_bytes is also numeric for the same reason.

Thanks, I fixed it.

Since I cast the type of wal_bytes from PgStat_Counter to uint64,
I changed the type of PgStat_MsgWal and PgStat_WalStats too.

+      <entry role="catalog_table_entry"><para 
role="column_definition">
+       <structfield>wal_write_time</structfield> <type>bigint</type>

Shouldn't the type of wal_xxx_time be double precision,
like pg_stat_database.blk_write_time?

Thanks, I changed it.

Even when fsync is set to off or wal_sync_method is set to open_sync,
wal_sync is incremented. Isn't this behavior confusing?

+       Total amount of time that has been spent in the portion of
+       WAL data was written to disk by backend and walwriter, in 
milliseconds
+       (if <xref linkend="guc-track-io-timing"/> is enabled, otherwise 
zero)

With the patch, track_io_timing controls both IO for data files and
WAL files. But we may want to track only either of them. So it's better
to extend track_io_timing so that we can specify the tracking target
in the parameter? For example, we can make track_io_timing accept
data, wal and all. Or we should introduce new GUC for WAL, e.g.,
track_wal_io_timing? Thought?

OK, I introduced the new GUC "track_wal_io_timing".

I'm afraid that "by backend and walwriter" part can make us thinkg
incorrectly that WAL writes by other processes like autovacuum
are not tracked.

Sorry, I removed "by backend and walwriter".

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION

On 2020-11-13 12:32, lchch1990@sina.cn wrote:

Now, pg_stat_wal supports reset all informantion in WalStats
using pg_stat_reset_shared('wal') function.
Isn't it enough?

Yes it ok, sorry I miss this infomation.

OK.

3. I do not think it's a correct describe in document for
'wal_buffers_full'.

Where should I rewrite the description? If my understanding is not
correct, please let me know.

Sorry I have not described it clearly, because I can not understand
the meaning of this
column after I read the describe in document.
And now I read the source code of walwrite and found the place where
'wal_buffers_full'
added is for a backend to wait a wal buffer which is occupied by other
wal page, so the
backend flush the old page in the wal buffer(after wait it can).
So i think the origin decribe in document is not so in point, we can
describe it such as
'Total number of times WAL data written to the disk because a backend
yelled a wal buffer
for an advanced wal page.

Sorry if my understand is wrong.

Thanks for your comments.

You're understanding is almost the same as mine.
It describes when not only backends but also other backgrounds
initialize a new wal page,
wal buffer's space is already used and there is no space.

'Total number of times WAL data written to the disk because a backend
yelled a wal buffer for an advanced wal page'

Thanks for your suggestion.
I wondered that users may confuse about how to use "wal_buffers_full"
and how to tune parameters.

I thought the reason which wal buffer has no space is
important for users to tune the wal_buffers parameter.

How about the following comments?

'Total number of times WAL data was written to the disk because WAL
buffers got full
when to initialize a new WAL page'

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION