Re: WAL format and API changes (9.5)

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?

> - Would it be a win to add an assertion check for (CritSectionCount == 0)
> in XLogEnsureRecordSpace?

Maybe; added.

> - There is no mention of REGBUF_NO_IMAGE in transam/README.

Fixed

> - This patch adds a lot of blocks like that in the redo code:
> + if (BufferIsValid(buffer))
> +                       UnlockReleaseBuffer(buffer);
> What do you think about adding a new macro like UnlockBufferIfValid(buffer)?

I don't think such a macro would be an improvement in readability, TBH.

> - 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.

> - There is still a TODO item in ValidXLogRecordHeader to check if block
> data header is valid or not. Just mentioning.

Removed.

Previously, we ValidXLogRecordHeader checked that the xl_tot_len of a 
record doesn't exceed the size of header + xl_len + (space needed for 
max number of bkp blocks). But the new record format has no limit on the 
amount of data registered with a buffer, so such a test is not possible 
anymore. I added the TODO item there to remind me that I need to check 
if we need to do something else there instead, but I think it's fine as 
it is. We still sanity-check the block data in ValidXLogRecord; the 
ValidXLogRecordHeader() check happens before we have read the whole 
record header in memory.

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

> - 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.

Here's a new patch with those little things fixed.

- Heikki