WAL usage calculation patch

On Wed, 5 Feb 2020 at 21:36, Kirill Bychik <kirill.bychik@gmail.com> wrote:

Hello pgsql-hackers,

Submitting a patch that would enable gathering of per-statement WAL
generation statistics, similar to how it is done for buffer usage.
Collected is the number of records added to WAL and number of WAL
bytes written.

The data collected was found valuable to analyze update-heavy load,
with WAL generation being the bottleneck.

The usage data is collected at low level, after compression is done on
WAL record. Data is then exposed via pg_stat_statements, could also be
used in EXPLAIN ANALYZE if needed. Instrumentation is alike to the one
used for buffer stats. I didn't dare to unify both usage metric sets
into single struct, nor rework the way both are passed to parallel
workers.

Performance impact is (supposed to be) very low, essentially adding
two int operations and memory access on WAL record insert. Additional
efforts to allocate shmem chunk for parallel workers. Parallel workers
shmem usage is increased to fir in a struct of two longs.

Patch is separated in two parts: core changes and pg_stat_statements
additions. Essentially the extension has its schema updated to allow
two more fields, docs updated to reflect the change. Patch is prepared
against master branch.

Please provide your comments and/or code findings.

I like the concept, I'm a big fan of anything that affordably improves
visibility into Pg's I/O and activity.

To date I've been relying on tools like systemtap to do this sort of
thing. But that's a bit specialised, and Pg currently lacks useful
instrumentation for it so it can be a pain to match up activity by
parallel workers and that sort of thing. (I aim to find time to submit
a patch for that.)

I haven't yet reviewed the patch.

--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise

On Mon, Feb 10, 2020 at 8:20 PM Craig Ringer <craig@2ndquadrant.com> wrote:

On Wed, 5 Feb 2020 at 21:36, Kirill Bychik <kirill.bychik@gmail.com> wrote:

Patch is separated in two parts: core changes and pg_stat_statements
additions. Essentially the extension has its schema updated to allow
two more fields, docs updated to reflect the change. Patch is prepared
against master branch.

Please provide your comments and/or code findings.

I like the concept, I'm a big fan of anything that affordably improves
visibility into Pg's I/O and activity.

+1

To date I've been relying on tools like systemtap to do this sort of
thing. But that's a bit specialised, and Pg currently lacks useful
instrumentation for it so it can be a pain to match up activity by
parallel workers and that sort of thing. (I aim to find time to submit
a patch for that.)

(I'm interested in seeing your conference talk about that! I did a
bunch of stuff with static probes to measure PHJ behaviour around
barrier waits and so on but it was hard to figure out what stuff like
that to put in the actual tree, it was all a bit
use-once-to-test-a-theory-and-then-throw-away.)

Kirill, I noticed that you included a regression test that is failing. Can
this possibly be stable across machines or even on the same machine?
Does it still pass for you or did something change on the master
branch to add a new WAL record since you posted the patch?

query | calls | rows | wal_write_bytes | wal_write_records
 -------------------------------------------+-------+------+-----------------+-------------------
- CREATE INDEX test_b ON test(b)            |     1 |    0 | 1673 |
            16
- DROP FUNCTION IF EXISTS PLUS_ONE(INTEGER) |     1 |    0 |   56 |
             1
+ CREATE INDEX test_b ON test(b)            |     1 |    0 | 1755 |
            17
+ DROP FUNCTION IF EXISTS PLUS_ONE(INTEGER) |     1 |    0 |    0 |
             0

вт, 18 февр. 2020 г. в 06:23, Thomas Munro <thomas.munro@gmail.com>:

On Mon, Feb 10, 2020 at 8:20 PM Craig Ringer <craig@2ndquadrant.com> wrote:

On Wed, 5 Feb 2020 at 21:36, Kirill Bychik <kirill.bychik@gmail.com> wrote:

Patch is separated in two parts: core changes and pg_stat_statements
additions. Essentially the extension has its schema updated to allow
two more fields, docs updated to reflect the change. Patch is prepared
against master branch.

Please provide your comments and/or code findings.

I like the concept, I'm a big fan of anything that affordably improves
visibility into Pg's I/O and activity.

+1

To date I've been relying on tools like systemtap to do this sort of
thing. But that's a bit specialised, and Pg currently lacks useful
instrumentation for it so it can be a pain to match up activity by
parallel workers and that sort of thing. (I aim to find time to submit
a patch for that.)

