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

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