Re: Regarding WAL Format Changes

On 27.06.2012 17:14, Amit Kapila wrote:
> 1. Function header for following functions still contains referece to log,
> seg
> a. InstallXLogFileSegment()
> b. RemoveOldXlogFiles()
> c. XLogFileCopy()
> d. XLogGetLastRemoved()
> e. UpdateLastRemovedPtr()
> f. RemoveOldXlogFiles()

Thanks, fixed.

> 2. @@ -2680,8 +2645,8 @@ InstallXLogFileSegment(uint32 *log, uint32 *seg,
> char *tmppath,
> LWLockRelease(ControlFileLock);
> ereport(LOG,
> (errcode_for_file_access(),
> - errmsg("could not link file \"%s\" to
> \"%s\" (initialization of log file %u, segment %u): %m",
> - tmppath, path, *log,
> *seg)));
> + errmsg("could not link file \"%s\" to
> \"%s\" (initialization of log file): %m",
> + tmppath, path)));
> If Changed error message can contain log file and segment number, it
> would be more clear. That should be easily
> deducible from segment number.

That seems redundant. The target file name is calculated from the
segment number, and we're now using the file name instead of log+seg in
other messages too.

> 3. -RemoveOldXlogFiles(uint32 log, uint32 seg, XLogRecPtr endptr)
> +RemoveOldXlogFiles(XLogSegNo segno, XLogRecPtr endptr)
> .
> .
> .
> @@ -4016,8 +3953,9 @@ retry:
> if (!(((XLogPageHeader) readBuf)->xlp_info&
> XLP_FIRST_IS_CONTRECORD))
> {
> ereport(emode_for_corrupt_record(emode,
> *RecPtr),
> - (errmsg("there is no
> contrecord flag in log file %u, segment %u, offset %u",
> - readId,
> readSeg, readOff)));
> + (errmsg("there is no
> contrecord flag in log segment %s, offset %u",
> +
> XLogFileNameP(curFileTLI, readSegNo),
> + readOff)));
>
> goto next_record_is_invalid;
> }
> pageHeaderSize =
> XLogPageHeaderSize((XLogPageHeader) readBuf);
> @@ -4025,10 +3963,13 @@ retry:
> if (contrecord->xl_rem_len == 0 ||
> total_len != (contrecord->xl_rem_len +
> gotlen))
> {
> + char fname[MAXFNAMELEN];
> + XLogFileName(fname, curFileTLI, readSegNo);
>
> ereport(emode_for_corrupt_record(emode,
> *RecPtr),
> - (errmsg("invalid contrecord
> length %u in log file %u, segment %u, offset %u",
> + (errmsg("invalid contrecord
> length %u in log segment %s, offset %u",
>
> contrecord->xl_rem_len,
> - readId,
> readSeg, readOff)));
> +
> XLogFileNameP(curFileTLI, readSegNo),
> + readOff)));
>
> goto next_record_is_invalid;
> }
>
> For the above 2 changed error messages, 'log segment' is used for
> filename.
> However all similar changes has 'log file' for filename. There are some
> places
> where 'log segment' is used and other places it is 'log file'.
> So is there any particular reason for it?

Not really. There are several messages that use "log file %s", and also
several places that use "log segment %s" Should we make it consistent
and use either "log segment" or "log file" everywhere?

> 4. @@ -533,33 +533,17 @@ pg_xlog_location_diff(PG_FUNCTION_ARGS)
> - /*
> - * Sanity check
> - */
> - if (loc1.xrecoff> XLogFileSize)
> - ereport(ERROR,
> - (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> - errmsg("xrecoff \"%X\" is out of valid
> range, 0..%X", loc1.xrecoff, XLogFileSize)));
> - if (loc2.xrecoff> XLogFileSize)
> - ereport(ERROR,
> - (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> - errmsg("xrecoff \"%X\" is out of valid
> range, 0..%X", loc2.xrecoff, XLogFileSize)));
> + bytes1 = (((uint64)loc1.xlogid)<< 32L) + loc1.xrecoff;
> + bytes2 = (((uint64)loc2.xlogid)<< 32L) + loc2.xrecoff;
>
> Is there no chance that it can be out of valid range after new changes,
> just a doubt?

No. Not in the sense it used to be, anyway, the XLogFileSize check is no
longer relevant. Perhaps we should check for InvalidXLogRecPtr or that
the pointer doesn't point e.g in the middle of a page header. But then
again, this calculation works fine with both of those cases, so I see no
reason to make it stricter.

> 5.
> --- a/src/backend/replication/walreceiver.c
> +++ b/src/backend/replication/walreceiver.c
> @@ -69,11 +69,12 @@ walrcv_disconnect_type walrcv_disconnect = NULL;
>
> /*
> * These variables are used similarly to openLogFile/Id/Seg/Off,
> - * but for walreceiver to write the XLOG.
> + * but for walreceiver to write the XLOG. recvFileTLI is the TimeLineID
>
> In the above comments, there is still reference to Id/Seg/Off.

Thanks, fixed.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com