Skip to content

Commit cbe9d6b

Browse files
tglsfdctglsfdc
committed
Fix up rickety handling of relation-truncation interlocks.
Move rd_targblock, rd_fsm_nblocks, and rd_vm_nblocks from relcache to the smgr relation entries, so that they will get reset to InvalidBlockNumber whenever an smgr-level flush happens. Because we now send smgr invalidation messages immediately (not at end of transaction) when a relation truncation occurs, this ensures that other backends will reset their values before they next access the relation. We no longer need the unreliable assumption that a VACUUM that's doing a truncation will hold its AccessExclusive lock until commit --- in fact, we can intentionally release that lock as soon as we've completed the truncation. This patch therefore reverts (most of) Alvaro's patch of 2009-11-10, as well as my marginal hacking on it yesterday. We can also get rid of assorted no-longer-needed relcache flushes, which are far more expensive than an smgr flush because they kill a lot more state. In passing this patch fixes smgr_redo's failure to perform visibility-map truncation, and cleans up some rather dubious assumptions in freespace.c and visibilitymap.c about when rd_fsm_nblocks and rd_vm_nblocks can be out of date.
1 parent 79647ee commit cbe9d6b

File tree

15 files changed

+207
-216
lines changed

15 files changed

+207
-216
lines changed

‎src/backend/access/heap/hio.c

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/access/heap/hio.c,v 1.77 2010/01/02 16:57:34 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/access/heap/hio.c,v 1.78 2010/02/09 21:43:29 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -20,6 +20,7 @@
2020
#include "storage/bufmgr.h"
2121
#include "storage/freespace.h"
2222
#include "storage/lmgr.h"
23+
#include "storage/smgr.h"
2324

2425

2526
/*
@@ -126,7 +127,7 @@ ReadBufferBI(Relation relation, BlockNumber targetBlock,
126127
*
127128
* HEAP_INSERT_SKIP_FSM is also useful for non-WAL-logged additions to a
128129
* relation, if the caller holds exclusive lock and is careful to invalidate
129-
* relation->rd_targblock before the first insertion --- that ensures that
130+
* relation's smgr_targblock before the first insertion --- that ensures that
130131
* all insertions will occur into newly added pages and not be intermixed
131132
* with tuples from other transactions. That way, a crash can't risk losing
132133
* any committed data of other transactions. (See heap_insert's comments
@@ -206,7 +207,7 @@ RelationGetBufferForTuple(Relation relation, Size len,
206207
else if (bistate && bistate->current_buf != InvalidBuffer)
207208
targetBlock = BufferGetBlockNumber(bistate->current_buf);
208209
else
209-
targetBlock = relation->rd_targblock;
210+
targetBlock = RelationGetTargetBlock(relation);
210211

211212
if (targetBlock == InvalidBlockNumber && use_fsm)
212213
{
@@ -273,7 +274,7 @@ RelationGetBufferForTuple(Relation relation, Size len,
273274
if (len + saveFreeSpace <= pageFreeSpace)
274275
{
275276
/* use this page as future insert target, too */
276-
relation->rd_targblock = targetBlock;
277+
RelationSetTargetBlock(relation, targetBlock);
277278
return buffer;
278279
}
279280

@@ -377,7 +378,7 @@ RelationGetBufferForTuple(Relation relation, Size len,
377378
* current backend to make more insertions or not, which is probably a
378379
* good bet most of the time. So for now, don't add it to FSM yet.
379380
*/
380-
relation->rd_targblock = BufferGetBlockNumber(buffer);
381+
RelationSetTargetBlock(relation, BufferGetBlockNumber(buffer));
381382

382383
return buffer;
383384
}

‎src/backend/access/heap/visibilitymap.c

