Re: TODO: Split out pg_resetxlog output into pre- and post-sections - Mailing list pgsql-hackers

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