(I'm interested in seeing your conference talk about that! I did a
bunch of stuff with static probes to measure PHJ behaviour around
barrier waits and so on but it was hard to figure out what stuff like
that to put in the actual tree, it was all a bit
use-once-to-test-a-theory-and-then-throw-away.)

Kirill, I noticed that you included a regression test that is failing. Can
this possibly be stable across machines or even on the same machine?
Does it still pass for you or did something change on the master
branch to add a new WAL record since you posted the patch?

Thank you for testing the patch and running extension checks. I assume
the patch applies without problems.

As for the regr test, it apparently requires some rework. I didn't pay
attention enough to make sure the data I check is actually meaningful
and isolated enough to be repeatable.

Please consider the extension part of the patch as WIP, I'll resubmit
the patch once I get a stable and meanngful test up. Thanks for
finding it!

вт, 18 февр. 2020 г. в 06:23, Thomas Munro <thomas.munro@gmail.com>:

On Mon, Feb 10, 2020 at 8:20 PM Craig Ringer <craig@2ndquadrant.com> wrote:

On Wed, 5 Feb 2020 at 21:36, Kirill Bychik <kirill.bychik@gmail.com> wrote:

Patch is separated in two parts: core changes and pg_stat_statements
additions. Essentially the extension has its schema updated to allow
two more fields, docs updated to reflect the change. Patch is prepared
against master branch.

Please provide your comments and/or code findings.

I like the concept, I'm a big fan of anything that affordably improves
visibility into Pg's I/O and activity.

+1

To date I've been relying on tools like systemtap to do this sort of
thing. But that's a bit specialised, and Pg currently lacks useful
instrumentation for it so it can be a pain to match up activity by
parallel workers and that sort of thing. (I aim to find time to submit
a patch for that.)

(I'm interested in seeing your conference talk about that! I did a
bunch of stuff with static probes to measure PHJ behaviour around
barrier waits and so on but it was hard to figure out what stuff like
that to put in the actual tree, it was all a bit
use-once-to-test-a-theory-and-then-throw-away.)

Kirill, I noticed that you included a regression test that is failing. Can
this possibly be stable across machines or even on the same machine?
Does it still pass for you or did something change on the master
branch to add a new WAL record since you posted the patch?

Thank you for testing the patch and running extension checks. I assume
the patch applies without problems.

As for the regr test, it apparently requires some rework. I didn't pay
attention enough to make sure the data I check is actually meaningful
and isolated enough to be repeatable.

Please consider the extension part of the patch as WIP, I'll resubmit
the patch once I get a stable and meanngful test up. Thanks for
finding it!

I have reworked the extension regression test to be more isolated.
Apparently, something merged into master branch shifted my numbers.

PFA the new patch. Core part didn't change a bit, the extension part
has regression test SQL and expected log changed.

Looking forward for new comments.

On Thu, Feb 20, 2020 at 06:56:27PM +0300, Kirill Bychik wrote:

вт, 18 февр. 2020 г. в 06:23, Thomas Munro <thomas.munro@gmail.com>:

On Mon, Feb 10, 2020 at 8:20 PM Craig Ringer <craig@2ndquadrant.com> wrote:

On Wed, 5 Feb 2020 at 21:36, Kirill Bychik <kirill.bychik@gmail.com> wrote:

Patch is separated in two parts: core changes and pg_stat_statements
additions. Essentially the extension has its schema updated to allow
two more fields, docs updated to reflect the change. Patch is prepared
against master branch.

Please provide your comments and/or code findings.

I like the concept, I'm a big fan of anything that affordably improves
visibility into Pg's I/O and activity.

+1

Huge +1 too.

Thank you for testing the patch and running extension checks. I assume
the patch applies without problems.

As for the regr test, it apparently requires some rework. I didn't pay
attention enough to make sure the data I check is actually meaningful
and isolated enough to be repeatable.

Please consider the extension part of the patch as WIP, I'll resubmit
the patch once I get a stable and meanngful test up. Thanks for
finding it!

I have reworked the extension regression test to be more isolated.
Apparently, something merged into master branch shifted my numbers.

PFA the new patch. Core part didn't change a bit, the extension part
has regression test SQL and expected log changed.

I'm quite worried about the stability of those counters for regression tests.
Wouldn't a checkpoint happening during the test change them?

