On Tue, Jan 28, 2014 at 3:42 AM, Fujii Masao <masao.fu...@gmail.com> wrote: > On Sat, Jan 25, 2014 at 3:19 PM, Michael Paquier > <michael.paqu...@gmail.com> wrote: >> On Sat, Jan 25, 2014 at 5:41 AM, Fujii Masao <masao.fu...@gmail.com> wrote: >>> On Thu, Jan 23, 2014 at 4:10 PM, Michael Paquier >>> <michael.paqu...@gmail.com> wrote: >>>> On Thu, Jan 9, 2014 at 6:36 AM, Gabriele Bartolini >>>> <gabriele.bartol...@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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers