Re: WAL usage calculation patch

On Wed, Apr 1, 2020 at 8:00 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> Hi,
>
> I'm replying here to all reviews that have been sent, thanks a lot!
>
> On Wed, Apr 01, 2020 at 04:29:16PM +0530, Amit Kapila wrote:
> > On Wed, Apr 1, 2020 at 1:32 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > >
> > > So here's a v9, rebased on top of the latest versions of Sawada-san's bug fixes
> > > (Amit's v6 for vacuum and Sawada-san's v2 for create index), with all
> > > previously mentionned changes.
> > >
> >
> > Few other comments:
> > v9-0003-Add-infrastructure-to-track-WAL-usage
> > 1.
> >  static void BufferUsageAdd(BufferUsage *dst, const BufferUsage *add);
> > -
> > +static void WalUsageAdd(WalUsage *dst, WalUsage *add);
> >
> > Looks like a spurious line removal
>
>
> Fixed.
>
>
> > 2.
> > + /* Report a full page imsage constructed for the WAL record */
> > + *num_fpw += 1;
> >
> > Typo. /imsage/image
>
>
> Ah sorry I though I fixed it previously, fixed.
>
>
> > 3. Doing some testing with and without parallelism to ensure WAL usage
> > data is correct would be great and if possible, share the results?
>
>
> I just saw that Dilip did some testing, but just in case here is some
> additional one
>
> - vacuum, after a truncate, loading 1M row and a "UPDATE t1 SET id = id"
>
> =# select query, calls, wal_bytes, wal_records, wal_num_fpw from pg_stat_statements where query ilike '%vacuum%';
>          query          | calls | wal_bytes | wal_records | wal_num_fpw
> ------------------------+-------+-----------+-------------+-------------
>  vacuum (parallel 3) t1 |     1 |  20098962 |       34104 |           2
>  vacuum (parallel 0) t1 |     1 |  20098962 |       34104 |           2
> (2 rows)
>
> - create index, overload t1's parallel_workers, using the 1M line just
>   vacuumed:
>
> =# alter table t1 set (parallel_workers = 2);
> ALTER TABLE
>
> =# create index t1_parallel_2 on t1(id);
> CREATE INDEX
>
> =# alter table t1 set (parallel_workers = 0);
> ALTER TABLE
>
> =# create index t1_parallel_0 on t1(id);
> CREATE INDEX
>
> =# select query, calls, wal_bytes, wal_records, wal_num_fpw from pg_stat_statements where query ilike '%create index%';
>                 query                 | calls | wal_bytes | wal_records | wal_num_fpw
> --------------------------------------+-------+-----------+-------------+-------------
>  create index t1_parallel_0 on t1(id) |     1 |  20355540 |        2762 |        2745
>  create index t1_parallel_2 on t1(id) |     1 |  20406811 |        2762 |        2758
> (2 rows)
>
> It all looks good to me.
>
>
> > v9-0005-Keep-track-of-WAL-usage-in-pg_stat_statements
> > 4.
> > +-- SELECT usage data, check WAL usage is reported, wal_records equal
> > rows count for INSERT/UPDATE/DELETE
> > +SELECT query, calls, rows,
> > +wal_bytes > 0 as wal_bytes_generated,
> > +wal_records > 0 as wal_records_generated,
> > +wal_records = rows as wal_records_as_rows
> > +FROM pg_stat_statements ORDER BY query COLLATE "C";
> > +                              query                               |
> > calls | rows | wal_bytes_generated | wal_records_generated |
> > wal_records_as_rows
> > +------------------------------------------------------------------+-------+------+---------------------+-----------------------+---------------------
> > + DELETE FROM pgss_test WHERE a > $1                               |
> >   1 |    1 | t                   | t                     | t
> > + DROP TABLE pgss_test                                             |
> >   1 |    0 | t                   | t                     | f
> > + INSERT INTO pgss_test (a, b) VALUES ($1, $2), ($3, $4), ($5, $6) |
> >   1 |    3 | t                   | t                     | t
> > + INSERT INTO pgss_test VALUES(generate_series($1, $2), $3)        |
> >   1 |   10 | t                   | t                     | t
> > + SELECT * FROM pgss_test ORDER BY a                               |
> >   1 |   12 | f                   | f                     | f
> > + SELECT * FROM pgss_test WHERE a > $1 ORDER BY a                  |
> >   2 |    4 | f                   | f                     | f
> > + SELECT * FROM pgss_test WHERE a IN ($1, $2, $3, $4, $5)          |
> >   1 |    8 | f                   | f                     | f
> > + SELECT pg_stat_statements_reset()                                |
> >   1 |    1 | f                   | f                     | f
> > + SET pg_stat_statements.track_utility = FALSE                     |
> >   1 |    0 | f                   | f                     | t
> > + UPDATE pgss_test SET b = $1 WHERE a = $2                         |
> >   6 |    6 | t                   | t                     | t
> > + UPDATE pgss_test SET b = $1 WHERE a > $2                         |
> >   1 |    3 | t                   | t                     | t
> > +(11 rows)
> > +
> >
> > I am not sure if the above tests make much sense as they are just
> > testing that if WAL is generated for these commands.  I understand it
> > is not easy to make these tests reliable but in that case, we can
> > think of some simple tests.  It seems to me that the difficulty is due
> > to full_page_writes as that depends on the checkpoint.  Can we make
> > full_page_writes = off for these tests and check some simple
> > Insert/Update/Delete cases?  Alternatively, if you can present the
> > reason why that is unstable or are tricky to write, then we can simply
> > get rid of these tests because I don't see tests for BufferUsage.  Let
> > not write tests for the sake of writing it unless they can detect bugs
> > in the future or are meaningfully covering the new code added.
>
>
> I don't think that we can have any hope in a stable amount of WAL bytes
> generated, so testing a positive number looks sensible to me.  Then testing
> that each 1-line-write query generates a WAL record also looks sensible, so I
> kept this.  I realized that Kirill used an existing set of queries that were
> previously added to validate the multi queries commands behavior, so there's no
> need to have all of them again.  I just kept one of each (insert, update,
> delete, select) to make sure that we do record WAL activity there, but I don't
> think that more can really be done.  I still think that this is better than
> nothing, but if you disagree feel free to drop those tests.
>
>
> > 5.
> > -SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
> > -               query               | calls | rows
> > ------------------------------------+-------+------
> > - SELECT $1::TEXT                   |     1 |    1
> > - SELECT PLUS_ONE($1)               |     2 |    2
> > - SELECT PLUS_TWO($1)               |     2 |    2
> > - SELECT pg_stat_statements_reset() |     1 |    1
> > +SELECT query, calls, rows, wal_bytes, wal_records FROM
> > pg_stat_statements ORDER BY query COLLATE "C";
> > +               query               | calls | rows | wal_bytes | wal_records
> > +-----------------------------------+-------+------+-----------+-------------
> > + SELECT $1::TEXT                   |     1 |    1 |         0 |           0
> > + SELECT PLUS_ONE($1)               |     2 |    2 |         0 |           0
> > + SELECT PLUS_TWO($1)               |     2 |    2 |         0 |           0
> > + SELECT pg_stat_statements_reset() |     1 |    1 |         0 |           0
> >  (4 rows)
> >
> > Again, I am not sure if these modifications make much sense?
>
>
> Those are queries that were previously executed.  As those are read-only query,
> that are pretty much guaranteed to not cause any WAL activity, I don't see how
> it hurts to test at the same time that that's we indeed record with
> pg_stat_statements, just to be safe.  Once again, feel free to drop the extra
> wal_* columns from the output if you disagree.
>
>
> > 6.
> >  static void pgss_shmem_startup(void);
> > @@ -313,6 +318,7 @@ static void pgss_store(const char *query, uint64 queryId,
> >      int query_location, int query_len,
> >      double total_time, uint64 rows,
> >      const BufferUsage *bufusage,
> > +    const WalUsage* walusage,
> >      pgssJumbleState *jstate);
> >
> > The alignment for walusage doesn't seem to be correct. Running
> > pgindent will fix this.
>
>
> Indeed, fixed.
>
> > 7.
> > + values[i++] = Int64GetDatumFast(tmp.wal_records);
> > + values[i++] = UInt64GetDatum(tmp.wal_num_fpw);
> >
> > Why are they different?  I think we should use the same *GetDatum API
> > (probably Int64GetDatumFast) for these.
>
>
> Oops, that's a mistake from when I was working on the wal_bytes output, fixed.
>
> > > 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.
>
>
> That's because it's harmless to report a signed number for a hash (at least
> comapred to the overhead of having it unsigned), while that's certainly not
> wanted to report a negative amount of WAL bytes generated if it goes beyond
> bigint limit.  See the usage of pg_lsn_mi in pg_lsn.c for instance.
>
> On Wed, Apr 01, 2020 at 07:20:31PM +0530, Dilip Kumar wrote:
> >
> > 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.
>
>
> Fixed.
>
>
> > 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.
>
>
> Ah indeed, I thought I caught all the comments but missed this one.  Fixed.
>
>
> > 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?
>
>
> Indeed, I changed it in the (auto)vacuum output but missed this one.  Fixed.
>
>
> > 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
>
>
> Thanks!  I did some similar testing, with also seq/parallel index creation and
> got similar results.
>
>
> > By tomorrow, I will try to finish reviewing 0005 and 0006.

I have reviewed these patches and I have a few cosmetic comments.
v10-0005-Keep-track-of-WAL-usage-in-pg_stat_statements

1.
+ uint64 wal_bytes; /* total amount of wal bytes written */
+ int64 wal_records; /* # of wal records written */
+ int64 wal_num_fpw; /* # of full page wal records written */


/s/# of full page wal records written / /* # of WAL full page image produced */

2.
 static void pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
  ProcessUtilityContext context, ParamListInfo params,
  QueryEnvironment *queryEnv,
- DestReceiver *dest, QueryCompletion *qc);
+ DestReceiver *dest, QueryCompletion * qc);

Useless hunk.

3.

v10-0006-Expose-WAL-usage-counters-in-verbose-auto-vacuum

@@ -3105,7 +3105,7 @@ show_wal_usage(ExplainState *es, const WalUsage *usage)
  {
  ExplainPropertyInteger("WAL records", NULL,
     usage->wal_records, es);
- ExplainPropertyInteger("WAL full page records", NULL,
+ ExplainPropertyInteger("WAL full page writes", NULL,
     usage->wal_num_fpw, es);
Just noticed that in 0004 you have first added "WAL full page
records", which is later corrected to "WAL full page writes" in 0006.
I think we better keep this proper in 0004 itself and avoid this hunk
in 0006, otherwise, it creates confusion while reviewing.

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