On Wed, Apr 1, 2020 at 5:01 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Apr 1, 2020 at 4:29 PM Amit Kapila <amit.kapil...@gmail.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