On Wed, Nov 5, 2014 at 2:56 PM, Heikki Linnakangas <
hlinnakangas@vmware.com> wrote:
> On 11/05/2014 09:06 AM, Amit Kapila wrote:
>>
>> 2.
>> XLogRecPtr
>> XLogInsertRecord(XLogRecData *rdata, XLogRecPtr fpw_lsn)
>
> 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.
>
How does it interact with backup, basically in stop backup we first
change forcePageWrite to false and then get the stop wal location
by inserting XLOG_BACKUP_END, so it seems to me that it is quite
possible that the record which backend is inserting using XLogInsert()
will be considered in backup. Now shouldn't this record contain FPW
if forcePageWrite was true when XLogInsert() started to avoid any torn
page taken in backup?
>> 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.
>
Agreed.