Re: [HACKERS] TODO: Split out pg_resetxlog output into pre- and post-sections

2013-12-12 Thread Rajeev rastogi
On 12 December 2013, Heikki Linnakangas  Wrote:
>  Further Review of this patch:
> 
>  I have done few more cosmetic changes in your patch, please find
>  the updated patch attached with this mail.
>  Kindly check once whether changes are okay.
> >>>
> >>> Changes are fine. Thanks you.
> >>
> >> I have uploaded the latest patch in CF app and marked it as Ready
> For
> >> Committer.
> >
> > Thanks a lot.
> 
> Committed, with some minor kibitzing of comments and docs. Thanks!

Thanks a lot..:-)


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] TODO: Split out pg_resetxlog output into pre- and post-sections

2013-12-12 Thread Heikki Linnakangas

On 11/29/2013 08:41 AM, Rajeev rastogi wrote:

On 29 November 2013, Amit Kapila Wrote:

Further Review of this patch:


I have done few more cosmetic changes in your patch, please find the
updated patch attached with this mail.
Kindly check once whether changes are okay.


Changes are fine. Thanks you.


I have uploaded the latest patch in CF app and marked it as Ready For
Committer.


Thanks a lot.


Committed, with some minor kibitzing of comments and docs. Thanks!

- Heikki


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] TODO: Split out pg_resetxlog output into pre- and post-sections

2013-11-28 Thread Rajeev rastogi
On 29 November 2013, Amit Kapila Wrote:
> >> >> >> Further Review of this patch:
> >>
> >> I have done few more cosmetic changes in your patch, please find the
> >> updated patch attached with this mail.
> >> Kindly check once whether changes are okay.
> >
> > Changes are fine. Thanks you.
> 
> I have uploaded the latest patch in CF app and marked it as Ready For
> Committer.

Thanks a lot.

Thanks and Regards,
Kumar Rajeev Rastogi



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] TODO: Split out pg_resetxlog output into pre- and post-sections

2013-11-28 Thread Amit Kapila
On Fri, Nov 29, 2013 at 10:00 AM, Rajeev rastogi
 wrote:
> On 29 November 2013, Amit Kapila Wrote:
>> >> >> Further Review of this patch:
>>
>> I have done few more cosmetic changes in your patch, please find the
>> updated patch attached with this mail.
>> Kindly check once whether changes are okay.
>
> Changes are fine. Thanks you.

I have uploaded the latest patch in CF app and marked it as Ready For Committer.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] TODO: Split out pg_resetxlog output into pre- and post-sections

2013-11-28 Thread Rajeev rastogi
On 29 November 2013, Amit Kapila Wrote:
> >> >> Further Review of this patch:
> >> >> b. why to display 'First log segment after reset' in 'Currrent
> >> >> pg_control values' section as now the same information
> >> >> is available in new section "Values to be used after reset:" ?
> >> >
> >> > May not be always. Suppose if user has given new value of seg no
> >> > and
> >> TLI, then it will be different.
> >> > Otherwise it will be same.
> >> > So now I have changed the code in such way that the value of XLOG
> >> > segment # read from checkpoint record gets printed as part of
> >> > current value and any further new value gets printed in Values to
> >> > be reset (This will be always at-least one plus the current value).
> >> > Because of
> >> following code in FindEndOfXLOG
> >> > xlogbytepos = newXlogSegNo *
> >> ControlFile.xlog_seg_size;
> >> > newXlogSegNo = (xlogbytepos + XLogSegSize
> -
> >> > 1)
> >> / XLogSegSize;
> >> > newXlogSegNo++;
> >>
> >> It can be different, but I don't think we should display it in both
> >> sections because:
> >> a. it doesn't belong to control file.
> >> b. if you carefully look at original link
> >> (http://www.postgresql.org/message-id/1283277511-sup-2...@alvh.no-
> >> ip.org),
> >> these values are not getting displayed in pg_control values
> section.
> >>
> >> So I suggest it is better to remove it from pg_control values
> section.
> >
> > Done as per suggestion.
> 
> I have done few more cosmetic changes in your patch, please find the
> updated patch attached with this mail.
> Kindly check once whether changes are okay.

Changes are fine. Thanks you.

Thanks and Regards,
Kumar Rajeev Rastogi


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] TODO: Split out pg_resetxlog output into pre- and post-sections

2013-11-28 Thread Amit Kapila
On Thu, Nov 28, 2013 at 12:11 PM, Rajeev rastogi
 wrote:
