Cleaned up obsolete @todo's; fixed inconsistent page file initialization

lladd/logger/logEntry.h

@@ -52,8 +52,6 @@ BEGIN_C_DECLS
 
    Structs and memory managment routines for log entries
 
-   @todo Is there a better way to deal with sizeof() and log entries?
-   
    @todo Other than some typedefs, is there anything in logEntry that belongs in the API?
 
    @ingroup LLADD_CORE

lladd/operations/alloc.h

@@ -3,9 +3,6 @@
 
    Allocates and deallocates records.
 
-   @todo Talloc() should reuse space freed by Tdealloc(), but
-   currently just leaks it.
-
    @ingroup OPERATIONS
 
    $Id$
@@ -32,8 +29,6 @@ void TallocInit();
     @param size The size of the new record to be allocated.  Talloc will allocate a
     blob if the record will not easily fit on a page.
 
-    @todo need to obtain (transaction-level) write locks _before_ writing log entries.  Otherwise, we can deadlock at recovery.
-
     @return the recordid of the new record.
 */
 compensated_function recordid Talloc(int xid, unsigned long size);
@@ -42,7 +37,6 @@ compensated_function recordid TallocFromPage(int xid, long page, unsigned long s
 
 /** 
    Free a record.  
-    @todo Currently, we just leak store space on dealloc. 
 */
 compensated_function void Tdealloc(int xid, recordid rid);
 

lladd/operations/prepare.h

@@ -64,8 +64,6 @@ terms specified in this license.
  (to support savepoints, for example) Right now, recovery uses a
  guarded iterator, transUndo() does not.
  
- @todo Test Tprepare()
-
  *
  * @ingroup OPERATIONS
  *

lladd/pageCache.h

@@ -11,18 +11,16 @@
     pageRead() function.  Eventually, this could be extended to
     support application specific caching schemes.
 
-    Currently, LLADD uses LRU-2S from Markatos "On Caching Searching
-    Engine Results"
-
     If you would like to implement your own caching policy, implement
     the functions below.  They are relatively straightforward.
     Note that pageCache does not perform any file I/O of its own.
 
     The implementation of this module does not need to be threadsafe.
 
-    @todo pageCache should not include page.h at all.  It should treat
+    @param first The caller should manually read this page by calling
-    pages as (int, void*) pairs.  (But the page struct contains the
+    pageRead() before calling pageCacheInit. 
-    pointers that pageCache manipulates..)
+
+    @todo pageCacheInit should not take a page as a parameter.
 
 */
 

lladd/pageHandle.h

@@ -34,8 +34,8 @@ extern int pageFile_isDurable;
    
    @see bufferManager.c for the implementation of pageRead.
 
-   @todo pageRead and pageWrite should be static, but pageCache needs
+   @todo pageRead and pageWrite should be stored in a struct returned by
-   to call them.
+   an initailizer, not in global function pointers.
 */
 extern void (*pageRead)(Page * ret);
 /**

lladd/transactional.h

@@ -51,7 +51,7 @@ terms specified in this license.
  * In theory, the other .h files that are installed in /usr/include
  * aren't needed for application developers.
  *
- * @todo Move as much of the stuff in lladd/ to src/lladd/ as possible.
+ * @todo Move as much of the stuff in lladd/ to src/lladd/ as possible.  Alternatively, move all headers to lladd/, and be done with it!
  *
  */
 /**

src/lladd/blobManager.h

@@ -38,7 +38,6 @@ BEGIN_C_DECLS
            physical logging (this is automatic, since we use Tset()
            calls to update the records.)
 
-       @todo Should the tripleHash be its own little support library? 
        @todo Set range??
 
        @ingroup LLADD_CORE
@@ -61,11 +60,6 @@ compensated_function recordid preAllocBlobFromPage(int xid, long page, long blob
 
 /**
    Allocate a blob of size blobSize. 
-
-   @todo This function does not atomically allocate space in the blob
-   file.  Instead of trusting the blob store length, upon recieving a
-   log entry, update a static file length variable in blobManager.
-
 */
 
 void allocBlob(int xid, recordid rid);

src/lladd/bufferManager.c

@@ -71,8 +71,7 @@ terms specified in this license.
 
 #include <lladd/lockManager.h>
 #include <lladd/pageCache.h>
-#include "pageFile.h"
+#include <lladd/pageHandle.h>
-
 #include <lladd/truncation.h>
 
 #include <lladd/lhtable.h>
@@ -135,9 +134,6 @@ static int bufManBufInit() {
 
         bufferPoolInit();
 
-	openPageFile();
-
-
 	pthread_mutex_init(&loadPagePtr_mutex, NULL);
 
 	activePages = LH_ENTRY(create)(16);
@@ -148,7 +144,7 @@ static int bufManBufInit() {
 	first = pageMalloc();
 	pageFree(first, 0);
 	LH_ENTRY(insert)(activePages, &first->id, sizeof(int), first);
-
+        pageRead(first);
 	pageCacheInit(first);
 
 	int err = pthread_key_create(&lastPage, 0);
@@ -180,7 +176,8 @@ static void bufManBufDeinit() {
 	pthread_mutex_destroy(&loadPagePtr_mutex);
 	
 	pageCacheDeinit();
-	closePageFile();
+
+	//closePageFile();
 
 	bufferPoolDeInit();
 	
@@ -194,6 +191,8 @@ static void bufManBufDeinit() {
 /**
     Just close file descriptors, don't do any other clean up. (For
     testing.)
+
+    @todo buffer manager should never call closePageFile(); it not longer manages pageFile handles
 */
 void bufManSimulateBufferManagerCrash() {
   closePageFile();

src/lladd/bufferManager/bufferHash.c

@@ -7,7 +7,7 @@
 
 
 #include <lladd/bufferPool.h>
-#include "pageFile.h"
+#include <lladd/pageHandle.h>
 
 #include <lladd/replacementPolicy.h>
 #include <lladd/bufferManager.h>

src/lladd/bufferPool.c

@@ -126,7 +126,6 @@ static void pageFreeNoLock(Page *p, int id) {
   p->dirty = 0;
 }
 
-/** @todo Does pageFree really need to obtain a lock? */
 void pageFree(Page *p, int id) {
   writelock(p->rwlatch, 10);
   pageFreeNoLock(p,id);

src/lladd/latches.h

@@ -1,13 +1,6 @@
 #include <config.h>
 #include <lladd/common.h>
-
-/** @todo threading should be moved into its own header file. */
 #include <pthread.h>
-
-
-
-
-/*#include <pbl/pbl.h> -- Don't want everything that touches threading to include pbl... */
 #include <lladd/stats.h>
 
 #ifndef __LATCHES_H

src/lladd/lhtable.c

@@ -6,10 +6,7 @@
 #include <string.h>
 #include <stdio.h>
 #include "latches.h"
-/**
+
-   @todo Look up the balls + bins stuff, and pick FILL_FACTOR in a
-   principled way...
-*/
 #define FILL_FACTOR (0.5)
 
 //#define MEASURE_GLOBAL_BUCKET_LENGTH

src/lladd/logger/logHandle.c

@@ -110,15 +110,7 @@ const LogEntry * previousInTransaction(LogHandle * h) {
 
 }
 
-/**
-   @todo The next_offset field is set in a way that assumes a
-   particular layout of log entries.  If we want to support other
-   loggers, then the lsn of the next entry should be calculated by the
-   logging implementation, not here.  (One possibility is to have
-   readLSNEntry return it explicitly.)
-*/
 static void set_offsets(LogHandle * h, const LogEntry * e) {
   h->next_offset = LogNextEntry(e);
   h->prev_offset = e->prevLSN;
 }
-

src/lladd/logger/logWriter.h

@@ -153,8 +153,8 @@ void closeLogWriter();
   Only use after calling closeLogStream AND you are sure there are no active (or
   future active) transactions!
 
-  @todo This is in here now for completeness, but once we implement
+  @todo This only exists because the tests use it...once the logfile
-  log truncation, it should leave.
+        name isn't hardcoded, remove this function.
 */
 void deleteLogWriter();
 
@@ -175,4 +175,3 @@ long sizeofInternalLogEntry_LogWriter(const LogEntry * e);
 END_C_DECLS
 
 #endif /* __LLADD_LOGGER_LOGWRITER_H */
-

src/lladd/logger/logger2.c

@@ -44,7 +44,7 @@ terms specified in this license.
    @file Abstract log implementation.  Provides access to methods that
    directly read and write log entries, force the log to disk, etc.
 
-   @todo Switch logger2 to use function pointers?
+   @todo Switch logger2 to use function pointers
 */
 
 #include <config.h>

src/lladd/operations/alloc.c

@@ -29,18 +29,16 @@
    too big of a deal, but it should be fixed someday.  A more serious
    problem results from crashes during blob allocation.
 
-   @todo The entire allocaction system needs to be redone.  
+   Here are some requirements for alloc:
 
-   Here are some requirements for the next version of alloc:
+   [DONE] Space Reuse: There are many ways to implement this.  One method
-
-   Space Reuse: There are many ways to implement this.  One method
    (that I'm not particularly attached to) is to maintain seperate
    linked lists for each type of page, seperated by an estimate of the
    amount of space free (actually 'un-reserved'; see below) on the
    page.  Allocation would move pages between linked lists, and search
    in the appropriate linked list before expanding the page file.
     
-   Treserve: Hashtables, linked lists, and other graph-like structures
+   @todo Treserve: Hashtables, linked lists, and other graph-like structures
    can be optimized by exploiting physical locality.  A call such as
    this allows page-level locality to be established / maintained:
 
@@ -52,15 +50,17 @@
    TallocFromPage (xid, page, size) already exists, and should ignore
    the presence of the 'reserved space' field.
 
-   Track level locality is another problem that Talloc should address, 
+   @todo Track level locality is another problem that Talloc should address, 
    especially for the blob implementation.
 
-   Better support for locking.  Consider this sequence of events:
+   [DONE] Concurrent transaction support.  Consider this sequence of events:
 
    recordid rid1 = Talloc (xid1, 1);
    recordid rid2 = Talloc (xid2, 1);  // May deadlock if page level  
                                      // locking is used.
 
+   [NOT TO BE DONE] (We don't want allocation to grab locks...
+
    The lock manager needs a 'try lock' operation that allows
    transactions to attempt to read multiple pages.  When the current
    lock manager returns "LLADD_DEADLOCK", it pretends the lock request
@@ -84,7 +84,6 @@ static int operate(int xid, Page * p, lsn_t lsn, recordid rid, const void * dat)
   return 0;
 }
 
-/** @todo Currently, we leak empty pages on dealloc. */
 static int deoperate(int xid, Page * p, lsn_t lsn, recordid rid, const void * dat) {
   assert(rid.page == p->id);
   slottedDeRalloc(xid, p, lsn, rid);
@@ -99,7 +98,6 @@ static int reoperate(int xid, Page *p, lsn_t lsn, recordid rid, const void * dat
     //  } 
 
   slottedPostRalloc(xid, p, lsn, rid); 
-  /** @todo dat should be the pointer to the space in the blob store. */
   recordWrite(xid, p, lsn, rid, dat);
 
   return 0;

src/lladd/operations/arrayList.c

@@ -195,7 +195,6 @@ compensated_function int TarrayListLength(int xid, recordid rid) {
 
 /*----------------------------------------------------------------------------*/
 
-/** @todo locking for arrayLists */
 recordid dereferenceArrayListRid(Page * p, int offset) {
 
   TarrayListParameters tlp = pageToTLP(p);

src/lladd/operations/linearHash.c

@@ -756,7 +756,7 @@ void ThashInstantInsert(int xid, recordid hashRid,
   free(e);
 
 }
-/** @todo hash hable probably should track the number of items in it,
+/** @todo hash table probably should track the number of items in it,
     so that expand can be selectively called. */
 void ThashInstantDelete(int xid, recordid hashRid, 
 	    const void * key, int keySize, int valSize) {

src/lladd/operations/pageOperations.c

@@ -38,7 +38,7 @@ compensated_function int TpageSet(int xid, int pageid, byte * memAddr) {
   return 0;
 }
 
-/** @todo this should be dynamic. */
+/** @todo region sizes should be dynamic. */
 #define TALLOC_PAGE_REGION_SIZE 128 // 512K
 
 /** 

src/lladd/page.c

@@ -164,8 +164,7 @@ int recordRead(int xid, Page * p, recordid rid, void *buf) {
     readBlob(xid, p, rid, buf);
   } else if(page_type == SLOTTED_PAGE || page_type == BOUNDARY_TAG_PAGE) {
     slottedRead(p, rid, buf);
-    /* FIXED_PAGES can function correctly even if they have not been
+    /* @TODO !page_type can never be required if this code is correct... arraylist is broken!!*/
-       initialized. */
   } else if(page_type == FIXED_PAGE || page_type==ARRAY_LIST_PAGE || !page_type) { 
     fixedRead(p, rid, buf);
   } else {
@@ -228,7 +227,7 @@ int recordType(int xid, Page * p, recordid rid) {
 	unlock(p->rwlatch);
 	return ret;
 }
-/** @todo implement getRecordLength for blobs and fixed length pages. */
+/** @todo implement recordSize for blobs and fixed length pages. */
 int recordSize(int xid, Page * p, recordid rid) {
   readlock(p->rwlatch, 353);
   int ret = recordTypeUnlocked(xid, p, rid);

src/lladd/page.h

@@ -272,11 +272,7 @@ int recordSize(int xid, Page * p, recordid rid);
 */
 int recordTypeUnlocked(int xid, Page * p, recordid rid);
 /**
-   return the length of the record rid.  (the rid parameter's size field will be ignored)
+   @return -1 if the field does not exist, the size of the field otherwise (the rid parameter's size field will be ignored).
-
-   @todo implement getRecordLength for blobs and fixed length pages.
-
-   @return -1 if the field does not exist, the size of the field otherwise.
  */
 int recordLength(int xid, Page * p, recordid rid);
 

src/lladd/page/slotted.c

@@ -290,7 +290,7 @@ recordid slottedRawRalloc(Page * page, int size) {
 }
 
 /** 
-  @todo Allocation is scary without locking.  Consider this situation:
+  Allocation is scary without locking.  Consider this situation:
 
    (1) *numslot_ptr(page) is 10
    (2) An aborting transcation calls really_do_ralloc(page) with rid.slot = 12
@@ -299,6 +299,9 @@ recordid slottedRawRalloc(Page * page, int size) {
    (4) This function adds it to the freelist to avoid leaking space.  (Therefore, Talloc() can return recordids that will
        be reused by aborting transactions...)
 
+  For now, we make sure that we don't alloc off a page that another active
+  transaction dealloced from.
+
   @param rid Recordid with 'internal' size.  The size should have already been translated to a type if necessary.
 */
 static void really_do_ralloc(Page * page, recordid rid) {
@@ -415,6 +418,9 @@ static void really_do_ralloc(Page * page, recordid rid) {
    @param page
    @param lsn
    @param rid with user-visible size.
+
+   @todo Does this still really need to check for BLOB_THRESHOLD_SIZE? 
+   Shouldn't the caller call slottedSetType if necessary?
 */
 recordid slottedPostRalloc(int xid, Page * page, lsn_t lsn, recordid rid) {
   

src/lladd/page/slotted.h

@@ -54,10 +54,9 @@ Slotted page layout:
 
  $Id$
 
-@todo slotted.c shouldn't need to include the buffer manager calls 
-                (like loadPage(), releasePage()).
 @todo slotted.c Should know that specific record types (like blobs) exist,
-                (but should not hardcode information about these types)
+                (but should not hardcode information about these types) This
+                has been handled, except in slottedPostRalloc...
 
 ************************************************************************/
 
@@ -168,7 +167,7 @@ void slottedPageDeInit();
  * @see slottedFreespace()
  *
  * @todo pageRalloc's algorithm for reusing slot id's reclaims the
- * highest numbered slots first, which encourages fragmentation.
+ * most recently freed slots first, which may encourage fragmentation.
  */
 recordid slottedRawRalloc(Page * page, int size);
 

src/lladd/pageCache.c

@@ -13,11 +13,8 @@
 #include <lladd/bufferManager.h>
 
 #include <assert.h>
-
-
 #include <stdio.h>
-/** @todo break dependency between pageCache and pageFile */
+
-#include "pageFile.h"
 static unsigned int bufferSize; /* < MAX_BUFFER_SIZE */
 static Page *repHead, *repMiddle, *repTail; /* replacement policy */
 
@@ -30,16 +27,11 @@ void pageCacheInit(Page * first) {
   bufferSize = 1;
   cache_state  = INITIAL;
 
-
-  
   DEBUG("pageCacheInit()");
 
   first->inCache = 1;
 
   first->prev = first->next = NULL;
-  /*  pageMap(first); */
-  pageRead(first);
-
   repHead = repTail = first;
   repMiddle = NULL;
 

src/lladd/pageFile.c

@@ -7,6 +7,7 @@
 #include <lladd/bufferManager.h>
 
 #include "pageFile.h"
+#include <lladd/pageHandle.h>
 #include <assert.h>
 #include <lladd/logger/logger2.h>
 #include <lladd/truncation.h>
@@ -169,9 +170,13 @@ static void pfForcePageFile() {
 }
 
 static void pfClosePageFile() {
+  assert(stable != -1);
   forcePageFile();
+  DEBUG("Closing storefile\n");
+
   int ret = close(stable);
 
+
   if(-1 == ret) { 
     perror("Couldn't close storefile.");
     fflush(NULL);
@@ -190,4 +195,3 @@ static pageid_t myLseekNoLock(int f, pageid_t offset, int whence) {
   }
   return ret;
 }
-

src/lladd/pageFile.h

@@ -1,12 +1,6 @@
 
 #ifndef __PAGE_FILE_H
 #define __PAGE_FILE_H
-/**
-  @todo this #include should be removed; almost nothing should 
-  include pageFile.h
-*/
-#include <lladd/pageHandle.h>
-#include "page.h"
 
 void openPageFile();
 

src/lladd/pageHandle.c

@@ -55,6 +55,7 @@ static void phForce() {
 }
 static void phClose() { 
   int err = h->close(h);
+  DEBUG("Closing pageHandle\n");
   if(err) {
     printf("Couldn't close page file: %s\n", strerror(err));
     fflush(stdout);
@@ -63,6 +64,7 @@ static void phClose() {
 }
 
 void pageHandleOpen(stasis_handle_t * handle) { 
+  DEBUG("Using pageHandle implementation\n");
   pageWrite = phWrite;
   pageRead  = phRead;
   forcePageFile = phForce;

src/lladd/transactional2.c

@@ -11,7 +11,10 @@
 #include <lladd/consumer.h>
 #include <lladd/lockManager.h>
 #include <lladd/compensations.h>
+#ifdef USE_PAGEFILE
 #include "pageFile.h"
+#endif
+#include <lladd/pageHandle.h>
 #include "page.h"
 #include <lladd/logger/logger2.h>
 #include <lladd/truncation.h>
@@ -127,7 +130,7 @@ int Tinit() {
 	dirtyPagesInit();
 	LogInit(loggerType);
 	pageInit();
-
+#ifndef USE_PAGEFILE
 	struct sf_args * slow_arg = malloc(sizeof(sf_args));
 	slow_arg->filename = STORE_FILE;
 #ifdef PAGE_FILE_O_DIRECT
@@ -137,12 +140,16 @@ int Tinit() {
 #endif
 	slow_arg->filePerm = FILE_PERM;
 	// Allow 4MB of outstanding writes.
-	stasis_handle_t * pageFile = 
+        // @todo Where / how should we open storefile?
-	  stasis_handle(open_non_blocking)(slow_factory, slow_arg, fast_factory,
+        stasis_handle_t * pageFile = 
-	  NULL, 20, PAGE_SIZE * 1024, 1024);  
+         stasis_handle(open_non_blocking)(slow_factory, slow_arg, fast_factory,
-	pageHandleOpen(pageFile); 
+                                          NULL, 20, PAGE_SIZE * 1024, 1024);
-	//	openPageFile();
+        pageHandleOpen(pageFile);
+#else
+        openPageFile();
+#endif // USE_PAGEFILE
 	bufInit(bufferManagerType);
+        DEBUG("Buffer manager type = %d\n", bufferManagerType);
 	pageOperationsInit();
 	initNestedTopActions();
 	TallocInit();
@@ -370,6 +377,7 @@ int Tdeinit() {
 	truncationDeinit();
 	ThashDeinit();
 	bufDeinit();
+        DEBUG("Closing page file tdeinit\n");
 	closePageFile();
 	pageDeinit();
 	LogDeinit();

test/dfa/ping_pong_dfa.c

@@ -69,7 +69,6 @@ int global_kill = 100000;
 
 
 /** 
-    @todo the dfa implementation uses jbhash!
     @test
 */
 START_TEST (pingpong_check) {

test/lladd/check_logWriter.c

@@ -45,9 +45,7 @@ terms specified in this license.
     
     Tests logWriter.  
 
-    @todo Change tests to apply to all logger implementations.
+    @todo Get rid of include for logWriter.h (stop calling deleteLogWriter, syncLog_logWriter...)
-
-    (Also Get rid of include for logWriter.h)
 */
 #include <config.h>
 #include <check.h>

test/lladd/check_page.c

@@ -195,9 +195,6 @@ static void* worker_thread(void * arg_ptr) {
    The number of slots allocated by the page tests is too low to check
    that freed space is recovered.
 
-   @todo While space is being reclaimed by page.c, it does not reclaim
-   slots, so freeing records still does not work properly.
-
 */
 START_TEST(pageNoThreadTest)
 {
@@ -459,10 +456,6 @@ START_TEST(pageTrecordTypeTest) {
 	recordid blob      = Talloc(xid, PAGE_SIZE * 2);
 	
 	assert(TrecordType(xid, slot) == SLOTTED_RECORD);
-	
-	/** @todo the use of the fixedRoot recordid to check recordType is 
-		  a bit questionable, but should work. */
-	
 	assert(TrecordType(xid, fixedRoot) == FIXED_RECORD);  
 	
 	fixedRoot.slot = 1;