Re: Patch-2 (2-move-continuation-record-to-page-header.patch) WAL Format Changes

On 28.06.2012 17:40, Amit Kapila wrote:
> 1.
> @@ -693,7 +693,6 @@ XLogInsert(RmgrId rmid, uint8 info, XLogRecData *rdata)
> {
> XLogCtlInsert *Insert =&XLogCtl->Insert;
> XLogRecord *record;
> - XLogContRecord *contrecord;
> XLogRecPtr RecPtr;
> XLogRecPtr WriteRqst;
> uint32 freespace;
> @@ -1082,9 +1081,7 @@ begin:;
> curridx = Insert->curridx;
> /* Insert cont-record header */
> Insert->currpage->xlp_info |= XLP_FIRST_IS_CONTRECORD;
> - contrecord = (XLogContRecord *) Insert->currpos;
> - contrecord->xl_rem_len = write_len;
> - Insert->currpos += SizeOfXLogContRecord;
> + Insert->currpage->xlp_rem_len = write_len;
>
> After above code changes the comment "/* Insert cont-record header */"
> should be changed.

Thanks, fixed.

> 2.
> Is XLP_FIRST_IS_CONTRECORD required after putting xl_rem_len in page header;
>
> Can't we do handling based on xl_rem_len?

Hmm, yeah, it's redundant now, we could use xl_rem_len > 0 to indicate a
continued record. I thought I'd keep the flag to avoid unnecessary
changes, to make life a bit easier for 3rd party tools that read the
WAL, but I don't know if it really makes any difference. There is no
shortage of xlog page header flag bits, so there's no hurry to get rid
of it.

> Sorry for sending the observations in pieces rather than all-together, as I
> am not sure how much I will be able to complete.
>
> So what ever I am able to read, I am sending you my doubts or observations.

Thanks for the review, much appreciated!

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