Re: POC: Cache data in GetSnapshotData()

On Mon, Jan 4, 2016 at 2:28 PM, Andres Freund <andres@anarazel.de> wrote:

> I think at the very least the cache should be protected by a separate
> lock, and that lock should be acquired with TryLock. I.e. the cache is
> updated opportunistically. I'd go for an lwlock in the first iteration.

I tried to implement a simple patch which protect the cache. Of all the backend which

compute the snapshot(when cache is invalid) only one of them will write to cache.

This is done with one atomic compare and swap operation.

After above fix memory corruption is not visible. But I see some more failures at higher client sessions(128 it is easily reproducible).

Errors are
DETAIL:  Key (bid)=(24) already exists.
STATEMENT:  UPDATE pgbench_branches SET bbalance = bbalance + $1 WHERE bid = $2;
client 17 aborted in state 11: ERROR:  duplicate key value violates unique constraint "pgbench_branches_pkey"
DETAIL:  Key (bid)=(24) already exists.
client 26 aborted in state 11: ERROR:  duplicate key value violates unique constraint "pgbench_branches_pkey"
DETAIL:  Key (bid)=(87) already exists.
ERROR:  duplicate key value violates unique constraint "pgbench_branches_pkey"
DETAIL:  Key (bid)=(113) already exists.

After some analysis I think In GetSnapshotData() while computing snapshot.
                /*
                 * We don't include our own XIDs (if any) in the snapshot, but we
                 * must include them in xmin.
                 */
                if (NormalTransactionIdPrecedes(xid, xmin))
                    xmin = xid;
 *********** if (pgxact == MyPgXact)   ******************
                    continue;

We do not add our own xid to xip array, I am wondering if other backend try to use

the same snapshot will it be able to see changes made by me(current backend).

I think since we compute a snapshot which will be used by other backends we need to

add our xid to xip array to tell transaction is open.