> On 28 November 2013, Amit Kapila Wrote:
>> >> Further Review of this patch:
>> >> b. why to display 'First log segment after reset' in 'Currrent
>> >> pg_control values' section as now the same information
>> >> is available in new section "Values to be used after reset:" ?
>> >
>> > May not be always. Suppose if user has given new value of seg no and
>> TLI, then it will be different.
>> > Otherwise it will be same.
>> > So now I have changed the code in such way that the value of XLOG
>> > segment # read from checkpoint record gets printed as part of current
>> > value and any further new value gets printed in Values to be reset
>> > (This will be always at-least one plus the current value). Because of
>> following code in FindEndOfXLOG
>> > xlogbytepos = newXlogSegNo *
>> ControlFile.xlog_seg_size;
>> > newXlogSegNo = (xlogbytepos + XLogSegSize - 1)
>> / XLogSegSize;
>> > newXlogSegNo++;
>>
>> It can be different, but I don't think we should display it in both
>> sections because:
>> a. it doesn't belong to control file.
>> b. if you carefully look at original link
>> (http://www.postgresql.org/message-id/1283277511-sup-2...@alvh.no-
>> ip.org),
>> these values are not getting displayed in pg_control values section.
>>
>> So I suggest it is better to remove it from pg_control values section.
>
> Done as per suggestion.

I have done few more cosmetic changes in your patch, please find the
updated patch attached with this mail.
Kindly check once whether changes are okay.

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


pg_resetxlogsectionV5.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] TODO: Split out pg_resetxlog output into pre- and post-sections

2013-11-27 Thread Rajeev rastogi
On 28 November 2013, Amit Kapila Wrote:
> >> Further Review of this patch:
> >> b. why to display 'First log segment after reset' in 'Currrent
> >> pg_control values' section as now the same information
> >> is available in new section "Values to be used after reset:" ?
> >
> > May not be always. Suppose if user has given new value of seg no and
> TLI, then it will be different.
> > Otherwise it will be same.
> > So now I have changed the code in such way that the value of XLOG
> > segment # read from checkpoint record gets printed as part of current
> > value and any further new value gets printed in Values to be reset
> > (This will be always at-least one plus the current value). Because of
> following code in FindEndOfXLOG
> > xlogbytepos = newXlogSegNo *
> ControlFile.xlog_seg_size;
> > newXlogSegNo = (xlogbytepos + XLogSegSize - 1)
> / XLogSegSize;
> > newXlogSegNo++;
> 
> It can be different, but I don't think we should display it in both
> sections because:
> a. it doesn't belong to control file.
> b. if you carefully look at original link
> (http://www.postgresql.org/message-id/1283277511-sup-2...@alvh.no-
> ip.org),
> these values are not getting displayed in pg_control values section.
> 
> So I suggest it is better to remove it from pg_control values section.

Done as per suggestion.

> 
> Few new comments:
> 
> 1.
> Formatting for 'OldestMulti's DB' and 'OldestXID's DB' is not
> consistent with other values, try by printing it.

Changed to align output.

> 2.
> +It will print values in two sections. In first section it will
> print all original/guessed values
> +and in second section all values to be used after reset. In
> second section filename will be
> +always printed as segment number will be at-least one more than
> current. Also if any other parameter
> +is given using appropriate switch, then those new values will be
> also printed.
> 
> a. the length of newly added lines is not in sync with previous text,
> try to keep length less than 80 chars.
> b. I think there is no need of text (In second section ), you can
> remove all text after that point.

Done.

