Tom Lane escribió:
> Alvaro Herrera <[EMAIL PROTECTED]> writes:
> > After the patch we don't have any way to detect whether resowner.c has
> > any snapshot still linked to. I assume there's no objection to adding a
> > new entry point in resowner.c for this.
>
> Hmm, that's a bit problematic because resowner.c doesn't have any global
> notion of what resource owners exist. I think you still need to have
> snapmgr.c maintain a list of all known snapshots. resowner.c can only
> help you with tracking reference counts for particular snapshots.
A counter seems to suffice. Patch attached.
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Index: src/backend/utils/resowner/resowner.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/resowner/resowner.c,v
retrieving revision 1.29
diff -c -p -r1.29 resowner.c
*** src/backend/utils/resowner/resowner.c 19 Jun 2008 00:46:05 -0000 1.29
--- src/backend/utils/resowner/resowner.c 25 Nov 2008 17:10:22 -0000
***************
*** 26,31 ****
--- 26,32 ----
#include "utils/memutils.h"
#include "utils/rel.h"
#include "utils/resowner.h"
+ #include "utils/snapmgr.h"
/*
*************** typedef struct ResourceOwnerData
*** 66,71 ****
--- 67,77 ----
int ntupdescs; /* number of owned tupdesc references */
TupleDesc *tupdescs; /* dynamically allocated array */
int maxtupdescs; /* currently allocated array size */
+
+ /* We have built-in support for remembering snapshot references */
+ int nsnapshots; /* number of owned snapshot references */
+ Snapshot *snapshots; /* dynamically allocated array */
+ int maxsnapshots; /* currently allocated array size */
} ResourceOwnerData;
*************** static void ResourceOwnerReleaseInternal
*** 98,103 ****
--- 104,110 ----
static void PrintRelCacheLeakWarning(Relation rel);
static void PrintPlanCacheLeakWarning(CachedPlan *plan);
static void PrintTupleDescLeakWarning(TupleDesc tupdesc);
+ static void PrintSnapshotLeakWarning(Snapshot snapshot);
/*****************************************************************************
*************** ResourceOwnerReleaseInternal(ResourceOwn
*** 301,306 ****
--- 308,320 ----
PrintTupleDescLeakWarning(owner->tupdescs[owner->ntupdescs - 1]);
DecrTupleDescRefCount(owner->tupdescs[owner->ntupdescs - 1]);
}
+ /* Ditto for snapshot references */
+ while (owner->nsnapshots > 0)
+ {
+ if (isCommit)
+ PrintSnapshotLeakWarning(owner->snapshots[owner->nsnapshots -1]);
+ UnregisterSnapshot(owner->snapshots[owner->nsnapshots -1]);
+ }
/* Clean up index scans too */
ReleaseResources_hash();
*************** ResourceOwnerDelete(ResourceOwner owner)
*** 332,337 ****
--- 346,352 ----
Assert(owner->nrelrefs == 0);
Assert(owner->nplanrefs == 0);
Assert(owner->ntupdescs == 0);
+ Assert(owner->nsnapshots == 0);
/*
* Delete children. The recursive call will delink the child from me, so
*************** ResourceOwnerDelete(ResourceOwner owner)
*** 360,365 ****
--- 375,382 ----
pfree(owner->planrefs);
if (owner->tupdescs)
pfree(owner->tupdescs);
+ if (owner->snapshots)
+ pfree(owner->snapshots);
pfree(owner);
}
*************** PrintTupleDescLeakWarning(TupleDesc tupd
*** 936,938 ****
--- 953,1037 ----
"TupleDesc reference leak: TupleDesc %p (%u,%d) still referenced",
tupdesc, tupdesc->tdtypeid, tupdesc->tdtypmod);
}
+
+ /*
+ * Make sure there is room for at least one more entry in a ResourceOwner's
+ * snapshot reference array.
+ *
+ * This is separate from actually inserting an entry because if we run out
+ * of memory, it's critical to do so *before* acquiring the resource.
+ */
+ void
+ ResourceOwnerEnlargeSnapshots(ResourceOwner owner)
+ {
+ int newmax;
+
+ if (owner->nsnapshots < owner->maxsnapshots)
+ return; /* nothing to do */
+
+ if (owner->snapshots == NULL)
+ {
+ newmax = 16;
+ owner->snapshots = (Snapshot *)
+ MemoryContextAlloc(TopMemoryContext, newmax * sizeof(Snapshot));
+ owner->maxsnapshots = newmax;
+ }
+ else
+ {
+ newmax = owner->maxsnapshots * 2;
+ owner->snapshots = (Snapshot *)
+ repalloc(owner->snapshots, newmax * sizeof(Snapshot));
+ owner->maxsnapshots = newmax;
+ }
+ }
+
+ /*
+ * Remember that a snapshot reference is owned by a ResourceOwner
+ *
+ * Caller must have previously done ResourceOwnerEnlargeSnapshots()
+ */
+ void
+ ResourceOwnerRememberSnapshot(ResourceOwner owner, Snapshot snapshot)
+ {
+ Assert(owner->nsnapshots < owner->maxsnapshots);
+ owner->snapshots[owner->nsnapshots] = snapshot;
+ owner->nsnapshots++;
+ }
+
+ /*
+ * Forget that a snapshot reference is owned by a ResourceOwner
+ */
+ void
+ ResourceOwnerForgetSnapshot(ResourceOwner owner, Snapshot snapshot)
+ {
+ Snapshot *snapshots = owner->snapshots;
+ int ns1 = owner->nsnapshots -1;
+ int i;
+
+ for (i = ns1; i >= 0; i--)
+ {
+ if (snapshots[i] == snapshot)
+ {
+ while (i < ns1)
+ {
+ snapshots[i] = snapshots[i + 1];
+ i++;
+ }
+ owner->nsnapshots = ns1;
+ return;
+ }
+ }
+ elog(ERROR, "snapshot reference %p is not owned by resource owner %s",
+ snapshot, owner->name);
+ }
+
+ /*
+ * Debugging subroutine
+ */
+ static void
+ PrintSnapshotLeakWarning(Snapshot snapshot)
+ {
+ elog(WARNING,
+ "Snapshot reference leak: Snapshot %p still referenced",
+ snapshot);
+ }
Index: src/backend/utils/time/snapmgr.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/time/snapmgr.c,v
retrieving revision 1.6
diff -c -p -r1.6 snapmgr.c
*** src/backend/utils/time/snapmgr.c 27 Oct 2008 22:15:05 -0000 1.6
--- src/backend/utils/time/snapmgr.c 25 Nov 2008 19:26:47 -0000
***************
*** 2,8 ****
* snapmgr.c
* PostgreSQL snapshot manager
*
! * We keep track of snapshots in two ways: the "registered snapshots" list,
* and the "active snapshot" stack. All snapshots in either of them live in
* persistent memory. When a snapshot is no longer in any of these lists
* (tracked by separate refcounts on each snapshot), its memory can be freed.
--- 2,8 ----
* snapmgr.c
* PostgreSQL snapshot manager
*
! * We keep track of snapshots in two ways: those "registered" by resowner.c,
* and the "active snapshot" stack. All snapshots in either of them live in
* persistent memory. When a snapshot is no longer in any of these lists
* (tracked by separate refcounts on each snapshot), its memory can be freed.
***************
*** 14,22 ****
* anyway it should be rather uncommon to keep snapshots referenced for too
* long.)
*
- * Note: parts of this code could probably be replaced by appropriate use
- * of resowner.c.
- *
*
* Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
--- 14,19 ----
***************
*** 34,39 ****
--- 31,37 ----
#include "storage/procarray.h"
#include "utils/memutils.h"
#include "utils/memutils.h"
+ #include "utils/resowner.h"
#include "utils/snapmgr.h"
#include "utils/tqual.h"
*************** TransactionId RecentXmin = FirstNormalTr
*** 69,101 ****
TransactionId RecentGlobalXmin = InvalidTransactionId;
/*
- * Elements of the list of registered snapshots.
- *
- * Note that we keep refcounts both here and in SnapshotData. This is because
- * the same snapshot may be registered more than once in a subtransaction, and
- * if a subxact aborts we want to be able to subtract the correct amount of
- * counts from SnapshotData. (Another approach would be keeping one
- * RegdSnapshotElt each time a snapshot is registered, but that seems
- * unnecessary wastage.)
- *
- * NB: the code assumes that elements in this list are in non-increasing
- * order of s_level; also, the list must be NULL-terminated.
- */
- typedef struct RegdSnapshotElt
- {
- Snapshot s_snap;
- uint32 s_count;
- int s_level;
- struct RegdSnapshotElt *s_next;
- } RegdSnapshotElt;
-
- /*
* Elements of the active snapshot stack.
*
! * It's not necessary to keep a refcount like we do for the registered list;
! * each element here accounts for exactly one active_count on SnapshotData.
! * We cannot condense them like we do for RegdSnapshotElt because it would mess
! * up the order of entries in the stack.
*
* NB: the code assumes that elements in this list are in non-increasing
* order of as_level; also, the list must be NULL-terminated.
--- 67,75 ----
TransactionId RecentGlobalXmin = InvalidTransactionId;
/*
* Elements of the active snapshot stack.
*
! * Each element here accounts for exactly one active_count on SnapshotData.
*
* NB: the code assumes that elements in this list are in non-increasing
* order of as_level; also, the list must be NULL-terminated.
*************** typedef struct ActiveSnapshotElt
*** 107,118 ****
struct ActiveSnapshotElt *as_next;
} ActiveSnapshotElt;
- /* Head of the list of registered snapshots */
- static RegdSnapshotElt *RegisteredSnapshotList = NULL;
-
/* Top of the stack of active snapshots */
static ActiveSnapshotElt *ActiveSnapshot = NULL;
/* first GetTransactionSnapshot call in a transaction? */
bool FirstSnapshotSet = false;
--- 81,92 ----
struct ActiveSnapshotElt *as_next;
} ActiveSnapshotElt;
/* Top of the stack of active snapshots */
static ActiveSnapshotElt *ActiveSnapshot = NULL;
+ /* How many snapshots is resowner.c tracking for us? */
+ static int RegisteredSnapshots = 0;
+
/* first GetTransactionSnapshot call in a transaction? */
bool FirstSnapshotSet = false;
*************** static void SnapshotResetXmin(void);
*** 133,139 ****
* GetTransactionSnapshot
* Get the appropriate snapshot for a new query in a transaction.
*
- *
* Note that the return value may point at static storage that will be modified
* by future calls and by CommandCounterIncrement(). Callers should call
* RegisterSnapshot or PushActiveSnapshot on the returned snap if it is to be
--- 107,112 ----
*************** GetTransactionSnapshot(void)
*** 145,150 ****
--- 118,125 ----
/* First call in transaction? */
if (!FirstSnapshotSet)
{
+ Assert(RegisteredSnapshots == 0);
+
CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData);
FirstSnapshotSet = true;
*************** ActiveSnapshotSet(void)
*** 371,478 ****
Snapshot
RegisterSnapshot(Snapshot snapshot)
{
! RegdSnapshotElt *elt;
! RegdSnapshotElt *newhead;
! int level;
if (snapshot == InvalidSnapshot)
return InvalidSnapshot;
- level = GetCurrentTransactionNestLevel();
-
- /*
- * If there's already an item in the list for the same snapshot and the
- * same subxact nest level, increment its refcounts. Otherwise create a
- * new one.
- */
- for (elt = RegisteredSnapshotList; elt != NULL; elt = elt->s_next)
- {
- if (elt->s_level < level)
- break;
-
- if (elt->s_snap == snapshot && elt->s_level == level)
- {
- elt->s_snap->regd_count++;
- elt->s_count++;
-
- return elt->s_snap;
- }
- }
-
- /*
- * Create the new list element. If it's not been copied into persistent
- * memory already, we must do so; otherwise we can just increment the
- * reference count.
- */
- newhead = MemoryContextAlloc(TopTransactionContext, sizeof(RegdSnapshotElt));
- newhead->s_next = RegisteredSnapshotList;
/* Static snapshot? Create a persistent copy */
! newhead->s_snap = snapshot->copied ? snapshot : CopySnapshot(snapshot);
! newhead->s_level = level;
! newhead->s_count = 1;
! newhead->s_snap->regd_count++;
! RegisteredSnapshotList = newhead;
! return RegisteredSnapshotList->s_snap;
}
/*
* UnregisterSnapshot
- * Signals that a snapshot is no longer necessary
*
! * If both reference counts fall to zero, the snapshot memory is released.
! * If only the registered list refcount falls to zero, just the list element is
! * freed.
*/
void
UnregisterSnapshot(Snapshot snapshot)
{
! RegdSnapshotElt *prev = NULL;
! RegdSnapshotElt *elt;
! bool found = false;
!
! if (snapshot == InvalidSnapshot)
return;
! for (elt = RegisteredSnapshotList; elt != NULL; elt = elt->s_next)
! {
! if (elt->s_snap == snapshot)
! {
! Assert(elt->s_snap->regd_count > 0);
! Assert(elt->s_count > 0);
! elt->s_snap->regd_count--;
! elt->s_count--;
! found = true;
!
! if (elt->s_count == 0)
! {
! /* delink it from the registered snapshot list */
! if (prev)
! prev->s_next = elt->s_next;
! else
! RegisteredSnapshotList = elt->s_next;
!
! /* free the snapshot itself if it's no longer relevant */
! if (elt->s_snap->regd_count == 0 && elt->s_snap->active_count == 0)
! FreeSnapshot(elt->s_snap);
!
! /* and free the list element */
! pfree(elt);
! }
!
! break;
! }
!
! prev = elt;
}
-
- if (!found)
- elog(WARNING, "unregistering failed for snapshot %p", snapshot);
-
- SnapshotResetXmin();
}
/*
--- 346,392 ----
Snapshot
RegisterSnapshot(Snapshot snapshot)
{
! Snapshot snap;
if (snapshot == InvalidSnapshot)
return InvalidSnapshot;
/* Static snapshot? Create a persistent copy */
! snap = snapshot->copied ? snapshot : CopySnapshot(snapshot);
! /* and tell resowner.c about it */
! ResourceOwnerEnlargeSnapshots(CurrentResourceOwner);
! snap->regd_count++;
! ResourceOwnerRememberSnapshot(CurrentResourceOwner, snap);
! RegisteredSnapshots++;
! return snap;
}
/*
* UnregisterSnapshot
*
! * Decrement the reference count of a snapshot, remove the corresponding
! * reference from CurrentResourceOwner, and free the snapshot if no more
! * references remain.
*/
void
UnregisterSnapshot(Snapshot snapshot)
{
! if (snapshot == NULL)
return;
! Assert(snapshot->regd_count > 0);
! Assert(RegisteredSnapshots > 0);
! ResourceOwnerForgetSnapshot(CurrentResourceOwner, snapshot);
! RegisteredSnapshots--;
! if (--snapshot->regd_count == 0 && snapshot->active_count == 0)
! {
! FreeSnapshot(snapshot);
! SnapshotResetXmin();
}
}
/*
*************** UnregisterSnapshot(Snapshot snapshot)
*** 485,491 ****
static void
SnapshotResetXmin(void)
{
! if (RegisteredSnapshotList == NULL && ActiveSnapshot == NULL)
MyProc->xmin = InvalidTransactionId;
}
--- 399,405 ----
static void
SnapshotResetXmin(void)
{
! if (RegisteredSnapshots == 0 && ActiveSnapshot == NULL)
MyProc->xmin = InvalidTransactionId;
}
*************** void
*** 496,502 ****
AtSubCommit_Snapshot(int level)
{
ActiveSnapshotElt *active;
- RegdSnapshotElt *regd;
/*
* Relabel the active snapshots set in this subtransaction as though they
--- 410,415 ----
*************** AtSubCommit_Snapshot(int level)
*** 508,527 ****
break;
active->as_level = level - 1;
}
-
- /*
- * Reassign all registered snapshots to the parent subxact.
- *
- * Note: this code is somewhat bogus in that we could end up with multiple
- * entries for the same snapshot and the same subxact level (my parent's
- * level). Cleaning that up is more trouble than it's currently worth,
- * however.
- */
- for (regd = RegisteredSnapshotList; regd != NULL; regd = regd->s_next)
- {
- if (regd->s_level == level)
- regd->s_level--;
- }
}
/*
--- 421,426 ----
*************** AtSubCommit_Snapshot(int level)
*** 531,539 ****
void
AtSubAbort_Snapshot(int level)
{
- RegdSnapshotElt *prev;
- RegdSnapshotElt *regd;
-
/* Forget the active snapshots set by this subtransaction */
while (ActiveSnapshot && ActiveSnapshot->as_level >= level)
{
--- 430,435 ----
*************** AtSubAbort_Snapshot(int level)
*** 558,596 ****
ActiveSnapshot = next;
}
- /* Unregister all snapshots registered during this subtransaction */
- prev = NULL;
- for (regd = RegisteredSnapshotList; regd != NULL; )
- {
- if (regd->s_level >= level)
- {
- RegdSnapshotElt *tofree;
-
- if (prev)
- prev->s_next = regd->s_next;
- else
- RegisteredSnapshotList = regd->s_next;
-
- tofree = regd;
- regd = regd->s_next;
-
- tofree->s_snap->regd_count -= tofree->s_count;
-
- /* free the snapshot if possible */
- if (tofree->s_snap->regd_count == 0 &&
- tofree->s_snap->active_count == 0)
- FreeSnapshot(tofree->s_snap);
-
- /* and free the list element */
- pfree(tofree);
- }
- else
- {
- prev = regd;
- regd = regd->s_next;
- }
- }
-
SnapshotResetXmin();
}
--- 454,459 ----
*************** AtEOXact_Snapshot(bool isCommit)
*** 605,611 ****
if (isCommit)
{
ActiveSnapshotElt *active;
- RegdSnapshotElt *regd;
/*
* On a serializable snapshot we must first unregister our private
--- 468,473 ----
*************** AtEOXact_Snapshot(bool isCommit)
*** 614,629 ****
if (registered_serializable)
UnregisterSnapshot(CurrentSnapshot);
/* complain about unpopped active snapshots */
for (active = ActiveSnapshot; active != NULL; active = active->as_next)
elog(WARNING, "snapshot %p still active", active);
-
- /* complain about any unregistered snapshot */
- for (regd = RegisteredSnapshotList; regd != NULL; regd = regd->s_next)
- elog(WARNING,
- "snapshot %p not destroyed at commit (%d regd refs, %d active refs)",
- regd->s_snap, regd->s_snap->regd_count,
- regd->s_snap->active_count);
}
/*
--- 476,488 ----
if (registered_serializable)
UnregisterSnapshot(CurrentSnapshot);
+ if (RegisteredSnapshots != 0)
+ elog(WARNING, "%d registered snapshots seem to remain after cleanup",
+ RegisteredSnapshots);
+
/* complain about unpopped active snapshots */
for (active = ActiveSnapshot; active != NULL; active = active->as_next)
elog(WARNING, "snapshot %p still active", active);
}
/*
*************** AtEOXact_Snapshot(bool isCommit)
*** 631,637 ****
* it'll go away with TopTransactionContext.
*/
ActiveSnapshot = NULL;
! RegisteredSnapshotList = NULL;
CurrentSnapshot = NULL;
SecondarySnapshot = NULL;
--- 490,496 ----
* it'll go away with TopTransactionContext.
*/
ActiveSnapshot = NULL;
! RegisteredSnapshots = 0;
CurrentSnapshot = NULL;
SecondarySnapshot = NULL;
Index: src/include/utils/resowner.h
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/include/utils/resowner.h,v
retrieving revision 1.15
diff -c -p -r1.15 resowner.h
*** src/include/utils/resowner.h 1 Jan 2008 19:45:59 -0000 1.15
--- src/include/utils/resowner.h 25 Nov 2008 16:30:16 -0000
***************
*** 22,27 ****
--- 22,28 ----
#include "storage/buf.h"
#include "utils/catcache.h"
#include "utils/plancache.h"
+ #include "utils/snapshot.h"
/*
*************** extern void ResourceOwnerRememberTupleDe
*** 121,124 ****
--- 122,132 ----
extern void ResourceOwnerForgetTupleDesc(ResourceOwner owner,
TupleDesc tupdesc);
+ /* support for snapshot refcount management */
+ extern void ResourceOwnerEnlargeSnapshots(ResourceOwner owner);
+ extern void ResourceOwnerRememberSnapshot(ResourceOwner owner,
+ Snapshot snapshot);
+ extern void ResourceOwnerForgetSnapshot(ResourceOwner owner,
+ Snapshot snapshot);
+
#endif /* RESOWNER_H */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