While at it, did you consider adding a full-page image counter in the WalUsage?
That's something I'd really like to have and it doesn't seem hard to integrate.

Another point is that this patch won't help to see autovacuum activity.
As an example, I did a quick test to store the informations in pgstat, sending
the data in the PG_FINALLY part of vacuum():

rjuju=# create table t1(id integer, val text);
CREATE TABLE
rjuju=# insert into t1 select i, 'val ' || i from generate_series(1, 100000) i;
INSERT 0 100000
rjuju=# vacuum t1;
VACUUM
rjuju=# select datname, vac_wal_records, vac_wal_bytes, autovac_wal_records, autovac_wal_bytes
from pg_stat_database where datname = 'rjuju';
datname | vac_wal_records | vac_wal_bytes | autovac_wal_records | autovac_wal_bytes
---------+-----------------+---------------+---------------------+-------------------
rjuju | 547 | 65201 | 0 | 0
(1 row)

rjuju=# delete from t1 where id % 2 = 0;
DELETE 50000
rjuju=# select pg_sleep(60);
pg_sleep
----------

(1 row)

rjuju=# select datname, vac_wal_records, vac_wal_bytes, autovac_wal_records, autovac_wal_bytes
from pg_stat_database where datname = 'rjuju';
datname | vac_wal_records | vac_wal_bytes | autovac_wal_records | autovac_wal_bytes
---------+-----------------+---------------+---------------------+-------------------
rjuju | 547 | 65201 | 1631 | 323193
(1 row)

That's seems like useful data (especially since I recently had to dig into a
problematic WAL consumption issue that was due to some autovacuum activity),
but that may seem strange to only account for (auto)vacuum activity, rather
than globally, grouping per RmgrId or CommandTag for instance. We could then
see the complete WAL usage per-database. What do you think?

Some minor points I noticed:

- the extension patch doesn't apply anymore, I guess since 70a7732007bc4689

#define PARALLEL_KEY_JIT_INSTRUMENTATION UINT64CONST(0xE000000000000009)
+#define PARALLEL_KEY_WAL_USAGE UINT64CONST(0xE000000000000010)

Shouldn't it be 0xA rather than 0x10?

- it would be better to add a version number to the patches, so we're sure
which one we're talking about.

On Wed, Mar 04, 2020 at 05:02:25PM +0100, Julien Rouhaud wrote:

I'm quite worried about the stability of those counters for regression tests.
Wouldn't a checkpoint happening during the test change them?

Yep. One way to go through that would be to test if this output is
non-zero still I suspect at quick glance that this won't be entirely
reliable either.

While at it, did you consider adding a full-page image counter in the WalUsage?
That's something I'd really like to have and it doesn't seem hard to integrate.

FWIW, one reason here is that we had recently some benchmark work done
internally where this would have been helpful in studying some spiky
WAL load patterns.
--
Michael

I'm quite worried about the stability of those counters for regression tests.
Wouldn't a checkpoint happening during the test change them?

Agree, stability of test could be an issue, even shifting of write
format or compression method or adding compatible changes could break
such test. Frankly speaking, the numbers expected are not actually
calculated, my logic was rather well described by "these numbers
should be non-zero for real tables". I believe the test can be
modified to check that numbers are above zero, both for bytes written
and for records stored.

Having a checkpoint in the moddle of the test can be almost 100%
countered by triggering one before the test. I'll add a checkpoint
call to the test scenario, if no objections here.

While at it, did you consider adding a full-page image counter in the WalUsage?
That's something I'd really like to have and it doesn't seem hard to integrate.

Well, not sure I understand you 100%, being new to Postgres dev. Do
you want a separate counter for pages written whenever doPageWrites is
true? I can do that, if needed. Please confirm.

Another point is that this patch won't help to see autovacuum activity.
As an example, I did a quick te.....
...LONG QUOTE...
but that may seem strange to only account for (auto)vacuum activity, rather
than globally, grouping per RmgrId or CommandTag for instance. We could then
see the complete WAL usage per-database. What do you think?

I wanted to keep the patch small and simple, and fit to practical
needs. This patch is supposed to provide tuning assistance, catching
an io heavy query in commit-bound situation.
Total WAL usage per DB can be assessed rather easily using other means.
Let's get this change into the codebase and then work on connecting
WAL usage to (auto)vacuum stats.

Some minor points I noticed:

- the extension patch doesn't apply anymore, I guess since 70a7732007bc4689

