Re: [PATCH] Support for pg_stat_archiver view

On Thu, Jan 23, 2014 at 4:10 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Jan 9, 2014 at 6:36 AM, Gabriele Bartolini
> <gabriele.bartolini@2ndquadrant.it> wrote:
>> Il 08/01/14 18:42, Simon Riggs ha scritto:
>>> Not sure I see why it needs to be an SRF. It only returns one row.
>> Attached is version 4, which removes management of SRF stages.
>
> I have been looking at v4 a bit more, and found a couple of small things:
> - a warning in pgstatfuncs.c
> - some typos and format fixing in the sgml docs
> - some corrections in a couple of comments
> - Fixed an error message related to pg_stat_reset_shared referring
> only to bgwriter and not the new option archiver. Here is how the new
> message looks:
> =# select pg_stat_reset_shared('popo');
> ERROR:  22023: unrecognized reset target: "popo"
> HINT:  Target must be "bgwriter" or "archiver"
> A new version is attached containing those fixes. I played also with
> the patch and pgbench, simulating some archive failures and successes
> while running pgbench and the reports given by pg_stat_archiver were
> correct, so I am marking this patch as "Ready for committer".

I refactored the patch further.

* Remove ArchiverStats global variable to simplify pgarch.c.
* Remove stats_timestamp field from PgStat_ArchiverStats struct because
   it's not required.

I have some review comments:

+        s.archived_wals,
+        s.last_archived_wal,
+        s.last_archived_wal_time,
+        s.failed_attempts,
+        s.last_failed_wal,
+        s.last_failed_wal_time,

The column names of pg_stat_archiver look not consistent at least to me.
What about the followings?

  archive_count
  last_archived_wal
  last_archived_time
  fail_count
  last_failed_wal
  last_failed_time

I think that it's time to rename all the variables related to pg_stat_bgwriter.
For example, it's better to change PgStat_GlobalStats to PgStat_Bgwriter.
I think that it's okay to make this change as separate patch, though.

+    char last_archived_wal[MAXFNAMELEN];    /* last WAL file archived */
+    TimestampTz last_archived_wal_timestamp;    /* last archival success */
+    PgStat_Counter failed_attempts;
+    char last_failed_wal[MAXFNAMELEN];    /* last WAL file involved
in failure */

Some hackers don't like the increase of the size of the statsfile. In order to
reduce the size as much as possible, we should use the exact size of WAL file
here instead of MAXFNAMELEN?

Regards,

--
Fujii Masao