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

4) Current patch crashes when running make check in contrib/test_decoding. Looking closer, wal_level = logical is broken:
=# show wal_level;
 wal_level
-----------
 logical
(1 row)
=# create database foo;
The connection to the server was lost. Attempting reset: Failed.

Looking even closer, log_heap_new_cid is broken:
(lldb) bt
* thread #1: tid = 0x0000, 0x00007fff8a3c2866 libsystem_kernel.dylib`__pthread_kill + 10, stop reason = signal SIGSTOP
  * frame #0: 0x00007fff8a3c2866 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff8d37235c libsystem_pthread.dylib`pthread_kill + 92
    frame #2: 0x00007fff9369db1a libsystem_c.dylib`abort + 125
    frame #3: 0x0000000104428819 postgres`ExceptionalCondition(conditionName=0x00000001044a711c, errorType=0x000000010448b19f, fileName=0x00000001044a2cfd, lineNumber=6879) + 137 at assert.c:54
    frame #4: 0x0000000103f08dbe postgres`log_heap_new_cid(relation=0x00007fae4482afb8, tup=0x00007fae438062e8) + 126 at heapam.c:6879
    frame #5: 0x0000000103f080cf postgres`heap_insert(relation=0x00007fae4482afb8, tup=0x00007fae438062e8, cid=0, options=0, bistate=0x0000000000000000) + 383 at heapam.c:2095
    frame #6: 0x0000000103f09b6a postgres`simple_heap_insert(relation=0x00007fae4482afb8, tup=0x00007fae438062e8) + 74 at heapam.c:2533
    frame #7: 0x000000010406aacf postgres`createdb(stmt=0x00007fae44843690) + 6335 at dbcommands.c:511
    frame #8: 0x000000010429def8 postgres`standard_ProcessUtility(parsetree=0x00007fae44843690, queryString=0x00007fae44842c38, context=PROCESS_UTILITY_TOPLEVEL, params=0x0000000000000000, dest=0x00007fae44843a18, completionTag=0x00007fff5bd4ee30) + 1720 at utility.c:56

5) Yes,there are some WAL records that have only data related to buffers, XLOG_GIN_VACUUM_DATA_LEAF_PAGE being one, with nothing registered with buffer_id = -d, but using a dummy entry seems counter-productive:
+       rdata = rdata_head;
+       if (rdata == NULL)
+       {
+               /*
+                * the rest of this function assumes that there is at least some
+                * rdata entries, so fake an empty rdata entry
+                */
+               dummy_rdata.data = NULL;
+               dummy_rdata.len = 0;
+               dummy_rdata.next = NULL;
+               rdata = &dummy_rdata;
+       }

Could this be removed and replaced by a couple of checks on rdata being NULL in some cases? XLogInsert should be updated in consequence.

6) XLogRegisterData creates a XLogRecData entry and appends it in one of the static XLogRecData entries if buffer_id >= 0 or to rdata_head/rdata_tail if buffer_id = -1 (for permanent data related to the WAL record). XLogRegisterXLogRecData does something similar, but with an entry of XLogRecData already built. What do you think about removing XLogRegisterXLogRecData and keeping only XLogRegisterData. This makes the interface simpler, and XLogRecData could be kept private in xlog.c. This would need more work especially on gin side where I am seeing for example constructLeafRecompressWALData directly building a XLogRecData entry.

By doing so, we could as remove the while loop at the bottom of XLogRegisterXLogRecData as we would insert in the tail only the latest record created, aka replacing that:
while ((*tail)->next)
        *tail = (*tail)->next;

By simply that:
*tail = (*tail)->next;

pg_xlogdump is a good example of what can be done as well.
9) In ginxlog.c, shouldn't this block of code use XLogGetPayload?
+               char       *payload = XLogRecGetData(record) + sizeof(ginxlogInsert);
+#ifdef NOT_USED
                leftChildBlkno = BlockIdGetBlockNumber((BlockId) payload);
+#endif

10) Shouldn't the call to XLogBeginInsert come first here?

@@ -1646,7 +1647,16 @@ spgAddNodeAction(Relation index, SpGistState *state,
                {
                        XLogRecPtr      recptr;

-                       recptr = XLogInsert(RM_SPGIST_ID, XLOG_SPGIST_ADD_NODE, rdata);
+                       XLogRegisterBuffer(0, saveCurrent.buffer, REGBUF_STANDARD);     /* orig page */
+                       XLogRegisterBuffer(1, current->buffer, REGBUF_STANDARD);                /* new page */
+                       if (xlrec.parentBlk == 2)
+                               XLogRegisterBuffer(2, parent->buffer, REGBUF_STANDARD);
+
+                       XLogBeginInsert();
+                       XLogRegisterData(-1, (char *) &xlrec, sizeof(xlrec));
+                       XLogRegisterData(-1, (char *) newInnerTuple, newInnerTuple->size);
+
+                       recptr = XLogInsert(RM_SPGIST_ID, XLOG_SPGIST_ADD_NODE);

11) Any reason for raising a PANIC here?

@@ -1126,7 +1126,7 @@ LWLockRelease(LWLock *l)
                        break;
        }
        if (i < 0)
-               elog(ERROR, "lock %s %d is not held", T_NAME(l), T_ID(l));
+               elog(PANIC, "lock %s %d is not held", T_NAME(l), T_ID(l));
        num_held_lwlocks--;
        for (; i < num_held_lwlocks; i++)

14) SPgist redo is crashing (during make installcheck run on a master, redo done on a standby at recovery).
(lldb) bt
* thread #1: tid = 0x0000, 0x00007fff8a3c2866 libsystem_kernel.dylib`__pthread_kill + 10, stop reason = signal SIGSTOP
  * frame #0: 0x00007fff8a3c2866 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff8d37235c libsystem_pthread.dylib`pthread_kill + 92
    frame #2: 0x00007fff9369db1a libsystem_c.dylib`abort + 125
    frame #3: 0x000000010ba8d819 postgres`ExceptionalCondition(conditionName=0x000000010bb21ea9, errorType=0x000000010baf019f, fileName=0x000000010bb212dd, lineNumber=65) + 137 at assert.c:54
    frame #4: 0x000000010b5c3a50 postgres`addOrReplaceTuple(page=0x000000010bfee980, tuple=0x00007ffd41822902, size=770059, offset=18) + 688 at spgxlog.c:65
    frame #5: 0x000000010b5c031f postgres`spgRedoMoveLeafs(lsn=71708608, record=0x00007ffd41822800) + 719 at spgxlog.c:263
    frame #6: 0x000000010b5bf492 postgres`spg_redo(lsn=71708608, record=0x00007ffd41822800) + 402 at spgxlog.c:988
    frame #7: 0x000000010b5e774a postgres`StartupXLOG + 8474 at xlog.c:6780
    frame #8: 0x000000010b86a5fe postgres`StartupProcessMain + 430 at startup.c:224
    frame #9: 0x000000010b600409 postgres`AuxiliaryProcessMain(argc=2, argv=0x00007fff546ea190) + 1897 at bootstrap.c:416
    frame #10: 0x000000010b864998 postgres`StartChildProcess(type=StartupProcess) + 328 at postmaster.c:5090
    frame #11: 0x000000010b862aa9 postgres`PostmasterMain(argc=3, argv=0x00007ffd40c04470) + 5401 at postmaster.c:1212
    frame #12: 0x000000010b7a9335 postgres`main(argc=3, argv=0x00007ffd40c04470) + 773 at main.c:227

Also, I wanted to run the WAL replay facility to compare before and after buffer images, but code crashed before... I am still planning to do so once this code gets more stable though.

Regards,

--
Michael