Thread: patch-3 (3-allow-wal-record-header-to-be-split.patch)WAL Format Changes

<div style="direction: ltr;font-family: Tahoma;color: #000000;font-size: 10pt;"><p>While reading patch-3
(3-allow-wal-record-header-to-be-split.patch)of<br /> WAL Format<br /> Changes(<a
href="http://archives.postgresql.org/message-id/4FDA5136.6080206@enterprisedb.com">http://archives.postgresql.org/message-id/4FDA5136.6080206@enterprisedb.com</a>),
Ihad few observations which are summarized below:<br /><br /> 1.<br /> ReadRecord(XLogRecPtr *RecPtr, int emode, bool
fetching_ckpt)<br/> + /*<br /> +  * If we got the whole header already, validate it immediately. Otherwise<br /> +  *
wevalidate it after reading the rest of the header from the next page.<br /> +  */<br /> + if (targetRecOff <=
XLOG_BLCKSZ- SizeOfXLogRecord)<br /> + {<br /> +  if (!ValidXLogRecordHeader(RecPtr, record, emode, randAccess))<br />
+   gotonext_record_is_invalid;<br /> +  gotheader = true;<br /> + }<br /> + else<br /> +  gotheader = false;<br />
+<p>Shouldn'tthe record header validation be done before the check for allocating a bigger record buffer based <br />
ontotal length. Otherwise it may lead to allocation of bigger buffer which may not be required if record header is
invalid.<br/> In cases where record header is not split, validation can be done before otherwise it can be done
later.<p> <p><br/> 3. General observation, not related to your changes <br /> XLogInsert(RmgrId rmid, uint8 info,
XLogRecData*rdata) <br /> . <br /> . <br />         if (freespace == 0) <br />          { <br />                 
updrqst= AdvanceXLInsertBuffer(false); <p><br /> In the code, AdvanceXLInsertBuffer(false); is called after starting
criticalsection and acquiring <br /> WALInsertLock, now if any error occurs inside  AdvanceXLInsertBuffer() <br /> (in
oneof the paths it calls XLogWrite(), from which it can call XLogFileInit() where error can occur); <br /> how will it
releaseWALInsertLock or end critical section. <p> <p> <p>With Regards,<p>Amit Kapila.</div> 

On 30.06.2012 10:11, Amit kapila wrote:
> ReadRecord(XLogRecPtr *RecPtr, int emode, bool fetching_ckpt)
> + /*
> +  * If we got the whole header already, validate it immediately. Otherwise
> +  * we validate it after reading the rest of the header from the next page.
> +  */
> + if (targetRecOff<= XLOG_BLCKSZ - SizeOfXLogRecord)
> + {
> +  if (!ValidXLogRecordHeader(RecPtr, record, emode, randAccess))
> +   goto next_record_is_invalid;
> +  gotheader = true;
> + }
> + else
> +  gotheader = false;
> +
>
> Shouldn't the record header validation be done before the check for allocating a bigger record buffer based
> on total length. Otherwise it may lead to allocation of bigger buffer which may not be required if record header is
invalid.

Hmm, doing an unnecessary memory allocation just before giving up isn't 
really a problem. And we treat out-of-memory the same as a corrupt 
record, so this isn't a correctness issue. But I agree it would still be 
better to change the order, if only because you're more likely to get a 
better error message than "out of memory".

> In cases where record header is not split, validation can be done before otherwise it can be done later.

Committed that way. We could also delay enlarging the buffer until after 
we read the next page and get the whole header, but it's probably fine 
as it is.

> 3. General observation, not related to your changes
> XLogInsert(RmgrId rmid, uint8 info, XLogRecData *rdata)
> .
> .
>          if (freespace == 0)
>           {
>                   updrqst = AdvanceXLInsertBuffer(false);
>
> In the code, AdvanceXLInsertBuffer(false); is called after starting critical section and acquiring
> WALInsertLock, now if any error occurs inside  AdvanceXLInsertBuffer()
> (in one of the paths it calls XLogWrite(), from which it can call XLogFileInit() where error can occur);
> how will it release WALInsertLock or end critical section.

Yep, if an I/O error happens while writing a WAL record - running out of 
disk space is the typical example - we PANIC. Nope, it's not ideal.

Even if we somehow managed to avoid doing I/O in the critical section in 
XLogInsert(), most callers of XLogInsert() surround the call in a 
critical section anyway. For example, when a new tuple is inserted to 
heap, once you've modified the page to add the new tuple, it's too late 
to back out. The WAL record is written while holding the lock on the 
page, and if something goes wrong with writing the WAL record, we have 
no choice but PANIC.

It would be nice to avoid that, at least for the common 
out-of-disk-space case. Perhaps we could somehow pre-reserve some WAL 
space before starting to modify the page. But that's a pretty big project..

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

From: Heikki Linnakangas [heikki.linnakangas@enterprisedb.com]
Sent: Sunday, July 01, 2012 1:54 AM
On 30.06.2012 10:11, Amit kapila wrote:
>> 3. General observation, not related to your changes
>> XLogInsert(RmgrId rmid, uint8 info, XLogRecData *rdata)
>>.
>>.
>>         if (freespace == 0)
>>           {
>>                   updrqst = AdvanceXLInsertBuffer(false);
>>
>> In the code, AdvanceXLInsertBuffer(false); is called after starting critical section and acquiring
>> WALInsertLock, now if any error occurs inside  AdvanceXLInsertBuffer()
>> (in one of the paths it calls XLogWrite(), from which it can call XLogFileInit() where error can occur);
>> how will it release WALInsertLock or end critical section.

> Yep, if an I/O error happens while writing a WAL record - running out of
> disk space is the typical example - we PANIC. Nope, it's not ideal.

> Even if we somehow managed to avoid doing I/O in the critical section in
> XLogInsert(), most callers of XLogInsert() surround the call in a
> critical section anyway. For example, when a new tuple is inserted to
> heap, once you've modified the page to add the new tuple, it's too late
> to back out. The WAL record is written while holding the lock on the
> page, and if something goes wrong with writing the WAL record, we have
> no choice but PANIC.

PANIC is understandable as after this user cannot perform operation without restart.
However if the level is ERROR, then there might be other problems as user can perform operations after that through
samesession  
The case which I am highlighting is of ERROR, please refer the code of
XLogFileInit().

For Example:
fd = BasicOpenFile(path, O_RDWR | PG_BINARY | get_sync_bit(sync_method),        S_IRUSR | S_IWUSR); if (fd < 0) {  if
(errno!= ENOENT)   ereport(ERROR,     (errcode_for_file_access(),      errmsg("could not open file \"%s\" (log file %u,
segment%u): %m",       path, log, seg))); }