Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

Thanks for taking a look.

On 3/3/18, 12:22 PM, "Peter Eisentraut" <peter.eisentraut@2ndquadrant.com> wrote:
> On 2/7/18 13:34, Bossart, Nathan wrote:
>> Here is a patch to allow users to change the WAL segment size of a cluster with
>> pg_resetwal.  Like the '--wal-segize' option in initdb, the new '-s' option
>> accepts segment sizes in megabytes.
>
> Or how about we call the new option, um, --wal-segsize?

It doesn't look like pg_resetwal takes any long options except for
--help and --version, although I suppose those could be added as part
of this effort.

>> The first is a division-by-zero error caused when the control data must be
>> guessed.  If the control data is damaged, WalSegSz will not be set to the
>> guessed WAL segment size, resulting in an error during XLogFromFileName().  The
>> attached patch resolves this issue by ensuring WalSegSz is set to a valid value
>> after either reading or guessing the control values.
>
> I noticed this issue in pg_controldata too.  Maybe that should be
> combined in a separate patch.

IIUC you are talking about the following code in pg_controldata:

	/*
	 * Calculate name of the WAL file containing the latest checkpoint's REDO
	 * start point.
	 */
	XLByteToSeg(ControlFile->checkPointCopy.redo, segno, WalSegSz);
	XLogFileName(xlogfilename, ControlFile->checkPointCopy.ThisTimeLineID,
				 segno, WalSegSz);

If the control file says that the WAL segment size is 0 bytes, then
XLByteToSeg() will indeed fail.  I think a similar issue is still
present for XLogFromFileName() in pg_resetwal even with this patch.
For pg_controldata, perhaps the affected output ("Latest checkpoint's
REDO WAL file") should be marked unknown if the control file specifies
a 0-byte WAL segment size.  For pg_resetwal, perhaps the WAL segment
size should be "guessed" in this case.

> The patch "Configurable file mode mask" contains the beginning of a test
> suite for pg_resetwal.  It would be great if we could get a test case
> for this new functionality implemented.

I agree.  I will take a look at that patch set.

Nathan