> 3.
> ! printf(_("  -n   no update, just show extracted control
> values (for testing) and to be reset values of parameters(if
> given)\n")); I find this line too long and elaborative, try to make it
> shorter.

Changed accordingly.
 

Please provide your opinion.

Thanks and Regards,
Kumar Rajeev Rastogi


pg_resetxlogsectionV4.patch
Description: pg_resetxlogsectionV4.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] TODO: Split out pg_resetxlog output into pre- and post-sections

2013-11-27 Thread Amit Kapila
On Tue, Nov 26, 2013 at 5:33 PM, Rajeev rastogi
 wrote:
> On 26 November 2013, Amit Kapila Wrote:
>> Further Review of this patch:
>> b. why to display 'First log segment after reset' in 'Currrent
>> pg_control values' section as now the same information
>> is available in new section "Values to be used after reset:" ?
>
> May not be always. Suppose if user has given new value of seg no and TLI, 
> then it will be different.
> Otherwise it will be same.
> So now I have changed the code in such way that the value of XLOG segment # 
> read from
> checkpoint record gets printed as part of current value and any further new 
> value gets
> printed in Values to be reset (This will be always at-least one plus the 
> current value). Because
> of following code in FindEndOfXLOG
> xlogbytepos = newXlogSegNo * 
> ControlFile.xlog_seg_size;
> newXlogSegNo = (xlogbytepos + XLogSegSize - 1) / 
> XLogSegSize;
> newXlogSegNo++;

It can be different, but I don't think we should display it in both
sections because:
a. it doesn't belong to control file.
b. if you carefully look at original link
(http://www.postgresql.org/message-id/1283277511-sup-2...@alvh.no-ip.org),
these values are not getting displayed in pg_control values section.

So I suggest it is better to remove it from pg_control values section.

Few new comments:

1.
Formatting for 'OldestMulti's DB' and 'OldestXID's DB' is not
consistent with other values, try by printing it.

2.
+It will print values in two sections. In first section it will
print all original/guessed values
+and in second section all values to be used after reset. In
second section filename will be
+always printed as segment number will be at-least one more than
current. Also if any other parameter
+is given using appropriate switch, then those new values will be
also printed.

a. the length of newly added lines is not in sync with previous text,
try to keep length less than 80 chars.
b. I think there is no need of text (In second section ), you can
remove all text after that point.

3.
! printf(_("  -n   no update, just show extracted control
values (for testing) and to be reset values of parameters(if
given)\n"));
I find this line too long and elaborative, try to make it shorter.


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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] TODO: Split out pg_resetxlog output into pre- and post-sections

2013-11-26 Thread Rajeev rastogi
On 26 November 2013, Amit Kapila Wrote: 
> Further Review of this patch:
> a. there are lot of trailing whitespaces in patch.

Fixed.

> b. why to display 'First log segment after reset' in 'Currrent
> pg_control values' section as now the same information
> is available in new section "Values to be used after reset:" ?

May not be always. Suppose if user has given new value of seg no and TLI, then 
it will be different.
Otherwise it will be same. 
So now I have changed the code in such way that the value of XLOG segment # 
read from 
checkpoint record gets printed as part of current value and any further new 
value gets
printed in Values to be reset (This will be always at-least one plus the 
current value). Because
of following code in FindEndOfXLOG
xlogbytepos = newXlogSegNo * ControlFile.xlog_seg_size;
newXlogSegNo = (xlogbytepos + XLogSegSize - 1) / 
XLogSegSize;
newXlogSegNo++;

> c. I think it is better to display 'First log segment after reset' as
> file name as it was earlier.
>This utility takes filename as input, so I think we should display
> filename.

Fixed. Printing filename.

> d. I still think it is not good idea to use new parameter changedParam
> to detect which parameters are changed and the reason is
> code already has that information. We should try to use that
> information rather introducing new parameter to mean the same.

Fixed. Removed changedParam and made existing variables like set_xid
global and same is being used for check.

> e.
> if (guessed && !force)
> {
> PrintControlValues(guessed);
> if (!noupdate)
> {
> printf(_("\nIf these values seem acceptable, use -f to force
> reset.\n")); exit(1); }
> 
> Do you think there will be any chance when noupdate can be true in
> above loop, if not then why to keep the check for same?

Fixed along with the last comment.

> f.
> if (changedParam & DISPLAY_XID)
>   {
>   printf(_("NextXID:  %u\n"),
> ControlFile.checkPointCopy.nextXid);
>   printf(_("oldestXID:%u\n"),
> ControlFile.checkPointCopy.oldestXid);
>   }
> Here you are priniting oldestXid, but not oldestXidDB, if user provides
> xid both values are changed. Same is the case for multitransaction.

Fixed.

> g.
> /* newXlogSegNo will be always  printed unconditionally*/ Extra space
> between always and printed. In single line comments space should be
> provided when comment text ends, please refer other single line
> comments.

Fixed.

> In case when the values are guessed and -n option is not given, then
> this patch will not be able to distinguish the values. Don't you think
> it is better to display values in 2 sections in case of guessed values
> without -n flag as well, otherwise this utility will have 2 format's to
> display values?

Yes you are right. Now printing the values in two section in two cases:
a. -n is given or
b. If we had to guess and -f not given.

Please let know your opinion.

Thanks and Regards,
Kumar Rajeev Rastogi



pg_resetxlogsectionV3.patch
Description: pg_resetxlogsectionV3.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] TODO: Split out pg_resetxlog output into pre- and post-sections

2013-11-25 Thread Amit Kapila
On Mon, Nov 25, 2013 at 9:17 AM, Rajeev rastogi
 wrote:
> On Sat, Nov 9, 2013, Amit Kapila wrote
>

Further Review of this patch:
a. there are lot of trailing whitespaces in patch.

b. why to display 'First log segment after reset' in 'Currrent
pg_control values' section as now the same information
is available in new section "Values to be used after reset:" ?

c. I think it is better to display 'First log segment after reset' as
file name as it was earlier.
   This utility takes filename as input, so I think we should display filename.

d. I still think it is not good idea to use new parameter changedParam
to detect which parameters are changed and the reason is
code already has that information. We should try to use that
information rather introducing new parameter to mean the same.

e.
if (guessed && !force)
{
PrintControlValues(guessed);
if (!noupdate)
{
printf(_("\nIf these values seem acceptable, use -f to force reset.\n"));
exit(1);
}

Do you think there will be any chance when noupdate can be true in
above loop, if not then why to keep the check for same?

f.
if (changedParam & DISPLAY_XID)
  {
  printf(_("NextXID:  %u\n"),
ControlFile.checkPointCopy.nextXid);
  printf(_("oldestXID:%u\n"),
ControlFile.checkPointCopy.oldestXid);
  }
Here you are priniting oldestXid, but not oldestXidDB, if user
provides xid both values are changed. Same is the case
for multitransaction.

g.
/* newXlogSegNo will be always  printed unconditionally*/
Extra space between always and printed. In single line comments space
should be provided when comment text ends, please refer
other single line comments.

In case when the values are guessed and -n option is not given, then
this patch will not be able to distinguish the values. Don't you think
it is better to display values in 2 sections in case of guessed values
without -n flag as well, otherwise this utility will have 2 format's
to display values?


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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] TODO: Split out pg_resetxlog output into pre- and post-sections

2013-11-24 Thread Rajeev rastogi
On Sat, Nov 9, 2013, Amit Kapila wrote



> On Fri, Nov 8, 2013 at 10:37 AM, Rajeev rastogi

> mailto:rajeev.rast...@huawei.com>> wrote:

> > On Fri, 08 November 2013 09:47

> >

> >> On Tue, Nov 5, 2013 at 3:20 PM, Rajeev rastogi

> >> mailto:rajeev.rast...@huawei.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.



I have added to documentation.



>

> 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.



Aim of this patch is to:

1. Without this patch, if I give some parameter using -l switch and also 
provide -n switch

   Then it will display this values as "TimeLineID of latest checkpoint", which 
is not

   Really the truth.

1. So we can print both actual values and values to be used after reset in 
different section

   So that is extra clear.



Usage of pg_controldata may not be preferable in this case because:

1. User will have to use two separate executable, which can be actually 
achieved by only pg_resetxlog.

2. pg_controldata prints many other additional parameters, in which user may 
not be interested.



I have attached the updated patch.

Please let me know if it is OK or anyone else have any other idea.



Note: Replied this mail on 11th Nov also but for some reason didn't appear in 
community mail chain.



Thanks and Regards,

Kumar Rajeev Rastogi



pg_resetxlogsectionV2.patch
Description: pg_resetxlogsectionV2.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] TODO: Split out pg_resetxlog output into pre- and post-sections

2013-11-08 Thread Amit Kapila
On Fri, Nov 8, 2013 at 10:37 AM, Rajeev rastogi
 wrote:
> On Fri, 08 November 2013 09:47
>
>> On Tue, Nov 5, 2013 at 3:20 PM, Rajeev rastogi
>>  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] TODO: Split out pg_resetxlog output into pre- and post-sections

2013-11-08 Thread Rajeev rastogi
On Fri, 08 November 2013 09:47

> On Tue, Nov 5, 2013 at 3:20 PM, Rajeev rastogi
>  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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] TODO: Split out pg_resetxlog output into pre- and post-sections

2013-11-07 Thread Amit Kapila
On Tue, Nov 5, 2013 at 3:20 PM, Rajeev rastogi
 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


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

2013-11-05 Thread Rajeev rastogi
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


pg_resetxlogsection.patch
Description: pg_resetxlogsection.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers