Re: Remove secondary checkpoint

On 25 October 2017 at 00:17, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> On Mon, Oct 23, 2017 at 11:20 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Remove the code that maintained two checkpoint's WAL files and all
>> associated stuff.
>>
>> Try to avoid breaking anything else
>>
>> This
>> * reduces disk space requirements on master
>> * removes a minor bug in fast failover
>> * simplifies code
>
> In short, yeah! I had a close look at the patch and noticed a couple of issues.

Thanks for the detailed review

> + else
> /*
> - * The last valid checkpoint record required for a streaming
> - * recovery exists in neither standby nor the primary.
> + * We used to attempt to go back to a secondary checkpoint
> + * record here, but only when not in standby_mode. We now
> + * just fail if we can't read the last checkpoint because
> + * this allows us to simplify processing around checkpoints.
> */
> ereport(PANIC,
> (errmsg("could not locate a valid checkpoint record")));
> - }
> Using brackets in this case is more elegant style IMO. OK, there is
> one line, but the comment is long so the block becomes
> confusingly-shaped.

OK

> /* Version identifier for this pg_control format */
> -#define PG_CONTROL_VERSION 1003
> +#define PG_CONTROL_VERSION 1100
> Yes, this had to happen anyway in this release cycle.
>
> backup.sgml describes the following:
> to higher segment numbers. It's assumed that segment files whose
> contents precede the checkpoint-before-last are no longer of
> interest and can be recycled.
> However this is not true anymore with this patch.

Thanks, will fix.

> The documentation of pg_control_checkpoint() is not updated. func.sgml
> lists the tuple fields returned for this function.

Ah, OK.

> You forgot to update pg_control_checkpoint in pg_proc.h. prior_lsn is
> still listed in the list of arguments returned. And you need to update
> as well the output list of types.
>
> * whichChkpt identifies the checkpoint (merely for reporting purposes).
> - * 1 for "primary", 2 for "secondary", 0 for "other" (backup_label)
> + * 1 for "primary", 0 for "other" (backup_label)
> + * 2 for "secondary" is no longer supported
> */
> I would suggest to just remove the reference to "secondary".

Yeh, that was there for review.

> - * Delete old log files (those no longer needed even for previous
> - * checkpoint or the standbys in XLOG streaming).
> + * Delete old log files and recycle them
> */
> Here that's more "Delete or recycle old log files". Recycling of a WAL
> segment is its renaming into a newer segment.

Sometimes files are deleted if there are too many.

> You forgot to update this formula in xlog.c:
> distance = (2.0 + CheckPointCompletionTarget) * CheckPointDistanceEstimate;
> /* add 10% for good measure. */
> distance *= 1.10;
> You can guess easily where the mistake is.

Doh - fixed that before posting, so I must have sent the wrong patch.

It's the 10%, right? ;-)

> - checkPointLoc = ControlFile->prevCheckPoint;
> + /*
> + * It appears to be a bug that we used to use
> prevCheckpoint here
> + */
> + checkPointLoc = ControlFile->checkPoint;
> Er, no. This is correct because we expect the prior checkpoint to
> still be present in the event of a failure detecting the latest
> checkpoint.

I'm removing "prevCheckPoint", so not sure what you mean.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services