Re: WAL usage calculation 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.
> > >>>
> > >>> 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?
>
> FTR I marked the commitfest entry as waiting on author.
>
> Kirill do you think you'll have time to address Fuji-san's review
> shortly? The end of the commitfest is approaching quite fast :(

All these are really valuable objections. Unfortunately, I won't be
able to get all sorted out soon, due to total lack of time. I would be
very glad if somebody could step in for this patch.