Re: [HACKERS] TODO: Split out pg_resetxlog output into pre- and post-sections
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
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
On Thu, Nov 28, 2013 at 12:11 PM, Rajeev rastogi rajeev.rast...@huawei.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-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
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
On Fri, Nov 29, 2013 at 10:00 AM, Rajeev rastogi rajeev.rast...@huawei.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 -- 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
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
On Tue, Nov 26, 2013 at 5:33 PM, Rajeev rastogi rajeev.rast...@huawei.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-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
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
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
On Mon, Nov 25, 2013 at 9:17 AM, Rajeev rastogi
rajeev.rast...@huawei.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
--
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
On Sat, Nov 9, 2013, Amit Kapila wrote
On Fri, Nov 8, 2013 at 10:37 AM, Rajeev rastogi
rajeev.rast...@huawei.commailto:rajeev.rast...@huawei.com wrote:
On Fri, 08 November 2013 09:47
On Tue, Nov 5, 2013 at 3:20 PM, Rajeev rastogi
rajeev.rast...@huawei.commailto: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
On Fri, 08 November 2013 09:47
On Tue, Nov 5, 2013 at 3:20 PM, Rajeev rastogi
rajeev.rast...@huawei.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
--
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
On Fri, Nov 8, 2013 at 10:37 AM, Rajeev rastogi
rajeev.rast...@huawei.com wrote:
On Fri, 08 November 2013 09:47
On Tue, Nov 5, 2013 at 3:20 PM, Rajeev rastogi
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.
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
On Tue, Nov 5, 2013 at 3:20 PM, Rajeev rastogi
rajeev.rast...@huawei.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
--
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
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
