Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit8aa3e47

Browse files
committed
Account for catalog snapshot in PGXACT->xmin updates.
The CatalogSnapshot was not plugged into SnapshotResetXmin()'s accountingfor whether MyPgXact->xmin could be cleared or advanced. In normaltransactions this was masked by the fact that the transaction snapshotwould be older, but during backend startup and certain utility commandsit was possible to re-use the CatalogSnapshot after MyPgXact->xmin hadbeen cleared, meaning that recently-deleted rows could be pruned eventhough this snapshot could still see them, causing unexpected cataloglookup failures. This effect appears to be the explanation for a recentfailure on buildfarm member piculet.To fix, add the CatalogSnapshot to the RegisteredSnapshots heap wheneverit is valid.In the previous logic, it was possible for the CatalogSnapshot to remainvalid across waits for client input, but with this change that would meanit delays advance of global xmin in cases where it did not before. Toavoid possibly causing new table-bloat problems with clients that sit idlefor long intervals, add code to invalidate the CatalogSnapshot beforewaiting for client input. (When the backend is busy, it's unlikely thatthe CatalogSnapshot would be the oldest snap for very long, so we don'tworry about forcing early invalidation of it otherwise.)In passing, remove the CatalogSnapshotStale flag in favor of using"CatalogSnapshot != NULL" to represent validity, as we do for the otherspecial snapshots in snapmgr.c. And improve some obsolete comments.No regression test because I don't know a deterministic way to cause thisfailure. But the stress test shown in the original discussion provokes"cache lookup failed for relation 1255" within a few dozen seconds for me.Back-patch to 9.4 where MVCC catalog scans were introduced. (Note: it'squite easy to produce similar failures with the same test case in branchesbefore 9.4. But MVCC catalog scans were supposed to fix that.)Discussion: <16447.1478818294@sss.pgh.pa.us>
1 parent7fcaec0 commit8aa3e47

File tree

3 files changed

+94
-35
lines changed

3 files changed

+94
-35
lines changed

‎src/backend/tcop/postgres.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3947,6 +3947,12 @@ PostgresMain(int argc, char *argv[],
39473947

39483948
initStringInfo(&input_message);
39493949

3950+
/*
3951+
* Also consider releasing our catalog snapshot if any, so that it's
3952+
* not preventing advance of global xmin while we wait for the client.
3953+
*/
3954+
InvalidateCatalogSnapshotConditionally();
3955+
39503956
/*
39513957
* (1) If we've reached idle state, tell the frontend we're ready for
39523958
* a new query.

‎src/backend/utils/time/snapmgr.c

Lines changed: 87 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
/*-------------------------------------------------------------------------
2+
*
23
* snapmgr.c
34
*PostgreSQL snapshot manager
45
*
@@ -8,27 +9,30 @@
89
* (tracked by separate refcounts on each snapshot), its memory can be freed.
910
*
1011
* The FirstXactSnapshot, if any, is treated a bit specially: we increment its
11-
* regd_count andcount it in RegisteredSnapshots, but this reference is not
12+
* regd_count andlist it in RegisteredSnapshots, but this reference is not
1213
* tracked by a resource owner. We used to use the TopTransactionResourceOwner
1314
* to track this snapshot reference, but that introduces logical circularity
1415
* and thus makes it impossible to clean up in a sane fashion. It's better to
1516
* handle this reference as an internally-tracked registration, so that this
1617
* module is entirely lower-level than ResourceOwners.
1718
*
1819
* Likewise, any snapshots that have been exported by pg_export_snapshot
19-
* have regd_count = 1 and arecounted in RegisteredSnapshots, but are not
20+
* have regd_count = 1 and arelisted in RegisteredSnapshots, but are not
2021
* tracked by any resource owner.
2122
*
23+
* Likewise, the CatalogSnapshot is listed in RegisteredSnapshots when it
24+
* is valid, but is not tracked by any resource owner.
25+
*
2226
* The same is true for historic snapshots used during logical decoding,
23-
* their lifetime is managed separately (as they live longeras one xact.c
27+
* their lifetime is managed separately (as they live longerthan one xact.c
2428
* transaction).
2529
*
2630
* These arrangements let us reset MyPgXact->xmin when there are no snapshots
27-
* referenced by this transaction. (One possible improvement would be to be
28-
*able to advanceXminwhen the snapshot with the earliest Xmin is no longer
29-
*referenced. That's a bit harder though, it requires more locking, and
30-
*anyway it should be rather uncommon to keep temporary snapshots referenced
31-
*for too long.)
31+
* referenced by this transaction, and advance it when the one with oldest
32+
* Xminis no longer referenced. For simplicity however, only registered
33+
*snapshots not active snapshots participate in tracking which one is oldest;
34+
*we don't try to change MyPgXact->xmin except when the active-snapshot
35+
*stack is empty.
3236
*
3337
*
3438
* Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
@@ -131,7 +135,7 @@ static volatile OldSnapshotControlData *oldSnapshotControl;
131135
* SecondarySnapshot is a snapshot that's always up-to-date as of the current
132136
* instant, even in transaction-snapshot mode. It should only be used for
133137
* special-purpose code (say, RI checking.) CatalogSnapshot points to an
134-
* MVCC snapshot intended to be used for catalog scans; we mustrefresh it
138+
* MVCC snapshot intended to be used for catalog scans; we mustinvalidate it
135139
* whenever a system catalog change occurs.
136140
*
137141
* These SnapshotData structs are static to simplify memory allocation
@@ -147,11 +151,6 @@ static Snapshot SecondarySnapshot = NULL;
147151
staticSnapshotCatalogSnapshot=NULL;
148152
staticSnapshotHistoricSnapshot=NULL;
149153

150-
/*
151-
* Staleness detection for CatalogSnapshot.
152-
*/
153-
staticboolCatalogSnapshotStale= true;
154-
155154
/*
156155
* These are updated by GetSnapshotData. We initialize them this way
157156
* for the convenience of TransactionIdIsInProgress: even in bootstrap
@@ -193,8 +192,7 @@ static ActiveSnapshotElt *OldestActiveSnapshot = NULL;
193192

194193
/*
195194
* Currently registered Snapshots. Ordered in a heap by xmin, so that we can
196-
* quickly find the one with lowest xmin, to advance our MyPgXat->xmin.
197-
* resowner.c also tracks these.
195+
* quickly find the one with lowest xmin, to advance our MyPgXact->xmin.
198196
*/
199197
staticintxmin_cmp(constpairingheap_node*a,constpairingheap_node*b,
200198
void*arg);
@@ -316,6 +314,12 @@ GetTransactionSnapshot(void)
316314
/* First call in transaction? */
317315
if (!FirstSnapshotSet)
318316
{
317+
/*
318+
* Don't allow catalog snapshot to be older than xact snapshot. Must
319+
* do this first to allow the empty-heap Assert to succeed.
320+
*/
321+
InvalidateCatalogSnapshot();
322+
319323
Assert(pairingheap_is_empty(&RegisteredSnapshots));
320324
Assert(FirstXactSnapshot==NULL);
321325

@@ -347,9 +351,6 @@ GetTransactionSnapshot(void)
347351
else
348352
CurrentSnapshot=GetSnapshotData(&CurrentSnapshotData);
349353

350-
/* Don't allow catalog snapshot to be older than xact snapshot. */
351-
CatalogSnapshotStale= true;
352-
353354
FirstSnapshotSet= true;
354355
returnCurrentSnapshot;
355356
}
@@ -358,7 +359,7 @@ GetTransactionSnapshot(void)
358359
returnCurrentSnapshot;
359360

360361
/* Don't allow catalog snapshot to be older than xact snapshot. */
361-
CatalogSnapshotStale= true;
362+
InvalidateCatalogSnapshot();
362363

363364
CurrentSnapshot=GetSnapshotData(&CurrentSnapshotData);
364365

@@ -463,36 +464,72 @@ GetNonHistoricCatalogSnapshot(Oid relid)
463464
* scan a relation for which neither catcache nor snapshot invalidations
464465
* are sent, we must refresh the snapshot every time.
465466
*/
466-
if (!CatalogSnapshotStale&& !RelationInvalidatesSnapshotsOnly(relid)&&
467+
if (CatalogSnapshot&&
468+
!RelationInvalidatesSnapshotsOnly(relid)&&
467469
!RelationHasSysCache(relid))
468-
CatalogSnapshotStale= true;
470+
InvalidateCatalogSnapshot();
469471

470-
if (CatalogSnapshotStale)
472+
if (CatalogSnapshot==NULL)
471473
{
472474
/* Get new snapshot. */
473475
CatalogSnapshot=GetSnapshotData(&CatalogSnapshotData);
474476

475477
/*
476-
* Mark new snapshost as valid. We must do this last, in case an
477-
* ERROR occurs inside GetSnapshotData().
478+
* Make sure the catalog snapshot will be accounted for in decisions
479+
* about advancing PGXACT->xmin. We could apply RegisterSnapshot, but
480+
* that would result in making a physical copy, which is overkill; and
481+
* it would also create a dependency on some resource owner, which we
482+
* do not want for reasons explained at the head of this file. Instead
483+
* just shove the CatalogSnapshot into the pairing heap manually. This
484+
* has to be reversed in InvalidateCatalogSnapshot, of course.
485+
*
486+
* NB: it had better be impossible for this to throw error, since the
487+
* CatalogSnapshot pointer is already valid.
478488
*/
479-
CatalogSnapshotStale= false;
489+
pairingheap_add(&RegisteredSnapshots,&CatalogSnapshot->ph_node);
480490
}
481491

482492
returnCatalogSnapshot;
483493
}
484494

485495
/*
486-
* Mark the current catalog snapshot as invalid. We could change this API
487-
* to allow the caller to provide more fine-grained invalidation details, so
488-
* that a change to relation A wouldn't prevent us from using our cached
489-
* snapshot to scan relation B, but so far there's no evidence that the CPU
490-
* cycles we spent tracking such fine details would be well-spent.
496+
* InvalidateCatalogSnapshot
497+
*Mark the current catalog snapshot, if any, as invalid
498+
*
499+
* We could change this API to allow the caller to provide more fine-grained
500+
* invalidation details, so that a change to relation A wouldn't prevent us
501+
* from using our cached snapshot to scan relation B, but so far there's no
502+
* evidence that the CPU cycles we spent tracking such fine details would be
503+
* well-spent.
491504
*/
492505
void
493506
InvalidateCatalogSnapshot(void)
494507
{
495-
CatalogSnapshotStale= true;
508+
if (CatalogSnapshot)
509+
{
510+
pairingheap_remove(&RegisteredSnapshots,&CatalogSnapshot->ph_node);
511+
CatalogSnapshot=NULL;
512+
SnapshotResetXmin();
513+
}
514+
}
515+
516+
/*
517+
* InvalidateCatalogSnapshotConditionally
518+
*Drop catalog snapshot if it's the only one we have
519+
*
520+
* This is called when we are about to wait for client input, so we don't
521+
* want to continue holding the catalog snapshot if it might mean that the
522+
* global xmin horizon can't advance. However, if there are other snapshots
523+
* still active or registered, the catalog snapshot isn't likely to be the
524+
* oldest one, so we might as well keep it.
525+
*/
526+
void
527+
InvalidateCatalogSnapshotConditionally(void)
528+
{
529+
if (CatalogSnapshot&&
530+
ActiveSnapshot==NULL&&
531+
pairingheap_is_singular(&RegisteredSnapshots))
532+
InvalidateCatalogSnapshot();
496533
}
497534

498535
/*
@@ -509,6 +546,7 @@ SnapshotSetCommandId(CommandId curcid)
509546
CurrentSnapshot->curcid=curcid;
510547
if (SecondarySnapshot)
511548
SecondarySnapshot->curcid=curcid;
549+
/* Should we do the same with CatalogSnapshot? */
512550
}
513551

514552
/*
@@ -526,6 +564,9 @@ SetTransactionSnapshot(Snapshot sourcesnap, TransactionId sourcexid,
526564
/* Caller should have checked this already */
527565
Assert(!FirstSnapshotSet);
528566

567+
/* Better do this to ensure following Assert succeeds. */
568+
InvalidateCatalogSnapshot();
569+
529570
Assert(pairingheap_is_empty(&RegisteredSnapshots));
530571
Assert(FirstXactSnapshot==NULL);
531572
Assert(!HistoricSnapshotActive());
@@ -918,7 +959,15 @@ xmin_cmp(const pairingheap_node *a, const pairingheap_node *b, void *arg)
918959
* Even if there are some remaining snapshots, we may be able to advance our
919960
* PGXACT->xmin to some degree. This typically happens when a portal is
920961
* dropped. For efficiency, we only consider recomputing PGXACT->xmin when
921-
* the active snapshot stack is empty.
962+
* the active snapshot stack is empty; this allows us not to need to track
963+
* which active snapshot is oldest.
964+
*
965+
* Note: it's tempting to use GetOldestSnapshot() here so that we can include
966+
* active snapshots in the calculation. However, that compares by LSN not
967+
* xmin so it's not entirely clear that it's the same thing. Also, we'd be
968+
* critically dependent on the assumption that the bottommost active snapshot
969+
* stack entry has the oldest xmin. (Current uses of GetOldestSnapshot() are
970+
* not actually critical, but this would be.)
922971
*/
923972
staticvoid
924973
SnapshotResetXmin(void)
@@ -1006,7 +1055,7 @@ AtEOXact_Snapshot(bool isCommit)
10061055
{
10071056
/*
10081057
* In transaction-snapshot mode we must release our privately-managed
1009-
* reference to the transaction snapshot. We mustdecrement
1058+
* reference to the transaction snapshot. We mustremove it from
10101059
* RegisteredSnapshots to keep the check below happy. But we don't bother
10111060
* to do FreeSnapshot, for two reasons: the memory will go away with
10121061
* TopTransactionContext anyway, and if someone has left the snapshot
@@ -1046,7 +1095,7 @@ AtEOXact_Snapshot(bool isCommit)
10461095
/*
10471096
* As with the FirstXactSnapshot, we needn't spend any effort on
10481097
* cleaning up the per-snapshot data structures, but we do need to
1049-
*unlink them from RegisteredSnapshots to prevent a warning below.
1098+
*remove them from RegisteredSnapshots to prevent a warning below.
10501099
*/
10511100
foreach(lc,exportedSnapshots)
10521101
{
@@ -1058,6 +1107,9 @@ AtEOXact_Snapshot(bool isCommit)
10581107
exportedSnapshots=NIL;
10591108
}
10601109

1110+
/* Drop catalog snapshot if any */
1111+
InvalidateCatalogSnapshot();
1112+
10611113
/* On commit, complain about leftover snapshots */
10621114
if (isCommit)
10631115
{

‎src/include/utils/snapmgr.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ extern Snapshot GetOldestSnapshot(void);
6969
externSnapshotGetCatalogSnapshot(Oidrelid);
7070
externSnapshotGetNonHistoricCatalogSnapshot(Oidrelid);
7171
externvoidInvalidateCatalogSnapshot(void);
72+
externvoidInvalidateCatalogSnapshotConditionally(void);
7273

7374
externvoidPushActiveSnapshot(Snapshotsnapshot);
7475
externvoidPushCopiedSnapshot(Snapshotsnapshot);

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp