Re: WAL usage calculation patch

On Wed, Apr 1, 2020 at 5:01 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Apr 1, 2020 at 4:29 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > v9-0005-Keep-track-of-WAL-usage-in-pg_stat_statements
> >
>
> One more comment related to this patch.
> +
> + snprintf(buf, sizeof buf, UINT64_FORMAT, tmp.wal_bytes);
> +
> + /* Convert to numeric. */
> + wal_bytes = DirectFunctionCall3(numeric_in,
> + CStringGetDatum(buf),
> + ObjectIdGetDatum(0),
> + Int32GetDatum(-1));
> +
> + values[i++] = wal_bytes;
>
> I see that other places that display uint64 values use BIGINT datatype
> in SQL, so why can't we do the same here? See the usage of queryid in
> pg_stat_statements or internal_pages, *_pages exposed via
> pgstatindex.c.

I have reviewed 0003 and 0004, I have a few comments.
v9-0003-Add-infrastructure-to-track-WAL-usage

1.
/* Points to buffer usage area in DSM */
BufferUsage *buffer_usage;
+ /* Points to WAL usage area in DSM */
+ WalUsage *wal_usage;

Better to give one blank line between the previous statement/variable
declaration and the next comment line.

/* Points to buffer usage area in DSM */
BufferUsage *buffer_usage;
---------Empty line here--------------------
+ /* Points to WAL usage area in DSM */
+ WalUsage *wal_usage;

2.
@@ -2154,7 +2157,7 @@ lazy_parallel_vacuum_indexes(Relation *Irel,
IndexBulkDeleteResult **stats,
WaitForParallelWorkersToFinish(lps->pcxt);

for (i = 0; i < lps->pcxt->nworkers_launched; i++)
- InstrAccumParallelQuery(&lps->buffer_usage[i]);
+ InstrAccumParallelQuery(&lps->buffer_usage[i], &lps->wal_usage[i]);
}

The existing comment above this loop, which just mentions the buffer
usage, not the wal usage so I guess we need to change that.
" /*
* Next, accumulate buffer usage. (This must wait for the workers to
* finish, or we might get incomplete data.)
*/"

v9-0004-Add-option-to-report-WAL-usage-in-EXPLAIN-and-aut

3.
+ if (usage->wal_num_fpw > 0)
+ appendStringInfo(es->str, " full page records=%ld",
+ usage->wal_num_fpw);
+ if (usage->wal_bytes > 0)
+ appendStringInfo(es->str, " bytes=" UINT64_FORMAT,
+ usage->wal_bytes);

Shall we change to 'full page writes' or 'full page image' instead of
full page records?

Apart from this, I have some testing to see the wal_usage with the
parallel vacuum and the results look fine.

postgres[104248]=# CREATE TABLE test (a int, b int);
CREATE TABLE
postgres[104248]=# INSERT INTO test SELECT i, i FROM
GENERATE_SERIES(1,2000000) as i;
INSERT 0 2000000
postgres[104248]=# CREATE INDEX idx1 on test(a);
CREATE INDEX
postgres[104248]=# VACUUM (PARALLEL 1) test;
VACUUM
postgres[104248]=# select query , wal_bytes, wal_records, wal_num_fpw
from pg_stat_statements where query like 'VACUUM%';
query | wal_bytes | wal_records | wal_num_fpw
--------------------------+-----------+-------------+-------------
VACUUM (PARALLEL 1) test | 72814331 | 8857 | 8855

postgres[106479]=# CREATE TABLE test (a int, b int);
CREATE TABLE
postgres[106479]=# INSERT INTO test SELECT i, i FROM
GENERATE_SERIES(1,2000000) as i;
INSERT 0 2000000
postgres[106479]=# CREATE INDEX idx1 on test(a);
CREATE INDEX
postgres[106479]=# VACUUM (PARALLEL 0) test;
VACUUM
postgres[106479]=# select query , wal_bytes, wal_records, wal_num_fpw
from pg_stat_statements where query like 'VACUUM%';
query | wal_bytes | wal_records | wal_num_fpw
--------------------------+-----------+-------------+-------------
VACUUM (PARALLEL 0) test | 72814331 | 8857 | 8855

By tomorrow, I will try to finish reviewing 0005 and 0006.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com