From: Andres Freund <andres@anarazel.de>
To: Haribabu Kommi <kommi.haribabu@gmail.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, Asim R P <apraveen@pivotal.io>,
	Pg Hackers <pgsql-hackers@postgresql.org>,
	Ashwin Agrawal <aagrawal@pivotal.io>,
	Dmitry Dolgov <9erthalion6@gmail.com>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	Alexander Korotkov <a.korotkov@postgrespro.ru>
Subject: Re: Pluggable Storage - Andres's take
In-Reply-To: <20190321181557.o7sx6ul47t2ojjcm@alap3.anarazel.de>
Reply-To: 
Date: Sat, 23 Mar 2019 18:26:03 -0700
Message-ID: <8736ndm5as.fsf@alap4.lan>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Hi,

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

Greetings,

Andres Freund