Re: WAL format and API changes (9.5)

On Tue, Nov 4, 2014 at 10:03 PM, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>
wrote:
>
> On 10/30/2014 06:02 PM, Andres Freund wrote:
>>
>> On 2014-10-29 10:24:20 +0200, Heikki Linnakangas wrote:
>>>
>>> On 10/06/2014 02:29 PM, Andres Freund wrote:
>>>>
>>>> On 2014-10-06 14:19:39 +0300, Heikki Linnakangas wrote:
>>>>>
>>>>> Barring objections, I'll commit this, and then continue benchmarking
the
>>>>> second patch with the WAL format and API changes.
>>>>
>>>>
>>>> I'd like to have a look at it beforehand.
>>>
>>>
>>> Ping? Here's an rebased patch. I'd like to proceed with this.
>>
>>
>> Doing so.
>
>
> Here's a new version of this refactoring patch. It fixes all the concrete
issues you pointed out.
>
>>>> I've not yet really looked,
>>>> but on a quick readthrough XLogInsertRecData() staying in xlog.c
doesn't
>>>> make me happy...
>>>
>>>
>>> Ok.. Can you elaborate?
>>
>>
>> To me the split between xloginsert.c doing some of the record assembly,
>> and xlog.c doing the lower level part of the assembly is just wrong.
>
>
> I moved the checks for bootstrap mode and xl_len == 0, from the current
XLogInsert to the new XLogInsert() function. I don't imagine that to be
enough address your feelings about the split between xlog.c and
xloginsert.c, but makes it a tiny bit clearer IMHO. I don't know what else
to do about it, as it feels very sensible to me as it is now. So unless you
object more loudly, I'm going to just go ahead with this and commit,
probably tomorrow.
>

Few observations while reading the latest patch:

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()?

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?

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.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com