Re: Pluggable Storage - Andres's take

Hi,

(sorry, I somehow miskeyed, and sent a partial version of this email
before it was ready)

On 2019-03-21 11:15:57 -0700, Andres Freund wrote:
> Pending work:
> - Wondering if table_insert/delete/update should rather be
> table_tuple_insert etc. Would be a bit more consistent with the
> callback names, but a bigger departure from existing code.

I've left this as is.

> - I'm not yet happy with TableTupleDeleted computation in heapam.c, I
> want to revise that further

I changed that. Found a bunch of untested paths, I've pushed tests for
those already.

> - formatting

Done that.

> - commit message

Done that.

> - a few comments need a bit of polishing (ExecCheckTIDVisible, heapam_tuple_lock)

Done that.

> - Rename TableTupleMayBeModified to TableTupleOk, but also probably a s/TableTuple/TableMod/

It's now TM_*.

/*
* Result codes for table_{update,delete,lock}_tuple, and for visibility
* routines inside table AMs.
*/
typedef enum TM_Result
{
/*
* Signals that the action succeeded (i.e. update/delete performed, lock
* was acquired)
*/
TM_Ok,

/* The affected tuple wasn't visible to the relevant snapshot */
TM_Invisible,

/* The affected tuple was already modified by the calling backend */
TM_SelfModified,

/*
* The affected tuple was updated by another transaction. This includes
* the case where tuple was moved to another partition.
*/
TM_Updated,

/* The affected tuple was deleted by another transaction */
TM_Deleted,

/*
* The affected tuple is currently being modified by another session. This
* will only be returned if (update/delete/lock)_tuple are instructed not
* to wait.
*/
TM_BeingModified,

/* lock couldn't be acquired, action skipped. Only used by lock_tuple */
TM_WouldBlock
} TM_Result;

> - I'll probably move TUPLE_LOCK_FLAG_LOCK_* into tableam.h

Done.

> - two more passes through the patch

One of them completed. Which is good, because there was a subtle bug in
heapam_tuple_lock (*tid was adjusted to be the followup tuple after the
heap_fetch(), before going to heap_lock_tuple - but that's wrong, it
should only be adjusted when heap_fetch() ing the next version.).

I'm pretty happy with that last version (of the first patch). I'm
planning to do one more pass, and then push.

There are no meaningful changes to later patches in the series besides
followup changes required from changes in the first patch.

Greetings,

Andres Freund