pg_stats_recovery view

Hi,

Attached is a patch thats implements a pg_stat_recovery view that
keeps counters about processed wal records. I just notice that it
still lacks documentation but i will add it during the week.

Because it tracks redo time this introduces to GetCurrentTimestamp()
calls to the redo main loop, so i add a track_recovery GUC so only the
people that wants the view has to spent time in those calls.

Probably the most controversial part of the patch will be the addition
of a new column in RmgrData that is a pointer to a new function for
*_short_desc() these functions are similar to the *_desc functions
that already exists and that is called via rm_desc but instead of
giving full details about the record being processed it just inform of
the type of the record, for example the 2 first columns will look
something like:

rmgr: XLOG
wal_record_type: xlog switch

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

--On 15. Januar 2012 02:50:00 -0500 Jaime Casanova <jaime(at)2ndquadrant(dot)com>
wrote:

> Attached is a patch thats implements a pg_stat_recovery view that
> keeps counters about processed wal records. I just notice that it
> still lacks documentation but i will add it during the week.

Hi Jaime,

do you have an updated patch? The current v1 patch doesn't apply cleanly
anymore, and before i go and rebase the patch i thought i'm asking...

--
Thanks

Bernd

On Thu, Jan 26, 2012 at 4:03 AM, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
>
>
> --On 15. Januar 2012 02:50:00 -0500 Jaime Casanova <jaime(at)2ndquadrant(dot)com>
> wrote:
>
>> Attached is a patch thats implements a pg_stat_recovery view that
>> keeps counters about processed wal records. I just notice that it
>> still lacks documentation but i will add it during the week.
>
>
> Hi Jaime,
>
> do you have an updated patch? The current v1 patch doesn't apply cleanly
> anymore, and before i go and rebase the patch i thought i'm asking...
>

here's the patch rebased to this morning's HEAD

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Fri, Jan 27, 2012 at 6:01 AM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
> On Thu, Jan 26, 2012 at 4:03 AM, Bernd Helmle <mailings(at)oopsware(dot)de> wrote:
>>
>>
>> --On 15. Januar 2012 02:50:00 -0500 Jaime Casanova <jaime(at)2ndquadrant(dot)com>
>> wrote:
>>
>>> Attached is a patch thats implements a pg_stat_recovery view that
>>> keeps counters about processed wal records. I just notice that it
>>> still lacks documentation but i will add it during the week.
>>
>>
>> Hi Jaime,
>>
>> do you have an updated patch? The current v1 patch doesn't apply cleanly
>> anymore, and before i go and rebase the patch i thought i'm asking...
>>
>
> here's the patch rebased to this morning's HEAD

Before reviewing the patch, I'd like to know: what's the purpose of this view?
It's only debug purpose? ISTM that most users don't care about this view at all.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Wed, Feb 1, 2012 at 9:18 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>
>>> --On 15. Januar 2012 02:50:00 -0500 Jaime Casanova <jaime(at)2ndquadrant(dot)com>
>>> wrote:
>>>
>>>> Attached is a patch thats implements a pg_stat_recovery view that
>>>> keeps counters about processed wal records. I just notice that it
>>>> still lacks documentation but i will add it during the week.
>>>
>>>
>
> Before reviewing the patch, I'd like to know: what's the purpose of this view?
> It's only debug purpose? ISTM that most users don't care about this view at all.
>

yeah! you're right. most users won't care about it... did i tell that
i added a track_recovery GUC so only users that wanted pay for it? i
probably did not tell that :D

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On Thu, Feb 2, 2012 at 08:26, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
> On Wed, Feb 1, 2012 at 9:18 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>>
>>>> --On 15. Januar 2012 02:50:00 -0500 Jaime Casanova <jaime(at)2ndquadrant(dot)com>
>>>> wrote:
>>>>
>>>>> Attached is a patch thats implements a pg_stat_recovery view that
>>>>> keeps counters about processed wal records. I just notice that it
>>>>> still lacks documentation but i will add it during the week.
>>>>
>>>>
>>
>> Before reviewing the patch, I'd like to know: what's the purpose of this view?
>> It's only debug purpose? ISTM that most users don't care about this view at all.
>>
>
> yeah! you're right. most users won't care about it... did i tell that
> i added a track_recovery GUC so only users that wanted pay for it? i
> probably did not tell that :D

I haven't looked through the code in detail, but one direct comment:
do we really need/want to send this through the stats collector? It
will only ever have one sender - perhaps we should just either store
it in shared memory or store it locally and only send it on demand?

(apologies if it already does the on-demand thing, I only spent about
30 seconds looking for it and noticed it did go through the stats
collector...)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Thu, Feb 2, 2012 at 4:26 PM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
> On Wed, Feb 1, 2012 at 9:18 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>>
>>>> --On 15. Januar 2012 02:50:00 -0500 Jaime Casanova <jaime(at)2ndquadrant(dot)com>
>>>> wrote:
>>>>
>>>>> Attached is a patch thats implements a pg_stat_recovery view that
>>>>> keeps counters about processed wal records. I just notice that it
>>>>> still lacks documentation but i will add it during the week.
>>>>
>>>>
>>
>> Before reviewing the patch, I'd like to know: what's the purpose of this view?
>> It's only debug purpose? ISTM that most users don't care about this view at all.
>>
>
> yeah! you're right. most users won't care about it... did i tell that
> i added a track_recovery GUC so only users that wanted pay for it? i
> probably did not tell that :D

