Re: [HACKERS] pg_stat_wal_write statistics view - Mailing list pgsql-hackers

From Julien Rouhaud
Subject Re: [HACKERS] pg_stat_wal_write statistics view
Date
Msg-id CAOBaU_YO9EfgKih=3evVxK-31R0HUSzoewxcY7wMfx_AKnxQ6w@mail.gmail.com
Whole thread Raw
In response to Re: [HACKERS] pg_stat_wal_write statistics view  (Haribabu Kommi <kommi.haribabu@gmail.com>)
Responses Re: [HACKERS] pg_stat_wal_write statistics view
List pgsql-hackers
Hello,

On Wed, Sep 13, 2017 at 3:01 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> I ran the latest performance tests with and without IO times, there is an
> overhead involved with IO time calculation and didn't observe any
> performance
> overhead with normal stats. May be we can enable the IO stats only in the
> development environment to find out the IO stats?
>

-1 for me to have these columns depending on a GUC *and* wether it's a
debug or assert build (unless this behaviour already exists for other
functions, but AFAIK that's not the case).

> I added the other background stats to find out how much WAL write is
> carried out by the other background processes. Now I am able to collect
> the stats for the other background processes also after the pgbench test.
> So I feel now the separate background stats may be useful.
>
> Attached latest patch, performance test results and stats details with
> separate background stats and also combine them with backend including
> the IO stats also.
>

The results seem to vary too much to have a strong opinion, but it
looks like the timing instrumentation can be too expensive, so I'd be
-1 on adding this overhead to track_io_timing.

I have some minor comments on the last patch:

+    <row>
+      <entry><structfield>backend_writes</></entry>
+      <entry><type>bigint</type></entry>
+      <entry>Number of WAL writes that are carried out by the backend
process</entry>

it should be backend processes

+#define NUM_PG_STAT_WALWRITE_ATTS 16
+
+Datum
+pg_stat_get_walwrites(PG_FUNCTION_ARGS)
+{
+   TupleDesc   tupdesc;
+   Datum       values[NUM_PG_STAT_WALWRITE_ATTS];
+   bool        nulls[NUM_PG_STAT_WALWRITE_ATTS];

For consistency, the #define should be just before the tupdesc
declaration, and be named PG_STAT_GET_WALWRITE_COLS


+   PgStat_Counter backend_writes;      /* No of writes by backend */

+   PgStat_Counter backend_dirty_writes;        /* No of dirty writes by
+                                                * backend when WAL buffers
+                                                * full */

+   PgStat_Counter backend_write_blocks;    /* Total no of pages
written by backend */

+   PgStat_Counter backend_total_write_time;    /* Total write time in
+                                                * milliseconds by backend */

+   PgStat_Counter backend_total_sync_time;     /* Total write time in
+                                                * milliseconds by backend */

these comments should all be say "backends" for consistency

+-- There will surely and maximum one record
+select count(*) > 0 as ok from pg_stat_walwrites;

The test should be count(*) = 1


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

pgsql-hackers by date:

Previous
From: Masahiko Sawada
Date:
Subject: Re: [HACKERS] Assertion failure when the non-exclusive pg_stop_backup aborted.
Next
From: Kyotaro HORIGUCHI
Date:
Subject: Re: [HACKERS] GUC for cleanup indexes threshold.