Re: [PATCH] Support for pg_stat_archiver view

On Sat, Feb 1, 2014 at 9:32 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Jan 29, 2014 at 3:03 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Tue, Jan 28, 2014 at 3:42 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> On Sat, Jan 25, 2014 at 3:19 PM, Michael Paquier
>>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>> 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.
>>>
>>> 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.
> "Depesz" mentioned here that it would be interesting to have as well
> in this view the size of archive queue:
> http://www.depesz.com/2014/01/29/waiting-for-9-4-add-pg_stat_archiver-statistics-view/
> And it looks indeed interesting to have. What do you think about
> adding it as a TODO item?

I think that it's OK to add that as TODO item. There might be
the system that the speed of WAL archiving is slower than
that of WAL generation, at peak time. The DBA of that system
might want to monitor the size of archive queue.

We can implement this by just counting the files with .ready
extension in pg_xlog/archive_status directory. Or we can also
implement that by adding new counter field in pg_stat_archiver
structure, incrementing it whenever creating .ready file, and
decrementing it whenever changing .ready to .done.

Regards,

--
Fujii Masao