table_tuple_lock's snapshot argument

> /*
> * Lock a tuple in the specified mode.
> *
> * Input parameters:
> * relation: relation containing tuple (caller must hold suitable lock)
> * tid: TID of tuple to lock
> * snapshot: snapshot to use for visibility determinations
> * cid: current command ID (used for visibility test, and stored into
> * tuple's cmax if lock is successful)
> * mode: lock mode desired
> * wait_policy: what to do if tuple lock is not available
> * flags:
> * If TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS, follow the update chain to
> * also lock descendant tuples if lock modes don't conflict.
> * If TUPLE_LOCK_FLAG_FIND_LAST_VERSION, follow the update chain and lock
> * latest version.
> *
> * Output parameters:
> * *slot: contains the target tuple
> * *tmfd: filled in failure cases (see below)
> *
> * Function result may be:
> * TM_Ok: lock was successfully acquired
> * TM_Invisible: lock failed because tuple was never visible to us
> * TM_SelfModified: lock failed because tuple updated by self
> * TM_Updated: lock failed because tuple updated by other xact
> * TM_Deleted: lock failed because tuple deleted by other xact
> * TM_WouldBlock: lock couldn't be acquired and wait_policy is skip
> *
> * In the failure cases other than TM_Invisible and TM_Deleted, the routine
> * fills *tmfd with the tuple's t_ctid, t_xmax, and, if possible, t_cmax. See
> * comments for struct TM_FailureData for additional info.
> */
> static inline TM_Result
> table_tuple_lock(Relation rel, ItemPointer tid, Snapshot snapshot,
> TupleTableSlot *slot, CommandId cid, LockTupleMode mode,
> LockWaitPolicy wait_policy, uint8 flags,
> TM_FailureData *tmfd)

What are the semantics of the 'snapshot' argument? In the heapam
implementation, it's not used for anything. What visibility checks the
function might do in a different implementation? I vaguely remember that
the idea was that the TID might not be sufficient to uniquely identify
the row version in something like zheap, which updates the row in place.
In that case, all the different row versions are represented by the same
TID, and the snapshot identifies the version.

There are a few callers of table_tuple_lock:

1. trigger.c: GetTupleForTrigger
2. nodeModifyTable.c
3. nodeLockRows.c
4. execReplication.c

The first three callers pass the EState's snapshot, the same that was
used in a table or index scan that returned the TID. That makes sense.
But the calls in execReplication.c look fishy:

> PushActiveSnapshot(GetLatestSnapshot());
>
> res = table_tuple_lock(rel, &(outslot->tts_tid), GetLatestSnapshot(),
> outslot,
> GetCurrentCommandId(false),
> lockmode,
> LockWaitBlock,
> 0 /* don't follow updates */ ,
> &tmfd);
>
> PopActiveSnapshot();
>
> if (should_refetch_tuple(res, &tmfd))
> goto retry;

Firstly, this calls GetLatestSnapshot() twice, which seems plain wrong.
I think the idea was to push the latest snapshot and use the same
snapshot in the call to table_tuple_lock(). But because each call to
GetLatestSnapshot() takes a new snapshot, this pushes one snapshot as
the active snapshot and passes a *different* snapshot to
table_tuple_lock(). This went wrong in commit 5db6df0c01, which
introduced the update/delete/insert/lock table AM interface. The
argument to table_tuple_lock() was supposed to be GetActiveSnapshot().

However, I think GetLatestSnapshot() is wrong here anyway. None of this
matters for heapam which just ignores the 'snapshot' argument, but let's
assume a different AM that would use the snapshot to identify the tuple
version. The TID was fetched from an index using an index scan with
SnapshotDirty. There's no guarantee that the LatestSnapshot would match
the same tuple version that the index scan found. If an MVCC snapshot is
needed, surely it should be acquired before the index scan, and used for
the index scan as well.

I see three options:

1. Remove the 'snapshot' argument, since it's not used by heapam. If we
get a table AM where a single TID represents multiple row versions, this
will need to be revisited.

2. Rewrite the recheck code in execReplication.c so that it uses the
snapshot in a more consistent fashion. Call GetLatestSnapshot() first,
and use the same snapshot in the index scan and table_tuple_lock().
Acquiring a snapshot isn't free though, so it would be nice to avoid
doing that when the heapam is just going to ignore it anyway. If we go
with this option, I think we could reuse the snapshot that is already
active in most cases, and only take a new snapshot if the tuple was
concurrently updated.

3. Modify the tableam interface so that the index scan can return a more
unique identifier of the tuple version. In heapam, it could be the TID
like today, but a different AM could return some other token. Continue
to use SnapshotDirty in the index scan, but in the call to
table_tuple_lock(), instead of passing GetLatestSnapshot() and TID, pass
the token you got index_getnext_slot().

Thoughts?

--
Heikki Linnakangas
Neon (https://neon.tech)