Will fix, thank you.

#define PARALLEL_KEY_JIT_INSTRUMENTATION UINT64CONST(0xE000000000000009)
+#define PARALLEL_KEY_WAL_USAGE UINT64CONST(0xE000000000000010)

Shouldn't it be 0xA rather than 0x10?

Oww, my bad, this is embaracing! Will fix, thank you.

- it would be better to add a version number to the patches, so we're sure
which one we're talking about.

Noted, thank you.

Please comment on the proposed changes, I will cook up a new version
once all are agreed upon.

On Thu, Mar 5, 2020 at 8:55 PM Kirill Bychik <kirill.bychik@gmail.com> wrote:

While at it, did you consider adding a full-page image counter in the WalUsage?
That's something I'd really like to have and it doesn't seem hard to integrate.

Well, not sure I understand you 100%, being new to Postgres dev. Do
you want a separate counter for pages written whenever doPageWrites is
true? I can do that, if needed. Please confirm.

Yes, I meant a separate 3rd counter for the number of full page images
written. However after a quick look I think that a FPI should be
detected with (doPageWrites && fpw_lsn != InvalidXLogRecPtr && fpw_lsn
<= RedoRecPtr).

Another point is that this patch won't help to see autovacuum activity.
As an example, I did a quick te.....
...LONG QUOTE...
but that may seem strange to only account for (auto)vacuum activity, rather
than globally, grouping per RmgrId or CommandTag for instance. We could then
see the complete WAL usage per-database. What do you think?

I wanted to keep the patch small and simple, and fit to practical
needs. This patch is supposed to provide tuning assistance, catching
an io heavy query in commit-bound situation.
Total WAL usage per DB can be assessed rather easily using other means.
Let's get this change into the codebase and then work on connecting
WAL usage to (auto)vacuum stats.

I agree that having a view of the full activity is a way bigger scope,
so it could be done later (and at this point in pg14), but I'm still
hoping that we can get insight of other backend WAL activity, such as
autovacuum, in pg13.

пт, 6 мар. 2020 г. в 20:14, Julien Rouhaud <rjuju123@gmail.com>:

On Thu, Mar 5, 2020 at 8:55 PM Kirill Bychik <kirill.bychik@gmail.com> wrote:

While at it, did you consider adding a full-page image counter in the WalUsage?
That's something I'd really like to have and it doesn't seem hard to integrate.

Well, not sure I understand you 100%, being new to Postgres dev. Do
you want a separate counter for pages written whenever doPageWrites is
true? I can do that, if needed. Please confirm.

Yes, I meant a separate 3rd counter for the number of full page images
written. However after a quick look I think that a FPI should be
detected with (doPageWrites && fpw_lsn != InvalidXLogRecPtr && fpw_lsn
<= RedoRecPtr).

This seems easy, will implement once I get some spare time.

Another point is that this patch won't help to see autovacuum activity.
As an example, I did a quick te.....
...LONG QUOTE...
but that may seem strange to only account for (auto)vacuum activity, rather
than globally, grouping per RmgrId or CommandTag for instance. We could then
see the complete WAL usage per-database. What do you think?

I wanted to keep the patch small and simple, and fit to practical
needs. This patch is supposed to provide tuning assistance, catching
an io heavy query in commit-bound situation.
Total WAL usage per DB can be assessed rather easily using other means.
Let's get this change into the codebase and then work on connecting
WAL usage to (auto)vacuum stats.

I agree that having a view of the full activity is a way bigger scope,
so it could be done later (and at this point in pg14), but I'm still
hoping that we can get insight of other backend WAL activity, such as
autovacuum, in pg13.

How do you think this information should be exposed? Via the pg_stat_statement?

Anyways, I believe this change could be bigger than FPI. I propose to
plan a separate patch for it, or even add it to the TODO after the
core patch of wal usage is merged.

Please expect a new patch version next week, with FPI counters added.

On Fri, Mar 6, 2020 at 6:59 PM Kirill Bychik <kirill.bychik@gmail.com> wrote:

пт, 6 мар. 2020 г. в 20:14, Julien Rouhaud <rjuju123@gmail.com>:

On Thu, Mar 5, 2020 at 8:55 PM Kirill Bychik <kirill.bychik@gmail.com> wrote:

I wanted to keep the patch small and simple, and fit to practical
needs. This patch is supposed to provide tuning assistance, catching
an io heavy query in commit-bound situation.
Total WAL usage per DB can be assessed rather easily using other means.
Let's get this change into the codebase and then work on connecting
WAL usage to (auto)vacuum stats.

I agree that having a view of the full activity is a way bigger scope,
so it could be done later (and at this point in pg14), but I'm still
hoping that we can get insight of other backend WAL activity, such as
autovacuum, in pg13.

How do you think this information should be exposed? Via the pg_stat_statement?

That's unlikely, since autovacuum won't trigger any hook. I was
thinking on some new view for pgstats, similarly to the example I
showed previously. The implementation is straightforward, although
pg_stat_database is maybe not the best choice here.

Anyways, I believe this change could be bigger than FPI. I propose to
plan a separate patch for it, or even add it to the TODO after the
core patch of wal usage is merged.

Just in case, if the problem is a lack of time, I'd be happy to help
on that if needed. Otherwise, I'll definitely not try to block any
progress for the feature as proposed.

Please expect a new patch version next week, with FPI counters added.

Thanks!

On Thu, Mar 5, 2020 at 8:55 PM Kirill Bychik <kirill.bychik@gmail.com> wrote:

I wanted to keep the patch small and simple, and fit to practical
needs. This patch is supposed to provide tuning assistance, catching
an io heavy query in commit-bound situation.
Total WAL usage per DB can be assessed rather easily using other means.
Let's get this change into the codebase and then work on connecting
WAL usage to (auto)vacuum stats.

I agree that having a view of the full activity is a way bigger scope,
so it could be done later (and at this point in pg14), but I'm still
hoping that we can get insight of other backend WAL activity, such as
autovacuum, in pg13.

How do you think this information should be exposed? Via the pg_stat_statement?

That's unlikely, since autovacuum won't trigger any hook. I was
thinking on some new view for pgstats, similarly to the example I
showed previously. The implementation is straightforward, although
pg_stat_database is maybe not the best choice here.

After extensive thinking and some code diving, I did not manage to
come up with a sane idea on how to expose data about autovacuum WAL
usage. Must be the flu.

Anyways, I believe this change could be bigger than FPI. I propose to
plan a separate patch for it, or even add it to the TODO after the
core patch of wal usage is merged.

Just in case, if the problem is a lack of time, I'd be happy to help
on that if needed. Otherwise, I'll definitely not try to block any
progress for the feature as proposed.

Please feel free to work on any extension of this patch idea. I lack
both time and knowledge to do it all by myself.

Please expect a new patch version next week, with FPI counters added.

Please find attached patch version 003, with FP writes and minor
corrections. Hope i use attachment versioning as expected in this
group :)

Test had been reworked, and I believe it should be stable now, the
part which checks WAL is written and there is a correlation between
affected rows and WAL records. I still have no idea how to test
full-page writes against regular updates, it seems very unstable.
Please share ideas if any.

Thanks!

On Sun, Mar 15, 2020 at 09:52:18PM +0300, Kirill Bychik wrote:

On Thu, Mar 5, 2020 at 8:55 PM Kirill Bychik <kirill.bychik@gmail.com> wrote:

After extensive thinking and some code diving, I did not manage to
come up with a sane idea on how to expose data about autovacuum WAL
usage. Must be the flu.

Anyways, I believe this change could be bigger than FPI. I propose to
plan a separate patch for it, or even add it to the TODO after the
core patch of wal usage is merged.

Just in case, if the problem is a lack of time, I'd be happy to help
on that if needed. Otherwise, I'll definitely not try to block any
progress for the feature as proposed.

Please feel free to work on any extension of this patch idea. I lack
both time and knowledge to do it all by myself.

I'm adding a 3rd patch on top of yours to expose the new WAL counters in
pg_stat_database, for vacuum and autovacuum. I'm not really enthiusiastic with
this approach but I didn't find better, and maybe this will raise some better
ideas. The only sure thing is that we're not going to add a bunch of new
fields in pg_stat_all_tables anyway.

We can also drop this 3rd patch entirely if no one's happy about it without
impacting the first two.

Please expect a new patch version next week, with FPI counters added.

Please find attached patch version 003, with FP writes and minor
corrections. Hope i use attachment versioning as expected in this
group :)

Thanks!

Test had been reworked, and I believe it should be stable now, the
part which checks WAL is written and there is a correlation between
affected rows and WAL records. I still have no idea how to test
full-page writes against regular updates, it seems very unstable.
Please share ideas if any.

