On 02/10/2015 02:46 PM, Andres Freund wrote:
On 2015-02-10 21:20:37 +0900, Michael Paquier wrote:Using test_decoding on HEAD (cc761b1) I am seeing the following assertion failure: TRAP: FailedAssertion("!(!((&RegisteredSnapshots)->ph_root == ((void*)0)))", File: "snapmgr.c", Line: 677)Ick. I guess a revert of 94028691609f8e148bd4ce72c46163f018832a5b fixes it? ... Yea, it really looks like the above commit is to "blame". The new xmin tracking infrastructure doesn't know about the historical snapshot...
The new xmin tracking code assumes that if a snapshots's regd_count > 0, it has already been pushed to the RegisteredSnapshots heap. That assumption doesn't hold because the logical decoding stuff creates its own snapshots and sets regd_count=1 to prevent snapmgr.c from freeing them, even though they haven't been registered with RegisterSnapshot.
The second paragraph in this comment in snapmgr.c needs fixing:
* Likewise, any snapshots that have been exported by pg_export_snapshot * have regd_count = 1 and are counted in RegisteredSnapshots, but are not * tracked by any resource owner. * * The same is true for historic snapshots used during logical decoding, * their lifetime is managed separately (as they life longer as one xact.c * transaction).
Besides the spelling, that's incorrect: historic snapshots are *not* counted in RegisteredSnapshots. That was harmless before commit 94028691, but it was always wrong.
I think setting regd_count=1 outside snapmgr.c is a pretty ugly hack. snapbuild.c also abuses active_count as a reference counter, which is similarly ugly. I think regd_count and active_count should both be treated as private to snapmgr.c, and initialized to zero elsewhere.
As a minimal fix, we could change the logical decoding code to not use regd_count to prevent snapmgr.c from freeing its snapshots, but use active_count for that too. Setting regd_count to 1 in SnapBuildBuildSnapshot() seems unnecessary in the first place, as the callers also call SnapBuildSnapIncRefcount(). Patch attached.
But could we get rid of those active_count manipulations too? Could you replace the SnapBuildSnap[Inc|Dec]Refcount calls with [Un]RegisterSnapshot()?
- Heikki
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index bcd5896..1f76731 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -1188,8 +1188,8 @@ ReorderBufferCopySnap(ReorderBuffer *rb, Snapshot orig_snap, memcpy(snap, orig_snap, sizeof(SnapshotData)); snap->copied = true; - snap->active_count = 0; - snap->regd_count = 1; + snap->active_count = 1; + snap->regd_count = 0; snap->xip = (TransactionId *) (snap + 1); memcpy(snap->xip, orig_snap->xip, sizeof(TransactionId) * snap->xcnt); diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c index e911453..80f5b44 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -348,7 +348,7 @@ SnapBuildFreeSnapshot(Snapshot snap) Assert(snap->curcid == FirstCommandId); Assert(!snap->suboverflowed); Assert(!snap->takenDuringRecovery); - Assert(snap->regd_count == 1); + Assert(snap->regd_count == 0); /* slightly more likely, so it's checked even without c-asserts */ if (snap->copied) @@ -407,16 +407,16 @@ SnapBuildSnapDecRefcount(Snapshot snap) Assert(!snap->suboverflowed); Assert(!snap->takenDuringRecovery); - Assert(snap->regd_count == 1); + Assert(snap->regd_count == 0); - Assert(snap->active_count); + Assert(snap->active_count > 0); /* slightly more likely, so it's checked even without casserts */ if (snap->copied) elog(ERROR, "cannot free a copied snapshot"); snap->active_count--; - if (!snap->active_count) + if (snap->active_count == 0) SnapBuildFreeSnapshot(snap); } @@ -495,7 +495,7 @@ SnapBuildBuildSnapshot(SnapBuild *builder, TransactionId xid) snapshot->copied = false; snapshot->curcid = FirstCommandId; snapshot->active_count = 0; - snapshot->regd_count = 1; /* mark as registered so nobody frees it */ + snapshot->regd_count = 0; return snapshot; }
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
