Skip to content

Commit 21d9c3e

Browse files
hlinnakahlinnaka
committed
Give SMgrRelation pointers a well-defined lifetime.
After calling smgropen(), it was not clear how long you could continue to use the result, because various code paths including cache invalidation could call smgrclose(), which freed the memory. Guarantee that the object won't be destroyed until the end of the current transaction, or in recovery, the commit/abort record that destroys the underlying storage. smgrclose() is now just an alias for smgrrelease(). It closes files and forgets all state except the rlocator, but keeps the SMgrRelation object valid. A new smgrdestroy() function is used by rare places that know there should be no other references to the SMgrRelation. The short version: * smgrclose() is now just an alias for smgrrelease(). It releases resources, but doesn't destroy until EOX * smgrdestroy() now frees memory, and should rarely be used. Existing code should be unaffected, but it is now possible for code that has an SMgrRelation object to use it repeatedly during a transaction as long as the storage hasn't been physically dropped. Such code would normally hold a lock on the relation. This also replaces the "ownership" mechanism of SMgrRelations with a pin counter. An SMgrRelation can now be "pinned", which prevents it from being destroyed at end of transaction. There can be multiple pins on the same SMgrRelation. In practice, the pin mechanism is only used by the relcache, so there cannot be more than one pin on the same SMgrRelation. Except with swap_relation_files XXX Author: Thomas Munro, Heikki Linnakangas Reviewed-by: Robert Haas <robertmhaas@gmail.com> Discussion: https://www.postgresql.org/message-id/CA%2BhUKGJ8NTvqLHz6dqbQnt2c8XCki4r2QvXjBQcXpVwxTY_pvA@mail.gmail.com
1 parent 6a8ffe8 commit 21d9c3e

File tree

12 files changed

+183
-227
lines changed

12 files changed

+183
-227
lines changed

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

+8-7
Original file line numberDiff line numberDiff line change
@@ -616,7 +616,11 @@ CreateFakeRelcacheEntry(RelFileLocator rlocator)
616616
rel->rd_lockInfo.lockRelId.dbId = rlocator.dbOid;
617617
rel->rd_lockInfo.lockRelId.relId = rlocator.relNumber;
618618

619-
rel->rd_smgr = NULL;
619+
/*
620+
* Set up a non-pinned SMgrRelation reference, so that we don't need to
621+
* worry about unpinning it on error.
622+
*/
623+
rel->rd_smgr = smgropen(rlocator, InvalidBackendId);
620624

621625
return rel;
622626
}
@@ -627,9 +631,6 @@ CreateFakeRelcacheEntry(RelFileLocator rlocator)
627631
void
628632
FreeFakeRelcacheEntry(Relation fakerel)
629633
{
630-
/* make sure the fakerel is not referenced by the SmgrRelation anymore */
631-
if (fakerel->rd_smgr != NULL)
632-
smgrclearowner(&fakerel->rd_smgr, fakerel->rd_smgr);
633634
pfree(fakerel);
634635
}
635636

@@ -656,10 +657,10 @@ XLogDropDatabase(Oid dbid)
656657
/*
657658
* This is unnecessarily heavy-handed, as it will close SMgrRelation
658659
* objects for other databases as well. DROP DATABASE occurs seldom enough
659-
* that it's not worth introducing a variant of smgrclose for just this
660-
* purpose. XXX: Or should we rather leave the smgr entries dangling?
660+
* that it's not worth introducing a variant of smgrdestroy for just this
661+
* purpose.
661662
*/
662-
smgrcloseall();
663+
smgrdestroyall();
663664

664665
forget_invalid_pages_db(dbid);
665666
}

‎src/backend/commands/cluster.c

-19
Original file line numberDiff line numberDiff line change
@@ -1422,25 +1422,6 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
14221422
heap_freetuple(reltup2);
14231423

