Re: WAL format and API changes (9.5)

On 11/05/2014 09:06 AM, Amit Kapila wrote:
> 1.
> +XLogRecPtr
> +XLogInsert(RmgrId rmid, uint8 info, XLogRecData *rdata)
> {
> ..
> + /* info's high bits are reserved for use by me */
> + if (info & XLR_INFO_MASK)
> + elog(PANIC, "invalid xlog info mask %02X", info);
> ..
> }
>
> Earlier before this check, we use to check XLogInsertAllowed()
> which is moved to XLogInsertRecord(), isn't it better to keep
> the check in beginning of the function XLogInsert()?

Doesn't matter much, but I think it's cleanest the way I had it in the
latest patch. XLogInsert() is responsible for building a valid WAL
record to insert, and XLogInsertRecord() handles all the locking, buffer
management etc. to actually insert it. The high-bit check quoted above
is more related to building a valid record, so it belongs in
XLogInsert(), while the XLogInsertAllowed() check is more related to the
actual insertion of the record, so it belongs in XLogInsertRecord().

> 2.
> XLogRecPtr
> XLogInsertRecord(XLogRecData *rdata, XLogRecPtr fpw_lsn)
> {
> ..
> if (fpw_lsn != InvalidXLogRecPtr && fpw_lsn <= RedoRecPtr && doPageWrites)
> {
> /*
> * Oops, some buffer now needs to be backed up that the caller
> * didn't back up. Start over.
> */
> WALInsertLockRelease();
> END_CRIT_SECTION();
> return InvalidXLogRecPtr;
> }
> ..
> }
>
> IIUC, there can be 4 states for doPageWrites w.r.t when record is getting
> assembled (XLogRecordAssemble) and just before actual insert (
> XLogInsertRecord)
>
> I think the behaviour for the state when doPageWrites is true during
> XLogRecordAssemble and false during XLogInsertRecord (just before
> actual insert) is different as compare to HEAD.
>
> In the patch, it will not retry if doPageWrites is false when we are
> about to insert even though fpw_lsn <= RedoRecPtr whereas in HEAD it
> will detect this during assembly of record and retry, isn't this a
> problem?

So the scenario is that:

* XLogRecordAssemble decides that a page doesn't need to be backed up
* both RedoRecPtr and doPageWrites change while building the record.
doPageWrites goes from true to false.

Without the patch, we would retry, because first check RedoRecPtr has
changed. With the patch, we notice that even though RedoRecPtr has
changed, doPageWrites is now off, so no FPWs are required regardless of
RedoRecPtr, and not retry. Yeah, you're right, the behavior changes in
that case. However, the new behavior is correct; the retry is
unnecessary in that scenario.

> 3. There are couple of places where *XLogInsert* is used in wal.sgml
> and it seems to me some of those needs change w.r.t this patch.

Hmm. It's a bit strange to mention XLogInsert and XLogFlush functions by
name in that text. It's otherwise admin-level stuff, on how to set
WAL-related settings. Up to 9.2 that manual page actually called them
"LogInsert" and "LogFlush". But I guess you're right; if we are to
mention the function by name, it should say XLogInsertRecord.

- Heikki