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

On Sat, Nov 9, 2013, Amit Kapila wrote

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

> <rajeev(dot)rastogi(at)huawei(dot)com<mailto: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<mailto: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.

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

On Mon, Nov 25, 2013 at 9:17 AM, Rajeev rastogi
<rajeev(dot)rastogi(at)huawei(dot)com> 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

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

On Tue, Nov 26, 2013 at 5:33 PM, Rajeev rastogi
<rajeev(dot)rastogi(at)huawei(dot)com> 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-2152@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

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-2152@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

On Thu, Nov 28, 2013 at 12:11 PM, Rajeev rastogi
<rajeev(dot)rastogi(at)huawei(dot)com> 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-2152@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

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-2152@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

On Fri, Nov 29, 2013 at 10:00 AM, Rajeev rastogi
<rajeev(dot)rastogi(at)huawei(dot)com> 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

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

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

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