Lines changed: 46 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/access/heap/visibilitymap.c,v 1.7 2010/01/02 16:57:35 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/access/heap/visibilitymap.c,v 1.8 2010/02/09 21:43:29 tgl Exp $
1212
*
1313
* INTERFACE ROUTINES
1414
* visibilitymap_clear - clear a bit in the visibility map
@@ -94,7 +94,7 @@
9494
#include "storage/bufpage.h"
9595
#include "storage/lmgr.h"
9696
#include "storage/smgr.h"
97-
#include "utils/inval.h"
97+
9898

9999
/*#define TRACE_VISIBILITYMAP */
100100

@@ -291,7 +291,13 @@ visibilitymap_test(Relation rel, BlockNumber heapBlk, Buffer *buf)
291291
}
292292

293293
/*
294-
* visibilitymap_test - truncate the visibility map
294+
* visibilitymap_truncate - truncate the visibility map
295+
*
296+
* The caller must hold AccessExclusiveLock on the relation, to ensure that
297+
* other backends receive the smgr invalidation event that this function sends
298+
* before they access the VM again.
299+
*
300+
* nheapblocks is the new size of the heap.
295301
*/
296302
void
297303
visibilitymap_truncate(Relation rel, BlockNumber nheapblocks)
@@ -307,6 +313,8 @@ visibilitymap_truncate(Relation rel, BlockNumber nheapblocks)
307313
elog(DEBUG1, "vm_truncate %s %d", RelationGetRelationName(rel), nheapblocks);
308314
#endif
309315

316+
RelationOpenSmgr(rel);
317+
310318
/*
311319
* If no visibility map has been created yet for this relation, there's
312320
* nothing to truncate.
@@ -358,23 +366,25 @@ visibilitymap_truncate(Relation rel, BlockNumber nheapblocks)
358366
else
359367
newnblocks = truncBlock;
360368

361-
if (smgrnblocks(rel->rd_smgr, VISIBILITYMAP_FORKNUM) < newnblocks)
369+
if (smgrnblocks(rel->rd_smgr, VISIBILITYMAP_FORKNUM) <= newnblocks)
362370
{
363371
/* nothing to do, the file was already smaller than requested size */
364372
return;
365373
}
366374

375+
/* Truncate the unused VM pages, and send smgr inval message */
367376
smgrtruncate(rel->rd_smgr, VISIBILITYMAP_FORKNUM, newnblocks,
368377
rel->rd_istemp);
369378

370379
/*
371-
* Need to invalidate the relcache entry, because rd_vm_nblocks seen by
372-
* other backends is no longer valid.
380+
* We might as well update the local smgr_vm_nblocks setting.
381+
* smgrtruncate sent an smgr cache inval message, which will cause
382+
* other backends to invalidate their copy of smgr_vm_nblocks, and
383+
* this one too at the next command boundary. But this ensures it
384+
* isn't outright wrong until then.
373385
*/
374-
if (!InRecovery)
375-
CacheInvalidateRelcache(rel);
376-
377-
rel->rd_vm_nblocks = newnblocks;
386+
if (rel->rd_smgr)
387+
rel->rd_smgr->smgr_vm_nblocks = newnblocks;
378388
}
379389

380390
/*
@@ -391,21 +401,23 @@ vm_readbuf(Relation rel, BlockNumber blkno, bool extend)
391401
RelationOpenSmgr(rel);
392402

393403
/*
394-
* The current size of the visibility map fork is kept in relcache, to
395-
* avoid reading beyond EOF. If we haven't cached the size of the map yet,
396-
* do that first.
404+
* If we haven't cached the size of the visibility map fork yet, check it
405+
* first. Also recheck if the requested block seems to be past end, since
406+
* our cached value might be stale. (We send smgr inval messages on
407+
* truncation, but not on extension.)
397408
*/
398-
if (rel->rd_vm_nblocks == InvalidBlockNumber)
409+
if (rel->rd_smgr->smgr_vm_nblocks == InvalidBlockNumber ||
410+
blkno >= rel->rd_smgr->smgr_vm_nblocks)
399411
{
400412
if (smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM))
401-
rel->rd_vm_nblocks = smgrnblocks(rel->rd_smgr,
402-
VISIBILITYMAP_FORKNUM);
413+
rel->rd_smgr->smgr_vm_nblocks = smgrnblocks(rel->rd_smgr,
414+
VISIBILITYMAP_FORKNUM);
403415
else
404-
rel->rd_vm_nblocks = 0;
416+
rel->rd_smgr->smgr_vm_nblocks = 0;
405417
}
406418

