diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index f78321d754..fce16fe6cb 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -3947,6 +3947,12 @@ PostgresMain(int argc, char *argv[], initStringInfo(&input_message); + /* + * Also consider releasing our catalog snapshot if any, so that it's + * not preventing advance of global xmin while we wait for the client. + */ + InvalidateCatalogSnapshotConditionally(); + /* * (1) If we've reached idle state, tell the frontend we're ready for * a new query. diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c index 9cbe226b22..a6264c7e20 100644 --- a/src/backend/utils/time/snapmgr.c +++ b/src/backend/utils/time/snapmgr.c @@ -1,4 +1,5 @@ /*------------------------------------------------------------------------- + * * snapmgr.c * PostgreSQL snapshot manager * @@ -8,7 +9,7 @@ * (tracked by separate refcounts on each snapshot), its memory can be freed. * * The FirstXactSnapshot, if any, is treated a bit specially: we increment its - * regd_count and count it in RegisteredSnapshots, but this reference is not + * regd_count and list it in RegisteredSnapshots, but this reference is not * tracked by a resource owner. We used to use the TopTransactionResourceOwner * to track this snapshot reference, but that introduces logical circularity * and thus makes it impossible to clean up in a sane fashion. It's better to @@ -16,19 +17,22 @@ * module is entirely lower-level than ResourceOwners. * * Likewise, any snapshots that have been exported by pg_export_snapshot - * have regd_count = 1 and are counted in RegisteredSnapshots, but are not + * have regd_count = 1 and are listed in RegisteredSnapshots, but are not * tracked by any resource owner. * + * Likewise, the CatalogSnapshot is listed in RegisteredSnapshots when it + * is valid, but is 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 + * their lifetime is managed separately (as they live longer than one xact.c * transaction). * * These arrangements let us reset MyPgXact->xmin when there are no snapshots - * referenced by this transaction. (One possible improvement would be to be - * able to advance Xmin when the snapshot with the earliest Xmin is no longer - * referenced. That's a bit harder though, it requires more locking, and - * anyway it should be rather uncommon to keep temporary snapshots referenced - * for too long.) + * referenced by this transaction, and advance it when the one with oldest + * Xmin is no longer referenced. For simplicity however, only registered + * snapshots not active snapshots participate in tracking which one is oldest; + * we don't try to change MyPgXact->xmin except when the active-snapshot + * stack is empty. * * * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group @@ -66,7 +70,7 @@ * SecondarySnapshot is a snapshot that's always up-to-date as of the current * instant, even in transaction-snapshot mode. It should only be used for * special-purpose code (say, RI checking.) CatalogSnapshot points to an - * MVCC snapshot intended to be used for catalog scans; we must refresh it + * MVCC snapshot intended to be used for catalog scans; we must invalidate it * whenever a system catalog change occurs. * * These SnapshotData structs are static to simplify memory allocation @@ -82,11 +86,6 @@ static Snapshot SecondarySnapshot = NULL; static Snapshot CatalogSnapshot = NULL; static Snapshot HistoricSnapshot = NULL; -/* - * Staleness detection for CatalogSnapshot. - */ -static bool CatalogSnapshotStale = true; - /* * These are updated by GetSnapshotData. We initialize them this way * for the convenience of TransactionIdIsInProgress: even in bootstrap @@ -125,8 +124,7 @@ static ActiveSnapshotElt *ActiveSnapshot = NULL; /* * Currently registered Snapshots. Ordered in a heap by xmin, so that we can - * quickly find the one with lowest xmin, to advance our MyPgXat->xmin. - * resowner.c also tracks these. + * quickly find the one with lowest xmin, to advance our MyPgXact->xmin. */ static int xmin_cmp(const pairingheap_node *a, const pairingheap_node *b, void *arg); @@ -201,6 +199,12 @@ GetTransactionSnapshot(void) /* First call in transaction? */ if (!FirstSnapshotSet) { + /* + * Don't allow catalog snapshot to be older than xact snapshot. Must + * do this first to allow the empty-heap Assert to succeed. + */ + InvalidateCatalogSnapshot(); + Assert(pairingheap_is_empty(&RegisteredSnapshots)); Assert(FirstXactSnapshot == NULL); @@ -232,9 +236,6 @@ GetTransactionSnapshot(void) else CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData); - /* Don't allow catalog snapshot to be older than xact snapshot. */ - CatalogSnapshotStale = true; - FirstSnapshotSet = true; return CurrentSnapshot; } @@ -243,7 +244,7 @@ GetTransactionSnapshot(void) return CurrentSnapshot; /* Don't allow catalog snapshot to be older than xact snapshot. */ - CatalogSnapshotStale = true; + InvalidateCatalogSnapshot(); CurrentSnapshot = GetSnapshotData(&CurrentSnapshotData); @@ -318,36 +319,72 @@ GetNonHistoricCatalogSnapshot(Oid relid) * scan a relation for which neither catcache nor snapshot invalidations * are sent, we must refresh the snapshot every time. */ - if (!CatalogSnapshotStale && !RelationInvalidatesSnapshotsOnly(relid) && + if (CatalogSnapshot && + !RelationInvalidatesSnapshotsOnly(relid) && !RelationHasSysCache(relid)) - CatalogSnapshotStale = true; + InvalidateCatalogSnapshot(); - if (CatalogSnapshotStale) + if (CatalogSnapshot == NULL) { /* Get new snapshot. */ CatalogSnapshot = GetSnapshotData(&CatalogSnapshotData); /* - * Mark new snapshost as valid. We must do this last, in case an - * ERROR occurs inside GetSnapshotData(). + * Make sure the catalog snapshot will be accounted for in decisions + * about advancing PGXACT->xmin. We could apply RegisterSnapshot, but + * that would result in making a physical copy, which is overkill; and + * it would also create a dependency on some resource owner, which we + * do not want for reasons explained at the head of this file. Instead + * just shove the CatalogSnapshot into the pairing heap manually. This + * has to be reversed in InvalidateCatalogSnapshot, of course. + * + * NB: it had better be impossible for this to throw error, since the + * CatalogSnapshot pointer is already valid. */ - CatalogSnapshotStale = false; + pairingheap_add(&RegisteredSnapshots, &CatalogSnapshot->ph_node); } return CatalogSnapshot; } /* - * Mark the current catalog snapshot as invalid. We could change this API - * to allow the caller to provide more fine-grained invalidation details, so - * that a change to relation A wouldn't prevent us from using our cached - * snapshot to scan relation B, but so far there's no evidence that the CPU - * cycles we spent tracking such fine details would be well-spent. + * InvalidateCatalogSnapshot + * Mark the current catalog snapshot, if any, as invalid + * + * We could change this API to allow the caller to provide more fine-grained + * invalidation details, so that a change to relation A wouldn't prevent us + * from using our cached snapshot to scan relation B, but so far there's no + * evidence that the CPU cycles we spent tracking such fine details would be + * well-spent. */ void -InvalidateCatalogSnapshot() +InvalidateCatalogSnapshot(void) { - CatalogSnapshotStale = true; + if (CatalogSnapshot) + { + pairingheap_remove(&RegisteredSnapshots, &CatalogSnapshot->ph_node); + CatalogSnapshot = NULL; + SnapshotResetXmin(); + } +} + +/* + * InvalidateCatalogSnapshotConditionally + * Drop catalog snapshot if it's the only one we have + * + * This is called when we are about to wait for client input, so we don't + * want to continue holding the catalog snapshot if it might mean that the + * global xmin horizon can't advance. However, if there are other snapshots + * still active or registered, the catalog snapshot isn't likely to be the + * oldest one, so we might as well keep it. + */ +void +InvalidateCatalogSnapshotConditionally(void) +{ + if (CatalogSnapshot && + ActiveSnapshot == NULL && + pairingheap_is_singular(&RegisteredSnapshots)) + InvalidateCatalogSnapshot(); } /* @@ -364,6 +401,7 @@ SnapshotSetCommandId(CommandId curcid) CurrentSnapshot->curcid = curcid; if (SecondarySnapshot) SecondarySnapshot->curcid = curcid; + /* Should we do the same with CatalogSnapshot? */ } /* @@ -381,6 +419,9 @@ SetTransactionSnapshot(Snapshot sourcesnap, TransactionId sourcexid, /* Caller should have checked this already */ Assert(!FirstSnapshotSet); + /* Better do this to ensure following Assert succeeds. */ + InvalidateCatalogSnapshot(); + Assert(pairingheap_is_empty(&RegisteredSnapshots)); Assert(FirstXactSnapshot == NULL); Assert(!HistoricSnapshotActive()); @@ -769,7 +810,15 @@ xmin_cmp(const pairingheap_node *a, const pairingheap_node *b, void *arg) * Even if there are some remaining snapshots, we may be able to advance our * PGXACT->xmin to some degree. This typically happens when a portal is * dropped. For efficiency, we only consider recomputing PGXACT->xmin when - * the active snapshot stack is empty. + * the active snapshot stack is empty; this allows us not to need to track + * which active snapshot is oldest. + * + * Note: it's tempting to use GetOldestSnapshot() here so that we can include + * active snapshots in the calculation. However, that compares by LSN not + * xmin so it's not entirely clear that it's the same thing. Also, we'd be + * critically dependent on the assumption that the bottommost active snapshot + * stack entry has the oldest xmin. (Current uses of GetOldestSnapshot() are + * not actually critical, but this would be.) */ static void SnapshotResetXmin(void) @@ -855,7 +904,7 @@ AtEOXact_Snapshot(bool isCommit) { /* * In transaction-snapshot mode we must release our privately-managed - * reference to the transaction snapshot. We must decrement + * reference to the transaction snapshot. We must remove it from * RegisteredSnapshots to keep the check below happy. But we don't bother * to do FreeSnapshot, for two reasons: the memory will go away with * TopTransactionContext anyway, and if someone has left the snapshot @@ -895,7 +944,7 @@ AtEOXact_Snapshot(bool isCommit) /* * As with the FirstXactSnapshot, we needn't spend any effort on * cleaning up the per-snapshot data structures, but we do need to - * unlink them from RegisteredSnapshots to prevent a warning below. + * remove them from RegisteredSnapshots to prevent a warning below. */ foreach(lc, exportedSnapshots) { @@ -907,6 +956,9 @@ AtEOXact_Snapshot(bool isCommit) exportedSnapshots = NIL; } + /* Drop catalog snapshot if any */ + InvalidateCatalogSnapshot(); + /* On commit, complain about leftover snapshots */ if (isCommit) { diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h index f8524eb687..7f21ea77d0 100644 --- a/src/include/utils/snapmgr.h +++ b/src/include/utils/snapmgr.h @@ -32,6 +32,7 @@ extern void SnapshotSetCommandId(CommandId curcid); extern Snapshot GetCatalogSnapshot(Oid relid); extern Snapshot GetNonHistoricCatalogSnapshot(Oid relid); extern void InvalidateCatalogSnapshot(void); +extern void InvalidateCatalogSnapshotConditionally(void); extern void PushActiveSnapshot(Snapshot snapshot); extern void PushCopiedSnapshot(Snapshot snapshot);