Re: WAL usage calculation patch

On Mon, Mar 23, 2020 at 3:24 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> 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?

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 :(