Re: [HACKERS] Pluggable storage

Hi!

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.

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

LockTupleMode and HeapUpdateFailureData shouldn't be private of heapam.  Any fullweight OLTP storage AM should support our tuple lock modes and should be able to report update failures.  HeapUpdateFailureData should be renamed to something like StorageUpdateFailureData.  Contents of HeapUpdateFailureData seems to me general enough to be supported by any storage with ItemPointer tuple locator.

storage_setscanlimits() is used only during index build.  I think that since storage AM may have different MVCC implementation then storage AM should decide how to communicate with indexes including index build.  Therefore, instead of exposing storage_setscanlimits(), the whole IndexBuildHeapScan() should be encapsulated into storage AM.

Also, BulkInsertState should be private of structure of heapam.  Another storages may have another state for bulk insert.  On API level we might have some abstract pointer instead of BulkInsertState while having GetBulkInsertState and others as API methods.

storage_freeze_tuple() is called only once from rewrite_heap_tuple().  That makes me think that tuple_freeze API method is wrong for abstraction.  We probably should make rewrite_heap_tuple() or even the whole rebuild_relation() an API method...

Heap reloptions are untouched for now.  Storage AM should be able to provide its own specific options just like index AMs do.

That's all I have for now.