Re: [PATCH] Support for pg_stat_archiver view

On Sat, Jan 25, 2014 at 3:19 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Sat, Jan 25, 2014 at 5:41 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> 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.
> 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.

Using MAX_XFN_CHARS instead of MAXFNAMELEN has a clear advantage
to reduce the size of the statistics file. Though I'm not sure how much this
change improve the performance of the statistics collector, basically
I'd like to
use MAX_XFN_CHARS here.

+                (errmsg("corrupted statistics file (global) \"%s\"",
statfile)));

I think that it's better to use the view name "pg_stat_bgwriter" instead of
internal name "global" here. The view name is easier-to-understand to a user.

+                (errmsg("corrupted statistics file (archiver)
\"%s\"", statfile)));

Same as above. What using "pg_stat_archiver" instead of just "archive"?

+    if (fread(&myArchiverStats, 1, sizeof(myArchiverStats),
+              fpin) != sizeof(myArchiverStats))
+    {
+        ereport(pgStatRunningInCollector ? LOG : WARNING,
+                (errmsg("corrupted statistics file \"%s\"", statfile)));

Same as above. Isn't it better to add something like
"pg_stat_archiver" even here?

Regards,

-- 
Fujii Masao