Re: WAL format and API changes (9.5)

On Tue, Aug 12, 2014 at 6:44 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> On 08/05/2014 03:50 PM, Michael Paquier wrote:
>>
>> - When testing pg_xlogdump I found an assertion failure for record
>> XLOG_GIN_INSERT:
>> Assertion failed: (((bool) (((const void*)(&insertData->newitem.key) !=
>> ((void*)0)) && ((&insertData->newitem.key)->ip_posid != 0)))), function
>> gin_desc, file gindesc.c, line 127.
>
>
> I could not reproduce this. Do you have a test case?
Indeed. I am not seeing anything now for this record. Perhaps I was
using an incorrect version of pg_xlogdump.

>> - Don't we need a call to XLogBeginInsert() in XLogSaveBufferForHint():
>> + int flags = REGBUF_FORCE_IMAGE;
>> + if (buffer_std)
>> + flags |= REGBUF_STANDARD;
>> + XLogRegisterBuffer(0, buffer, flags);
>> [...]
>> - recptr = XLogInsert(RM_XLOG_ID, XLOG_FPI, rdata);
>> + recptr = XLogInsert(RM_XLOG_ID, XLOG_FPI);
>
>
> Indeed, fixed. With that, "initdb -k" works, I had not tested the patch with
> page checksums before.
Thanks for the fix. Yes now this works.

>> - XLogRecGetBlockRefIds needing an already-allocated array for *out is not
>> user-friendly. Cannot we just do all the work inside this function?
>
>
> I agree it's not a nice API. Returning a palloc'd array would be nicer, but
> this needs to work outside the backend too (e.g. pg_xlogdump). It could
> return a malloc'd array in frontend code, but it's not as nice. On the
> whole, do you think that be better than the current approach?

A palloc'd array returned is nicer for the user. And I would rather
rewrite the API like that, having it return the array instead:
int *XLogRecGetBlockRefIds(XLogRecord *record, int *num_array)
Alvaro has also outlined that in the FRONTEND context pfree and palloc
would work as pg_free and pg_malloc (didn't recall it before he
mentioned it btw).

>> - All the functions in xlogconstruct.c could be defined in a separate
>> header xlogconstruct.h. What they do is rather independent from xlog.h.
>> The
>> same applies to all the structures and functions of xlogconstruct.c in
>> xlog_internal.h: XLogRecordAssemble, XLogRecordAssemble,
>> InitXLogRecordConstruct. (worth noticing as well that the only reason
>> XLogRecData is defined externally of xlogconstruct.c is that it is being
>> used in XLogInsert()).
>
>
> Hmm. I left the defines for xlogconstruct.c functions that are used to build
> a WAL record in xlog.h, so that it's not necessary to include both xlog.h
> and xlogconstruct.h in all files that write a WAL record (XLogInsert() is
> defined in xlog.h).
>
> But perhaps it would be better to move the prototype for XLogInsert to
> xlogconstruct.h too? That might be a better division; everything needed to
> insert a WAL record would be in xlogconstruct.h, and xlog.h would left for
> more internal functions related to WAL. And perhaps rename the files to
> xloginsert.c and xloginsert.h.

Yes indeed. As XLogBeginInsert() is already part of xlogconstruct.c,
it is not weird. This approach has the merit to make XLogRecData
completely bundled with the insertion and construction of the WAL
records. Then for the name xloginsert.c is fine for me too.

Among the things changed since v5, v6 is introducing a safety limit to
the maximum number of backup blocks within a single WAL record:
#define XLR_MAX_BKP_BLOCKS 256
XLogRecordBlockData is updated to use uint8 instead of char to
identify the block id, and XLogEnsureRecordSpace fails if more than
XLR_MAX_BKP_BLOCKS are wanted by the caller. I am fine with this
change, just mentioning it.
Regards,
--
Michael