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