I just reviewed the patches, and it globally looks good to me. The way to
detect full page images looks sensible, but I'm really not familiar with that
code so additional review would be useful.

I noticed that the new wal_write_fp_records field in pg_stat_statements wasn't
used in the test. Since I have to add all the patches to make the cfbot happy,
I slightly adapted the tests to reference the fp column too. There was also a
minor issue in the documentation, as wal_records and wal_bytes were copy/pasted
twice while wal_write_fp_records wasn't documented, so I also changed it.

Let me know if you're ok with those changes.

Please feel free to work on any extension of this patch idea. I lack
both time and knowledge to do it all by myself.

I'm adding a 3rd patch on top of yours to expose the new WAL counters in
pg_stat_database, for vacuum and autovacuum. I'm not really enthiusiastic with
this approach but I didn't find better, and maybe this will raise some better
ideas. The only sure thing is that we're not going to add a bunch of new
fields in pg_stat_all_tables anyway.

We can also drop this 3rd patch entirely if no one's happy about it without
impacting the first two.

No objections about 3rd on my side, unless we miss the CF completely.

As for the code, I believe:
+ walusage.wal_records = pgWalUsage.wal_records -
+ walusage_start.wal_records;
+ walusage.wal_fp_records = pgWalUsage.wal_fp_records -
+ walusage_start.wal_fp_records;
+ walusage.wal_bytes = pgWalUsage.wal_bytes - walusage_start.wal_bytes;

Could be done much simpler via the utility:
WalUsageAccumDiff(walusage, pgWalUsage, walusage_start);

On a side note, I agree API to the buf/wal usage is far from perfect.

Test had been reworked, and I believe it should be stable now, the
part which checks WAL is written and there is a correlation between
affected rows and WAL records. I still have no idea how to test
full-page writes against regular updates, it seems very unstable.
Please share ideas if any.

I just reviewed the patches, and it globally looks good to me. The way to
detect full page images looks sensible, but I'm really not familiar with that
code so additional review would be useful.

I noticed that the new wal_write_fp_records field in pg_stat_statements wasn't
used in the test. Since I have to add all the patches to make the cfbot happy,
I slightly adapted the tests to reference the fp column too. There was also a
minor issue in the documentation, as wal_records and wal_bytes were copy/pasted
twice while wal_write_fp_records wasn't documented, so I also changed it.

Let me know if you're ok with those changes.

Sorry for not getting wal_fp_usage into the docs, my fault.

As for the tests, please get somebody else to review this. I strongly
believe checking full page writes here could be a source of
instability.

On Tue, Mar 17, 2020 at 10:27:05PM +0300, Kirill Bychik wrote:

Please feel free to work on any extension of this patch idea. I lack
both time and knowledge to do it all by myself.

I'm adding a 3rd patch on top of yours to expose the new WAL counters in
pg_stat_database, for vacuum and autovacuum. I'm not really enthiusiastic with
this approach but I didn't find better, and maybe this will raise some better
ideas. The only sure thing is that we're not going to add a bunch of new
fields in pg_stat_all_tables anyway.

We can also drop this 3rd patch entirely if no one's happy about it without
impacting the first two.

No objections about 3rd on my side, unless we miss the CF completely.

As for the code, I believe:
+ walusage.wal_records = pgWalUsage.wal_records -
+ walusage_start.wal_records;
+ walusage.wal_fp_records = pgWalUsage.wal_fp_records -
+ walusage_start.wal_fp_records;
+ walusage.wal_bytes = pgWalUsage.wal_bytes - walusage_start.wal_bytes;

Could be done much simpler via the utility:
WalUsageAccumDiff(walusage, pgWalUsage, walusage_start);

Indeed, but this function is private to instrument.c. AFAICT
pg_stat_statements is already duplicating similar code for buffers rather than
having BufferUsageAccumDiff being exported, so I chose the same approach.

I'd be in favor of exporting both functions though.

On a side note, I agree API to the buf/wal usage is far from perfect.

Yes clearly.

Test had been reworked, and I believe it should be stable now, the
part which checks WAL is written and there is a correlation between
affected rows and WAL records. I still have no idea how to test
full-page writes against regular updates, it seems very unstable.
Please share ideas if any.

I just reviewed the patches, and it globally looks good to me. The way to
detect full page images looks sensible, but I'm really not familiar with that
code so additional review would be useful.

