Re: WAL format and API changes (9.5) - Mailing list pgsql-hackers

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