Re: [PATCH] Support for pg_stat_archiver view

On Tue, Jan 28, 2014 at 3:42 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> 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.

I changed the patch in this way, fixed some existing bugs (e.g.,
correct the column
names of pg_stat_archiver in rules.out), and then just committed it.

Regards,

-- 
Fujii Masao