I noticed that the new wal_write_fp_records field in pg_stat_statements wasn't
used in the test. Since I have to add all the patches to make the cfbot happy,
I slightly adapted the tests to reference the fp column too. There was also a
minor issue in the documentation, as wal_records and wal_bytes were copy/pasted
twice while wal_write_fp_records wasn't documented, so I also changed it.

Let me know if you're ok with those changes.

Sorry for not getting wal_fp_usage into the docs, my fault.

As for the tests, please get somebody else to review this. I strongly
believe checking full page writes here could be a source of
instability.

I'm also a little bit dubious about it. The initial checkpoint should make
things stable (of course unless full_page_writes is disabled), and Cfbot also
seems happy about it. At least keeping it for the temporary tables test
shouldn't be a problem.

Please feel free to work on any extension of this patch idea. I lack
both time and knowledge to do it all by myself.

I'm adding a 3rd patch on top of yours to expose the new WAL counters in
pg_stat_database, for vacuum and autovacuum. I'm not really enthiusiastic with
this approach but I didn't find better, and maybe this will raise some better
ideas. The only sure thing is that we're not going to add a bunch of new
fields in pg_stat_all_tables anyway.

We can also drop this 3rd patch entirely if no one's happy about it without
impacting the first two.

No objections about 3rd on my side, unless we miss the CF completely.

As for the code, I believe:
+ walusage.wal_records = pgWalUsage.wal_records -
+ walusage_start.wal_records;
+ walusage.wal_fp_records = pgWalUsage.wal_fp_records -
+ walusage_start.wal_fp_records;
+ walusage.wal_bytes = pgWalUsage.wal_bytes - walusage_start.wal_bytes;

Could be done much simpler via the utility:
WalUsageAccumDiff(walusage, pgWalUsage, walusage_start);

Indeed, but this function is private to instrument.c. AFAICT
pg_stat_statements is already duplicating similar code for buffers rather than
having BufferUsageAccumDiff being exported, so I chose the same approach.

I'd be in favor of exporting both functions though.

On a side note, I agree API to the buf/wal usage is far from perfect.

Yes clearly.

There is a higher-level Instrumentation API that can be used with
INSTRUMENT_WAL flag to collect the wal usage information. I believe
the instrumentation is widely used in the executor code, so it should
not be a problem to colelct instrumentation information on autovacuum
worker level.

Just a recommendation/chat, though. I am happy with the way the data
is collected now. If you commit this variant, please add a TODO to
rework wal usage to common instr API.

Test had been reworked, and I believe it should be stable now, the
part which checks WAL is written and there is a correlation between
affected rows and WAL records. I still have no idea how to test
full-page writes against regular updates, it seems very unstable.
Please share ideas if any.

I just reviewed the patches, and it globally looks good to me. The way to
detect full page images looks sensible, but I'm really not familiar with that
code so additional review would be useful.

I noticed that the new wal_write_fp_records field in pg_stat_statements wasn't
used in the test. Since I have to add all the patches to make the cfbot happy,
I slightly adapted the tests to reference the fp column too. There was also a
minor issue in the documentation, as wal_records and wal_bytes were copy/pasted
twice while wal_write_fp_records wasn't documented, so I also changed it.

Let me know if you're ok with those changes.

Sorry for not getting wal_fp_usage into the docs, my fault.

As for the tests, please get somebody else to review this. I strongly
believe checking full page writes here could be a source of
instability.

I'm also a little bit dubious about it. The initial checkpoint should make
things stable (of course unless full_page_writes is disabled), and Cfbot also
seems happy about it. At least keeping it for the temporary tables test
shouldn't be a problem.

Temp tables should show zero FPI WAL records, true :)

I have no objections to the patch.

On Wed, Mar 18, 2020 at 09:02:58AM +0300, Kirill Bychik wrote:

There is a higher-level Instrumentation API that can be used with
INSTRUMENT_WAL flag to collect the wal usage information. I believe
the instrumentation is widely used in the executor code, so it should
not be a problem to colelct instrumentation information on autovacuum
worker level.

Just a recommendation/chat, though. I am happy with the way the data
is collected now. If you commit this variant, please add a TODO to
rework wal usage to common instr API.

The instrumentation is somewhat intended to be used with executor nodes, not
backend commands. I don't see real technical reason that would prevent that,
but I prefer to keep things as-is for now, as it sound less controversial.
This is for the 3rd patch, which may not even be considered for this CF anyway.