If only core developer is interested in this view, ISTM that short
description for
each WAL record is not required because he or she can know the meaning of each
WAL record by reading the source code. No? Adding short descriptions for every
WAL records seems to be overkill.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Thu, Feb 2, 2012 at 4:32 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Thu, Feb 2, 2012 at 08:26, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
>> On Wed, Feb 1, 2012 at 9:18 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>>>
>>>>> --On 15. Januar 2012 02:50:00 -0500 Jaime Casanova <jaime(at)2ndquadrant(dot)com>
>>>>> wrote:
>>>>>
>>>>>> Attached is a patch thats implements a pg_stat_recovery view that
>>>>>> keeps counters about processed wal records. I just notice that it
>>>>>> still lacks documentation but i will add it during the week.
>>>>>
>>>>>
>>>
>>> Before reviewing the patch, I'd like to know: what's the purpose of this view?
>>> It's only debug purpose? ISTM that most users don't care about this view at all.
>>>
>>
>> yeah! you're right. most users won't care about it... did i tell that
>> i added a track_recovery GUC so only users that wanted pay for it? i
>> probably did not tell that :D
>
> I haven't looked through the code in detail, but one direct comment:
> do we really need/want to send this through the stats collector? It
> will only ever have one sender - perhaps we should just either store
> it in shared memory or store it locally and only send it on demand?

Agreed. I think we should either.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

--On 2. Februar 2012 17:12:11 +0900 Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> If only core developer is interested in this view, ISTM that short
> description for
> each WAL record is not required because he or she can know the meaning of each
> WAL record by reading the source code. No? Adding short descriptions for every
> WAL records seems to be overkill.

Yes, for a developer option alone adding all these *_short_desc functions looks
too much code for too less benefit. However, if someone with less code affinity
is interested to debug his server during recovery, it might be easier for him
to interpret the statistic counters.

Unfortunately i didn't manage to do it this week, but what i'm also interested
in is how large the performance regression is if the track_recovery variable is
activated. Not sure wether it really makes a big difference, but maybe
debugging recovery from a large archive could slow down to a degree, where you
want the GUC but can't afford it?

And, for display purposes, when this is intended for developers only, shouldn't
it be treated like all the other debug options as a DEVELOPER_OPTION, too?

--
Thanks

Bernd

On Thu, Feb 2, 2012 at 2:32 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>
> I haven't looked through the code in detail, but one direct comment:
> do we really need/want to send this through the stats collector? It
> will only ever have one sender - perhaps we should just either store
> it in shared memory or store it locally and only send it on demand?
>

fyi, i intend to send a reworked patch later today, it will store the
info locally and send it on demand.
about the _short_desc functions, i added that because i wanted to
understand what was happening during recovery and the wal_record_type
(xl_info) being a number is not that clear

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

Excerpts from Jaime Casanova's message of mar feb 14 04:10:58 -0300 2012:
> On Thu, Feb 2, 2012 at 2:32 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> >
> > I haven't looked through the code in detail, but one direct comment:
> > do we really need/want to send this through the stats collector? It
> > will only ever have one sender - perhaps we should just either store
> > it in shared memory or store it locally and only send it on demand?
> >
>
> fyi, i intend to send a reworked patch later today, it will store the
> info locally and send it on demand.
> about the _short_desc functions, i added that because i wanted to
> understand what was happening during recovery and the wal_record_type
> (xl_info) being a number is not that clear

Maybe it'd be clearer if you display it in hex and filter out just the
bits that are interesting for this use? IIRC xl_info carries some other
bits than the ones to identify the record type, which could be
confusing.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Tue, Feb 14, 2012 at 4:10 PM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
> On Thu, Feb 2, 2012 at 2:32 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>
>> I haven't looked through the code in detail, but one direct comment:
>> do we really need/want to send this through the stats collector? It
>> will only ever have one sender - perhaps we should just either store
>> it in shared memory or store it locally and only send it on demand?
>>
>
> fyi, i intend to send a reworked patch later today, it will store the
> info locally and send it on demand.

Jaime,
are you planning to submit the updated version of the patch?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Fri, Mar 9, 2012 at 7:20 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Tue, Feb 14, 2012 at 4:10 PM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
>> On Thu, Feb 2, 2012 at 2:32 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>>
>>> I haven't looked through the code in detail, but one direct comment:
>>> do we really need/want to send this through the stats collector? It
>>> will only ever have one sender - perhaps we should just either store
>>> it in shared memory or store it locally and only send it on demand?
>>>
>>
>> fyi, i intend to send a reworked patch later today, it will store the
>> info locally and send it on demand.
>
> Jaime,
> are you planning to submit the updated version of the patch?

Hearing no response, I have marked this patch Returned with Feedback.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company