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
wal-format-and-api-changes-6.patch.gz
Description: application/gzip
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
