Skip site navigation (1) Skip section navigation (2)

Re: [PATCH] Support for pg_stat_archiver view

From: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
To: Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
Cc: Gabriele Bartolini <gabriele(dot)bartolini(at)2ndquadrant(dot)it>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Magnus Hagander <magnus(at)hagander(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] Support for pg_stat_archiver view
Date: 2014-02-01 12:32:46
Message-ID: CAB7nPqSCrcZGGy_SmpT7ubSzVGNMtphYU1JJZYyapHuN46E-Tw@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
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?
-- 
Michael


In response to

Responses

pgsql-hackers by date

Next:From: Andres FreundDate: 2014-02-01 12:57:38
Subject: Re: Wait free LW_SHARED acquisition - v0.2
Previous:From: Oskari SaarenmaaDate: 2014-02-01 11:29:39
Subject: Re: [PATCH] pg_basebackup: progress report max once per second

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group