14241424
table_close(relRelation, RowExclusiveLock);
1425-
1426-
/*
1427-
* Close both relcache entries' smgr links. We need this kluge because
1428-
* both links will be invalidated during upcoming CommandCounterIncrement.
1429-
* Whichever of the rels is the second to be cleared will have a dangling
1430-
* reference to the other's smgr entry. Rather than trying to avoid this
1431-
* by ordering operations just so, it's easiest to close the links first.
1432-
* (Fortunately, since one of the entries is local in our transaction,
1433-
* it's sufficient to clear out our own relcache this way; the problem
1434-
* cannot arise for other backends when they see our update on the
1435-
* non-transient relation.)
1436-
*
1437-
* Caution: the placement of this step interacts with the decision to
1438-
* handle toast rels by recursion. When we are trying to rebuild pg_class
1439-
* itself, the smgr close on pg_class must happen after all accesses in
1440-
* this function.
1441-
*/
1442-
RelationCloseSmgrByOid(r1);
1443-
RelationCloseSmgrByOid(r2);
14441425
}
14451426

14461427
/*

‎src/backend/postmaster/bgwriter.c

+5-3
Original file line numberDiff line numberDiff line change
@@ -239,10 +239,12 @@ BackgroundWriterMain(void)
239239
if (FirstCallSinceLastCheckpoint())
240240
{
241241
/*
242-
* After any checkpoint, close all smgr files. This is so we
243-
* won't hang onto smgr references to deleted files indefinitely.
242+
* After any checkpoint, free all smgr objects. Otherwise we
243+
* would never do so for dropped relations, as the bgwriter does
244+
* not process shared invalidation messages or call
245+
* AtEOXact_SMgr().
244246
*/
245-
smgrcloseall();
247+
smgrdestroyall();
246248
}
247249

248250
/*

‎src/backend/postmaster/checkpointer.c

+7-8
Original file line numberDiff line numberDiff line change
@@ -462,10 +462,12 @@ CheckpointerMain(void)
462462
ckpt_performed = CreateRestartPoint(flags);
463463

464464
/*
465-
* After any checkpoint, close all smgr files. This is so we
466-
* won't hang onto smgr references to deleted files indefinitely.
465+
* After any checkpoint, free all smgr objects. Otherwise we
466+
* would never do so for dropped relations, as the checkpointer
467+
* does not process shared invalidation messages or call
468+
* AtEOXact_SMgr().
467469
*/
468-
smgrcloseall();
470+
smgrdestroyall();
469471

470472
/*
471473
* Indicate checkpoint completion to any waiting backends.
@@ -951,11 +953,8 @@ RequestCheckpoint(int flags)
951953
*/
952954
CreateCheckPoint(flags | CHECKPOINT_IMMEDIATE);
953955

954-
/*
955-
* After any checkpoint, close all smgr files. This is so we won't
956-
* hang onto smgr references to deleted files indefinitely.
957-
*/
958-
smgrcloseall();
956+
/* Free all smgr objects, as CheckpointerMain() normally would. */
957+
smgrdestroyall();
959958

960959
return;
961960
}

‎src/backend/storage/buffer/bufmgr.c

