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

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