Re: [HACKERS] Pluggable storage

I took a look on this patch.  I've following notes for now.

tuple_insert_hook tuple_insert; /* heap_insert */
tuple_update_hook tuple_update; /* heap_update */
tuple_delete_hook tuple_delete; /* heap_delete */

I don't think this set of functions provides good enough level of abstraction for storage AM.  This functions encapsulate only low-level work of insert/update/delete tuples into heap itself.  However, it still assumed that indexes are managed outside of storage AM.  I don't think this is right, assuming that one of most demanded storage API usage would be different MVCC implementation (for instance, UNDO log based as discussed upthread).  Different MVCC implementation is likely going to manage indexes in a different way.  For example, storage AM utilizing UNDO would implement in-place update even when indexed columns are modified.  Therefore this piece of code in ExecUpdate() wouldn't be relevant anymore.

/*
* insert index entries for tuple
*
* Note: heap_update returns the tid (location) of the new tuple in
* the t_self field.
*
* If it's a HOT update, we mustn't insert new index entries.
*/
if ((resultRelInfo->ri_NumIndices > 0) && !storage_tuple_is_heaponly(resultRelationDesc, tuple))
recheckIndexes = ExecInsertIndexTuples(slot, &(slot->tts_tid),
  estate, false, NULL, NIL);

I'm firmly convinced that this logic should be encapsulated into storage AM altogether with inserting new index tuples on storage insert.  Also, HOT should be completely encapsulated into heapam.  It's quite evident for me that storage utilizing UNDO wouldn't have anything like our current HOT.  Therefore, I think there shouldn't be hot_search_buffer() API function.  tuple_fetch() may cover hot_search_buffer(). That might require some signature change of tuple_fetch() (probably, extra arguments).