Re: [PATCH] Support for pg_stat_archiver view

On Sat, Jan 25, 2014 at 5:41 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Thu, Jan 23, 2014 at 4:10 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Thu, Jan 9, 2014 at 6:36 AM, Gabriele Bartolini
>> <gabriele(dot)bartolini(at)2ndquadrant(dot)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.
Thanks, pgstat_send_archiver is cleaner now.

>
> 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
And what about archived_count and failed_count instead of respectively
archive_count and failed_count? The rest of the names are better now
indeed.

> 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.
Yep, this is definitely a different patch.

> + 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?
The first versions of the patch used a more limited size length more
inline with what you say:
+#define MAX_XFN_CHARS 40
But this is inconsistent with xlog_internal.h.
Regards,
--
Michael