+10-22
Original file line numberDiff line numberDiff line change
@@ -934,10 +934,6 @@ ExtendBufferedRelTo(BufferManagerRelation bmr,
934934
{
935935
LockRelationForExtension(bmr.rel, ExclusiveLock);
936936

937-
/* could have been closed while waiting for lock */
938-
if (bmr.rel)
939-
bmr.smgr = RelationGetSmgr(bmr.rel);
940-
941937
/* recheck, fork might have been created concurrently */
942938
if (!smgrexists(bmr.smgr, fork))
943939
smgrcreate(bmr.smgr, fork, flags & EB_PERFORMING_RECOVERY);
@@ -1897,11 +1893,7 @@ ExtendBufferedRelShared(BufferManagerRelation bmr,
18971893
* we get the lock.
18981894
*/
18991895
if (!(flags & EB_SKIP_EXTENSION_LOCK))
1900-
{
19011896
LockRelationForExtension(bmr.rel, ExclusiveLock);
1902-
if (bmr.rel)
1903-
bmr.smgr = RelationGetSmgr(bmr.rel);
1904-
}
19051897

19061898
/*
19071899
* If requested, invalidate size cache, so that smgrnblocks asks the
@@ -4155,6 +4147,7 @@ FlushRelationBuffers(Relation rel)
41554147
{
41564148
int i;
41574149
BufferDesc *bufHdr;
4150+
SMgrRelation srel = RelationGetSmgr(rel);
41584151

41594152
if (RelationUsesLocalBuffers(rel))
41604153
{
@@ -4183,7 +4176,7 @@ FlushRelationBuffers(Relation rel)
41834176

41844177
io_start = pgstat_prepare_io_time(track_io_timing);
41854178

4186-
smgrwrite(RelationGetSmgr(rel),
4179+
smgrwrite(srel,
41874180
BufTagGetForkNum(&bufHdr->tag),
41884181
bufHdr->tag.blockNum,
41894182
localpage,
@@ -4229,7 +4222,7 @@ FlushRelationBuffers(Relation rel)
42294222
{
42304223
PinBuffer_Locked(bufHdr);
42314224
LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_SHARED);
4232-
FlushBuffer(bufHdr, RelationGetSmgr(rel), IOOBJECT_RELATION, IOCONTEXT_NORMAL);
4225+
FlushBuffer(bufHdr, srel, IOOBJECT_RELATION, IOCONTEXT_NORMAL);
42334226
LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
42344227
UnpinBuffer(bufHdr);
42354228
}
@@ -4442,13 +4435,17 @@ void
44424435
CreateAndCopyRelationData(RelFileLocator src_rlocator,
44434436
RelFileLocator dst_rlocator, bool permanent)
44444437
{
4445-
RelFileLocatorBackend rlocator;
44464438
char relpersistence;
4439+
SMgrRelation src_rel;
4440+
SMgrRelation dst_rel;
44474441

44484442
/* Set the relpersistence. */
44494443
relpersistence = permanent ?
44504444
RELPERSISTENCE_PERMANENT : RELPERSISTENCE_UNLOGGED;
44514445

4446+
src_rel = smgropen(src_rlocator, InvalidBackendId);
4447+
dst_rel = smgropen(dst_rlocator, InvalidBackendId);
4448+
44524449
/*
44534450
* Create and copy all forks of the relation. During create database we
44544451
* have a separate cleanup mechanism which deletes complete database
@@ -4465,9 +4462,9 @@ CreateAndCopyRelationData(RelFileLocator src_rlocator,
44654462
for (ForkNumber forkNum = MAIN_FORKNUM + 1;
44664463
forkNum <= MAX_FORKNUM; forkNum++)
44674464
{
4468-
if (smgrexists(smgropen(src_rlocator, InvalidBackendId), forkNum))
4465+
if (smgrexists(src_rel, forkNum))
44694466
{
4470-
smgrcreate(smgropen(dst_rlocator, InvalidBackendId), forkNum, false);
4467+
smgrcreate(dst_rel, forkNum, false);
44714468

44724469
/*
44734470
* WAL log creation if the relation is persistent, or this is the
@@ -4481,15 +4478,6 @@ CreateAndCopyRelationData(RelFileLocator src_rlocator,
44814478
permanent);
44824479
}
44834480
}
4484-
4485-
/* close source and destination smgr if exists. */
4486-
rlocator.backend = InvalidBackendId;
4487-
4488-
rlocator.locator = src_rlocator;
4489-
smgrcloserellocator(rlocator);
4490-
4491-
rlocator.locator = dst_rlocator;
4492-
smgrcloserellocator(rlocator);
44934481
}
44944482

44954483
/* ---------------------------------------------------------------------

‎src/backend/storage/freespace/freespace.c

+1-8
Original file line numberDiff line numberDiff line change
@@ -532,14 +532,7 @@ fsm_readbuf(Relation rel, FSMAddress addr, bool extend)
532532
{
533533
BlockNumber blkno = fsm_logical_to_physical(addr);
534534
Buffer buf;
535-
SMgrRelation reln;
536-
537-
/*
538-
* Caution: re-using this smgr pointer could fail if the relcache entry
539-
* gets closed. It's safe as long as we only do smgr-level operations
540-
* between here and the last use of the pointer.
541-
*/
542-
reln = RelationGetSmgr(rel);
535+
SMgrRelation reln = RelationGetSmgr(rel);
543536

544537
/*
545538
* If we haven't cached the size of the FSM yet, check it first. Also

0 commit comments

Comments
 (0)