Re: WAL usage calculation patch

[Fujii Masao]

On 2020/03/23 21:01, Fujii Masao wrote:

On 2020/03/23 7:32, 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.

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

Ah right, I totally missed that when I tried to clean up the original POC.

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.

The amount per database isn't really useful, but I didn't had a better idea on
how to expose (auto)vacuum WAL usage until this:

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.

That's a way better idea!  I'm attaching the full patchset with the 3rd patch
to use this approach instead.  There's a bit a duplicate code for computing the
WalUsage, as I didn't find a better way to avoid that without exposing
WalUsageAccumDiff().

Autovacuum log sample:

2020-03-19 15:49:05.708 CET [5843] LOG:  automatic vacuum of table 
"rjuju.public.t1": index scans: 0
         pages: 0 removed, 2213 remain, 0 skipped due to pins, 0 skipped frozen
         tuples: 250000 removed, 250000 remain, 0 are dead but not yet 
removable, oldest xmin: 502
         buffer usage: 4448 hits, 4 misses, 4 dirtied
         avg read rate: 0.160 MB/s, avg write rate: 0.160 MB/s
         system usage: CPU: user: 0.13 s, system: 0.00 s, elapsed: 0.19 s
         WAL usage: 6643 records, 4 full page records, 1402679 bytes

VACUUM log sample:

# vacuum VERBOSE t1;
INFO:  vacuuming "public.t1"
INFO:  "t1": removed 50000 row versions in 443 pages
INFO:  "t1": found 50000 removable, 0 nonremovable row versions in 443 out of 
443 pages
DETAIL:  0 dead row versions cannot be removed yet, oldest xmin: 512
There were 50000 unused item identifiers.
Skipped 0 pages due to buffer pins, 0 frozen pages.
0 pages are entirely empty.
1332 WAL records, 4 WAL full page records, 306901 WAL bytes
CPU: user: 0.01 s, system: 0.00 s, elapsed: 0.01 s.
INFO:  "t1": truncated 443 to 0 pages
DETAIL:  CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s
INFO:  vacuuming "pg_toast.pg_toast_16385"
INFO:  index "pg_toast_16385_index" now contains 0 row versions in 1 pages
DETAIL:  0 index row versions were removed.
0 index pages have been deleted, 0 are currently reusable.
CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.
INFO:  "pg_toast_16385": found 0 removable, 0 nonremovable row versions in 0 
out of 0 pages
DETAIL:  0 dead row versions cannot be removed yet, oldest xmin: 513
There were 0 unused item identifiers.
Skipped 0 pages due to buffer pins, 0 frozen pages.
0 pages are entirely empty.
0 WAL records, 0 WAL full page records, 0 WAL bytes
CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.
VACUUM

Note that the 3rd patch is an addition on top of Kirill's original patch, as
this is information that would have been greatly helpful to investigate in some
performance issues I had to investigate recently.  I'd be happy to have it land
into v13, but if that's controversial or too late I'm happy to postpone it to
v14 if the infrastructure added in Kirill's patches can make it to v13.

Dear all, can we please focus on getting the core patch committed?
Given the uncertainity regarding autovacuum stats, can we please get
parts 1 and 2 into the codebase, and think about exposing autovacuum
stats later?

Here are the comments for 0001 patch.

+            /*
+             * Report a full page image constructed for the WAL record
+             */
+            pgWalUsage.wal_fp_records++;

Isn't it better to use "fpw" or "fpi" for the variable name rather than
"fp" here? In other places, "fpw" and "fpi" are used for full page
writes/image.

ISTM that this counter could be incorrect if XLogInsertRecord() determines to
calculate again whether FPI is necessary or not. No? IOW, this issue could
happen if XLogInsert() calls  XLogRecordAssemble() multiple times in
its do-while loop. Isn't this problematic?

+    long        wal_bytes;        /* size of wal records produced */

Isn't it safer to use uint64 (i.e., XLogRecPtr) as the type of this variable
rather than long?

+    shm_toc_insert(pcxt->toc, PARALLEL_KEY_WAL_USAGE, bufusage_space);

bufusage_space should be walusage_space here?

/*
  * Finish parallel execution.  We wait for parallel workers to finish, and
  * accumulate their buffer usage.
  */

There are some comments mentioning buffer usage, in execParallel.c.
For example, the top comment for ExecParallelFinish(), as the above.
These should be updated.

Here are the comments for 0002 patch.

+    OUT wal_write_bytes int8,
+    OUT wal_write_records int8,
+    OUT wal_write_fp_records int8

Isn't "write" part in the column names confusing because it's WAL
*generated* (not written) by the statement?

+RETURNS SETOF record
+AS 'MODULE_PATHNAME', 'pg_stat_statements_1_4'
+LANGUAGE C STRICT VOLATILE;

PARALLEL SAFE should be specified?

+/* contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql */

ISTM it's good timing to have also pg_stat_statements--1.8.sql since
the definition of pg_stat_statements() is changed. Thought?

+-- CHECKPOINT before WAL tests to ensure test stability
+CHECKPOINT;

Is this true? I thought you added this because the number of FPI
should be larger than zero in the subsequent test. No? But there
seems no such test. I'm not excited about adding the test checking
the number of FPI because it looks fragile, though...

+UPDATE pgss_test SET b = '333' WHERE a = 3 \;
+UPDATE pgss_test SET b = '444' WHERE a = 4 ;

Could you tell me why several queries need to be run to test
the WAL usage? Isn't running a few query enough for the test purpase?

Regards,

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