TODO: Split out pg_resetxlog output into pre- and post-sections

This patch implements the following TODO item:

Split out pg_resetxlog output into pre- and post-sections

http://archives.postgresql.org/pgsql-hackers/2010-08/msg02040.php

On execution of pg_resetxlog using the option -n
1. It will display values in two section.
2. First section will be called as "Current pg_control values or Guess pg_control values".
3. In first section, it will display all current (i.e. before change) values of control file or guessed values.
4. Second section will be called as "Values to be used after reset".
5. In second section, it will display new values of parameters to be reset as per user request.

Please provide your opinion or expectation out of this patch.

I will add the same to November commitFest.

Thanks and Regards,
Kumar Rajeev Rastogi

On Tue, Nov 5, 2013 at 3:20 PM, Rajeev rastogi
<rajeev(dot)rastogi(at)huawei(dot)com> wrote:
> On execution of pg_resetxlog using the option -n
>
> 1. It will display values in two section.
>
> 2. First section will be called as "Current pg_control
> values or Guess pg_control values".
>
> 3. In first section, it will display all current (i.e.
> before change) values of control file or guessed values.
>
> 4. Second section will be called as "Values to be used after
> reset".
>
> 5. In second section, it will display new values of
> parameters to be reset as per user request.
>
>
>
> Please provide your opinion or expectation out of this patch.

Your approach in patch seems to be inline with Todo item. On a quick
glance, I observed few things which can make your patch better:

1. The purpose was to print pg_control values in one section and any
other reset values in different section, so in that
regard, should we display below in new section, as here
newXlogSegNo is not directly from pg_control.

PrintControlValues()
{
..
XLogFileName(fname, ControlFile.checkPointCopy.ThisTimeLineID, newXlogSegNo);

printf(_("First log segment after reset: %s\n"),
fname);
}

2. why to have extra flags for changedParam, can't we do without it.
For example, we already use set_xid value to check if user has provided xid.

3.
+ static void
+ PrintNewControlValues(int changedParam)
+ {
+ if (changedParam)
+ printf(_("\n\nValues to be used after reset:\n\n"));

Even after first if check fails, still you continue to check other
values, it is better if you can have check if (changedParam) before
calling this function

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, 08 November 2013 09:47

> On Tue, Nov 5, 2013 at 3:20 PM, Rajeev rastogi
> <rajeev(dot)rastogi(at)huawei(dot)com> wrote:
> > On execution of pg_resetxlog using the option -n
> >
> > 1. It will display values in two section.
> >
> > 2. First section will be called as "Current
> pg_control
> > values or Guess pg_control values".
> >
> > 3. In first section, it will display all current (i.e.
> > before change) values of control file or guessed values.
> >
> > 4. Second section will be called as "Values to be
> used
> > after reset".
> >
> > 5. In second section, it will display new values of
> > parameters to be reset as per user request.
> >
> >
> >
> > Please provide your opinion or expectation out of this patch.
>
> Your approach in patch seems to be inline with Todo item. On a quick
> glance, I observed few things which can make your patch better:
>
> 1. The purpose was to print pg_control values in one section and any
> other reset values in different section, so in that
> regard, should we display below in new section, as here newXlogSegNo
> is not directly from pg_control.
>
> PrintControlValues()
> {
> ..
> XLogFileName(fname, ControlFile.checkPointCopy.ThisTimeLineID,
> newXlogSegNo);
>
> printf(_("First log segment after reset: %s\n"),
> fname);
> }

Yes we can print newXlogSegNo.

> 2. why to have extra flags for changedParam, can't we do without it.
> For example, we already use set_xid value to check if user has provided
> xid.

You mean to say that we can print wherever values of control file parameter is
getting assigned.

If yes, then the problem is that every places will have to check the condition as
if(noupdate) and then print the corresponding value. E.g.
If (noupdate)
printf(_("NextMultiXactId:%u\n"), ControlFile.checkPointCopy.nextMulti);

> 3.
> + static void
> + PrintNewControlValues(int changedParam) {
> + if (changedParam)
> + printf(_("\n\nValues to be used after reset:\n\n"));
>
> Even after first if check fails, still you continue to check other
> values, it is better if you can have check if (changedParam) before
> calling this function

Yes you are correct.
But now this check will not be required because always at-least one value (i.e. of newXlogSegNo)
will be printed.

Please let me know if understanding of all comments are correct.
After that I will update the patch.

Thanks and Regards,
Kumar Rajeev Rastogi

On Fri, Nov 8, 2013 at 10:37 AM, Rajeev rastogi
<rajeev(dot)rastogi(at)huawei(dot)com> wrote:
> On Fri, 08 November 2013 09:47
>
>> On Tue, Nov 5, 2013 at 3:20 PM, Rajeev rastogi
>> <rajeev(dot)rastogi(at)huawei(dot)com> wrote:
>> > On execution of pg_resetxlog using the option -n
>> >
>> > Please provide your opinion or expectation out of this patch.
>>
>> Your approach in patch seems to be inline with Todo item. On a quick
>> glance, I observed few things which can make your patch better:
>>
>> 1. The purpose was to print pg_control values in one section and any
>> other reset values in different section, so in that
>> regard, should we display below in new section, as here newXlogSegNo
>> is not directly from pg_control.
>>
>> PrintControlValues()
>> {
>> ..
>> XLogFileName(fname, ControlFile.checkPointCopy.ThisTimeLineID,
>> newXlogSegNo);
>>
>> printf(_("First log segment after reset: %s\n"),
>> fname);
>> }
>
> Yes we can print newXlogSegNo.

I think then your documentation also need updates.

>> 2. why to have extra flags for changedParam, can't we do without it.
>> For example, we already use set_xid value to check if user has provided
>> xid.
>
> You mean to say that we can print wherever values of control file parameter is
> getting assigned.
>
> If yes, then the problem is that every places will have to check the condition as
> if(noupdate) and then print the corresponding value. E.g.
> If (noupdate)
> printf(_("NextMultiXactId:%u\n"), ControlFile.checkPointCopy.nextMulti);
No, not this way, may be making set_xid as file level parameters,
so that you can use them later as well.
Your current way also doesn't seem to be too unreasonable, however
it is better if you can do without it.

One more thing, I think as per this patch few parameters will be
displayed twice once in "pg_control values .." section and once in
"Values to be used after reset:", so by doing this I guess you want to
make it easier for user to refer both pg_control's original/guessed
value and new value after reset. Here I wonder if someone wants to
refer to original values, can't he directly use pg_controldata? Anyone
else have thoughts about how can we display values which can make
current situation better for user.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com