As for the tests, please get somebody else to review this. I strongly
believe checking full page writes here could be a source of
instability.

I'm also a little bit dubious about it. The initial checkpoint should make
things stable (of course unless full_page_writes is disabled), and Cfbot also
seems happy about it. At least keeping it for the temporary tables test
shouldn't be a problem.

Temp tables should show zero FPI WAL records, true :)

I have no objections to the patch.

I'm attaching a v5 with fp records only for temp tables, so there's no risk of
instability. As I previously said I'm fine with your two patches, so unless
you have objections on the fpi test for temp tables or the documentation
changes, I believe those should be ready for committer.

There is a higher-level Instrumentation API that can be used with
INSTRUMENT_WAL flag to collect the wal usage information. I believe
the instrumentation is widely used in the executor code, so it should
not be a problem to colelct instrumentation information on autovacuum
worker level.

Just a recommendation/chat, though. I am happy with the way the data
is collected now. If you commit this variant, please add a TODO to
rework wal usage to common instr API.

The instrumentation is somewhat intended to be used with executor nodes, not
backend commands. I don't see real technical reason that would prevent that,
but I prefer to keep things as-is for now, as it sound less controversial.
This is for the 3rd patch, which may not even be considered for this CF anyway.

As for the tests, please get somebody else to review this. I strongly
believe checking full page writes here could be a source of
instability.

I'm also a little bit dubious about it. The initial checkpoint should make
things stable (of course unless full_page_writes is disabled), and Cfbot also
seems happy about it. At least keeping it for the temporary tables test
shouldn't be a problem.

Temp tables should show zero FPI WAL records, true :)

I have no objections to the patch.

I'm attaching a v5 with fp records only for temp tables, so there's no risk of
instability. As I previously said I'm fine with your two patches, so unless
you have objections on the fpi test for temp tables or the documentation
changes, I believe those should be ready for committer.

No objections on my side either. Thank you for your review, time and efforts!

On Wed, Mar 18, 2020 at 08:48:17PM +0300, Kirill Bychik wrote:

I'm attaching a v5 with fp records only for temp tables, so there's no risk of
instability. As I previously said I'm fine with your two patches, so unless
you have objections on the fpi test for temp tables or the documentation
changes, I believe those should be ready for committer.

No objections on my side either. Thank you for your review, time and efforts!

Great, thanks also for the patches and efforts! I'll mark the entry as RFC.

On 2020/03/19 2:19, Julien Rouhaud wrote:

On Wed, Mar 18, 2020 at 09:02:58AM +0300, Kirill Bychik wrote:

There is a higher-level Instrumentation API that can be used with
INSTRUMENT_WAL flag to collect the wal usage information. I believe
the instrumentation is widely used in the executor code, so it should
not be a problem to colelct instrumentation information on autovacuum
worker level.

Just a recommendation/chat, though. I am happy with the way the data
is collected now. If you commit this variant, please add a TODO to
rework wal usage to common instr API.

The instrumentation is somewhat intended to be used with executor nodes, not
backend commands. I don't see real technical reason that would prevent that,
but I prefer to keep things as-is for now, as it sound less controversial.
This is for the 3rd patch, which may not even be considered for this CF anyway.

As for the tests, please get somebody else to review this. I strongly
believe checking full page writes here could be a source of
instability.

I'm also a little bit dubious about it. The initial checkpoint should make
things stable (of course unless full_page_writes is disabled), and Cfbot also
seems happy about it. At least keeping it for the temporary tables test
shouldn't be a problem.

Temp tables should show zero FPI WAL records, true :)

I have no objections to the patch.

I'm attaching a v5 with fp records only for temp tables, so there's no risk of
instability. As I previously said I'm fine with your two patches, so unless
you have objections on the fpi test for temp tables or the documentation
changes, I believe those should be ready for committer.

You added the columns into pg_stat_database, but seem to forget to
update the document for pg_stat_database.

Is it really reasonable to add the columns for vacuum's WAL usage into
pg_stat_database? I'm not sure how much the information about
the amount of WAL generated by vacuum per database is useful.
Isn't it better to make VACUUM VERBOSE and autovacuum log include
that information, instead, to see how much each vacuum activity
generates the WAL? Sorry if this discussion has already been done
upthread.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters