Re: logical replication busy-waiting on a lock

Hi,

On 2017-06-03 03:03:20 +0200, Petr Jelinek wrote:
> All right, here is my rough attempt on doing what Andres has suggested,
> going straight from xid to vxid, seems to work fine in my tests and
> parallel pg_dump seems happy with it as well.

Cool!

> The second patch removes the xid parameter from SnapBuildBuildSnapshot
> as it's not used for anything and skips the GetTopTransactionId() call
> as well.

Makes sense.

> That should solve the original problem reported here.

Did you verify that?

> From 4f7eb5b8fdbab2539731414e81d7a7419a83e5b1 Mon Sep 17 00:00:00 2001
> From: Petr Jelinek <pjmodos(at)pjmodos(dot)net>
> Date: Sat, 3 Jun 2017 02:06:22 +0200
> Subject: [PATCH 1/2] Use virtual transaction instead of normal ones in
> exported snapshots

> @@ -1817,8 +1818,10 @@ ProcArrayInstallImportedXmin(TransactionId xmin, TransactionId sourcexid)
> if (pgxact->vacuumFlags & PROC_IN_VACUUM)
> continue;
>
> - xid = pgxact->xid; /* fetch just once */

Hm, I don't quite understand what the 'fetch just once' bit was trying
to achieve here (not to speak of the fact that it's simply not
guaranteed this way). Therefore I think you're right to simply
disregard it.

> @@ -1741,17 +1741,17 @@ GetSerializableTransactionSnapshotInt(Snapshot snapshot,
> } while (!sxact);
>
> /* Get the snapshot, or check that it's safe to use */
> - if (!TransactionIdIsValid(sourcexid))
> + if (!sourcevxid)
> snapshot = GetSnapshotData(snapshot);
> - else if (!ProcArrayInstallImportedXmin(snapshot->xmin, sourcexid))
> + else if (!ProcArrayInstallImportedXmin(snapshot->xmin, sourcevxid))
> {
> ReleasePredXact(sxact);
> LWLockRelease(SerializableXactHashLock);
> ereport(ERROR,
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> errmsg("could not import the requested snapshot"),
> - errdetail("The source transaction %u is not running anymore.",
> - sourcexid)));
> + errdetail("The source virtual transaction %d/%u is not running anymore.",
> + sourcevxid->backendId, sourcevxid->localTransactionId)));

Hm, this is a harder to read. Wonder if we should add a pid field, that'd
make it a bit easier to interpret?

- Andres