407419
/* Handle requests beyond EOF */
408-
if (blkno >= rel->rd_vm_nblocks)
420+
if (blkno >= rel->rd_smgr->smgr_vm_nblocks)
409421
{
410422
if (extend)
411423
vm_extend(rel, blkno + 1);
@@ -446,19 +458,23 @@ vm_extend(Relation rel, BlockNumber vm_nblocks)
446458
* separate lock tag type for it.
447459
*
448460
* Note that another backend might have extended or created the relation
449-
* before we get the lock.
461+
* by the time we get the lock.
450462
*/
451463
LockRelationForExtension(rel, ExclusiveLock);
452464

453-
/* Create the file first if it doesn't exist */
454-
if ((rel->rd_vm_nblocks == 0 || rel->rd_vm_nblocks == InvalidBlockNumber)
455-
&& !smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM))
456-
{
465+
/* Might have to re-open if a cache flush happened */
466+
RelationOpenSmgr(rel);
467+
468+
/*
469+
* Create the file first if it doesn't exist. If smgr_vm_nblocks
470+
* is positive then it must exist, no need for an smgrexists call.
471+
*/
472+
if ((rel->rd_smgr->smgr_vm_nblocks == 0 ||
473+
rel->rd_smgr->smgr_vm_nblocks == InvalidBlockNumber) &&
474+
!smgrexists(rel->rd_smgr, VISIBILITYMAP_FORKNUM))
457475
smgrcreate(rel->rd_smgr, VISIBILITYMAP_FORKNUM, false);
458-
vm_nblocks_now = 0;
459-
}
460-
else
461-
vm_nblocks_now = smgrnblocks(rel->rd_smgr, VISIBILITYMAP_FORKNUM);
476+
477+
vm_nblocks_now = smgrnblocks(rel->rd_smgr, VISIBILITYMAP_FORKNUM);
462478

463479
while (vm_nblocks_now < vm_nblocks)
464480
{
@@ -467,12 +483,10 @@ vm_extend(Relation rel, BlockNumber vm_nblocks)
467483
vm_nblocks_now++;
468484
}
469485

486+
/* Update local cache with the up-to-date size */
487+
rel->rd_smgr->smgr_vm_nblocks = vm_nblocks_now;
488+
470489
UnlockRelationForExtension(rel, ExclusiveLock);
471490

472491
pfree(pg);
473-
474-
/* Update the relcache with the up-to-date size */
475-
if (!InRecovery)
476-
CacheInvalidateRelcache(rel);
477-
rel->rd_vm_nblocks = vm_nblocks_now;
478492
}

‎src/backend/access/transam/xlogutils.c

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
* Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
1212
* Portions Copyright (c) 1994, Regents of the University of California
1313
*
14-
* $PostgreSQL: pgsql/src/backend/access/transam/xlogutils.c,v 1.69 2010/01/02 16:57:35 momjian Exp $
14+
* $PostgreSQL: pgsql/src/backend/access/transam/xlogutils.c,v 1.70 2010/02/09 21:43:29 tgl Exp $
1515
*
1616
*-------------------------------------------------------------------------
1717
*/
@@ -393,9 +393,6 @@ CreateFakeRelcacheEntry(RelFileNode rnode)
393393
rel->rd_lockInfo.lockRelId.dbId = rnode.dbNode;
394394
rel->rd_lockInfo.lockRelId.relId = rnode.relNode;
395395

396-
rel->rd_targblock = InvalidBlockNumber;
397-
rel->rd_fsm_nblocks = InvalidBlockNumber;
398-
rel->rd_vm_nblocks = InvalidBlockNumber;
399396
rel->rd_smgr = NULL;
400397

401398
return rel;

‎src/backend/catalog/storage.c

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/catalog/storage.c,v 1.9 2010/02/08 19:59:49 heikki Exp $
11+
* $PostgreSQL: pgsql/src/backend/catalog/storage.c,v 1.10 2010/02/09 21:43:30 tgl Exp $
1212
*
1313
* NOTES
1414
* Some of this code used to be in storage/smgr/smgr.c, and the
@@ -226,8 +226,12 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
226226
/* Open it at the smgr level if not already done */
227227
RelationOpenSmgr(rel);
228228

229-
/* Make sure rd_targblock isn't pointing somewhere past end */
230-
rel->rd_targblock = InvalidBlockNumber;
229+
/*
230+
* Make sure smgr_targblock etc aren't pointing somewhere past new end
231+
*/
232+
rel->rd_smgr->smgr_targblock = InvalidBlockNumber;
233+
rel->rd_smgr->smgr_fsm_nblocks = InvalidBlockNumber;
234+
rel->rd_smgr->smgr_vm_nblocks = InvalidBlockNumber;
231235

232236
/* Truncate the FSM first if it exists */
233237
fsm = smgrexists(rel->rd_smgr, FSM_FORKNUM);
@@ -459,6 +463,7 @@ smgr_redo(XLogRecPtr lsn, XLogRecord *record)
459463
{
460464
xl_smgr_truncate *xlrec = (xl_smgr_truncate *) XLogRecGetData(record);
461465
SMgrRelation reln;
466+
Relation rel;
462467

463468
reln = smgropen(xlrec->rnode);
464469

@@ -475,14 +480,15 @@ smgr_redo(XLogRecPtr lsn, XLogRecord *record)
475480
/* Also tell xlogutils.c about it */
476481
XLogTruncateRelation(xlrec->rnode, MAIN_FORKNUM, xlrec->blkno);
477482

478-
/* Truncate FSM too */
479-
if (smgrexists(reln, FSM_FORKNUM))
480-
{
481-
Relation rel = CreateFakeRelcacheEntry(xlrec->rnode);
483+
/* Truncate FSM and VM too */
484+
rel = CreateFakeRelcacheEntry(xlrec->rnode);
482485

486+
if (smgrexists(reln, FSM_FORKNUM))
483487
FreeSpaceMapTruncateRel(rel, xlrec->blkno);
484-
FreeFakeRelcacheEntry(rel);
485-
}
488+
if (smgrexists(reln, VISIBILITYMAP_FORKNUM))
489+
visibilitymap_truncate(rel, xlrec->blkno);
490+
491+
FreeFakeRelcacheEntry(rel);
486492
}
487493
else
488494
elog(PANIC, "smgr_redo: unknown op code %u", info);

‎src/backend/commands/cluster.c

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
*
1212
*
1313
* IDENTIFICATION
14-
* $PostgreSQL: pgsql/src/backend/commands/cluster.c,v 1.199 2010/02/07 22:40:33 tgl Exp $
14+
* $PostgreSQL: pgsql/src/backend/commands/cluster.c,v 1.200 2010/02/09 21:43:30 tgl Exp $
1515
*
1616
*-------------------------------------------------------------------------
1717
*/
@@ -38,6 +38,7 @@
3838
#include "miscadmin.h"
3939
#include "storage/bufmgr.h"
4040
#include "storage/procarray.h"
41+
#include "storage/smgr.h"
4142
#include "utils/acl.h"
4243
#include "utils/fmgroids.h"
4344
#include "utils/inval.h"
@@ -559,16 +560,12 @@ mark_index_clustered(Relation rel, Oid indexOid)
559560
indexForm->indisclustered = false;
560561
simple_heap_update(pg_index, &indexTuple->t_self, indexTuple);
561562
CatalogUpdateIndexes(pg_index, indexTuple);
562-
/* Ensure we see the update in the index's relcache entry */
563-
CacheInvalidateRelcacheByRelid(thisIndexOid);
564563
}
565564
else if (thisIndexOid == indexOid)
566565
{
567566
indexForm->indisclustered = true;
568567
simple_heap_update(pg_index, &indexTuple->t_self, indexTuple);
569568
CatalogUpdateIndexes(pg_index, indexTuple);
570-
/* Ensure we see the update in the index's relcache entry */
571-
CacheInvalidateRelcacheByRelid(thisIndexOid);
572569
}
573570
heap_freetuple(indexTuple);
574571
}
@@ -819,8 +816,8 @@ copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
819816
XLogReportUnloggedStatement(reason);
820817
}
821818

822-
/* use_wal off requires rd_targblock be initially invalid */
823-
Assert(NewHeap->rd_targblock == InvalidBlockNumber);
819+
/* use_wal off requires smgr_targblock be initially invalid */
820+
Assert(RelationGetTargetBlock(NewHeap) == InvalidBlockNumber);
824821

825822
/*
826823
* If both tables have TOAST tables, perform toast swap by content. It is

‎src/backend/commands/sequence.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/commands/sequence.c,v 1.164 2010/01/02 16:57:37 momjian Exp $
11+
* $PostgreSQL: pgsql/src/backend/commands/sequence.c,v 1.165 2010/02/09 21:43:30 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -29,6 +29,7 @@
2929
#include "storage/bufmgr.h"
3030
#include "storage/lmgr.h"
3131
#include "storage/proc.h"
32+
#include "storage/smgr.h"
3233
#include "utils/acl.h"
3334
#include "utils/builtins.h"
3435
#include "utils/lsyscache.h"
@@ -220,7 +221,7 @@ DefineSequence(CreateSeqStmt *seq)
220221
sm->magic = SEQ_MAGIC;
221222

222223
/* hack: ensure heap_insert will insert on the just-created page */
223-
rel->rd_targblock = 0;
224+
RelationSetTargetBlock(rel, 0);
224225

225226
/* Now form & insert sequence tuple */
226227
tuple = heap_form_tuple(tupDesc, value, null);

‎src/backend/commands/vacuum.c

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
*
1515
*
1616
* IDENTIFICATION
17-
* $PostgreSQL: pgsql/src/backend/commands/vacuum.c,v 1.406 2010/02/08 16:50:21 tgl Exp $
17+
* $PostgreSQL: pgsql/src/backend/commands/vacuum.c,v 1.407 2010/02/09 21:43:30 tgl Exp $
1818
*
1919
*-------------------------------------------------------------------------
2020
*/
@@ -789,7 +789,6 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound,
789789
Oid save_userid;
790790
int save_sec_context;
791791
int save_nestlevel;
792-
bool heldoff;
793792

794793
if (scanned_all)
795794
*scanned_all = false;
@@ -970,10 +969,9 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound,
970969
cluster_rel(relid, InvalidOid, false,
971970
(vacstmt->options & VACOPT_VERBOSE) != 0,
972971
vacstmt->freeze_min_age, vacstmt->freeze_table_age);
973-
heldoff = false;
974972
}
975973
else
976-
heldoff = lazy_vacuum_rel(onerel, vacstmt, vac_strategy, scanned_all);
974+
lazy_vacuum_rel(onerel, vacstmt, vac_strategy, scanned_all);
977975

978976
/* Roll back any GUC changes executed by index functions */
979977
AtEOXact_GUC(false, save_nestlevel);
@@ -991,10 +989,6 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound,
991989
PopActiveSnapshot();
992990
CommitTransactionCommand();
993991

994-
/* now we can allow interrupts again, if disabled */
995-
if (heldoff)
996-
RESUME_INTERRUPTS();
997-
998992
/*
999993
* If the relation has a secondary toast rel, vacuum that too while we
1000994
* still hold the session lock on the master table. Note however that

0 commit comments

Comments
 (0)