Re: WAL format and API changes (9.5) - Mailing list pgsql-hackers

On Sat, Aug 2, 2014 at 1:40 AM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
> I ran this through my WAL page comparison tool to verify that all the WAL
> record types are replayed correctly (although I did some small cleanup after
> that, so it's not impossible that I broke it again; will re-test before
> committing).
I have run as well some tests on it with a master a a standby to check WAL construction and replay:
- regression tests as well as tests from contrib, isolation
- WAL page comparison tool
- Some tests with pgbench
- 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.

Then, I had a look at the code, and here are some comments:
- This comment has been introduced with 96ef3b8 that has added page checksums. Perhaps the authors can tell what was the original purpose of this comment (Jeff, Simon). For now, it may be better to remove this FIXME and to let this comment as is.
+                * FIXME: I didn't understand below comment. Is it still relevant?
+                *
                 * This also means there is no corresponding API call for this, so an
                 * smgr implementation has no need to implement anything. Which means
                 * nothing is needed in md.c etc

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

- 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);

- There is a typo in the header of xlogrecord.h, the "to" is not needed:
+ * Definitions for to the WAL record format.                                                                                                                                        

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

- Just came across that: s/reference refes to/reference refers to

- XLogRecGetBlockRefIds needing an already-allocated array for *out is not user-friendly. Cannot we just do all the work inside this function?

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

The patch is more solid and better structured than the previous versions. It is in good shape.

Regards,

--
Michael