From 21998ff7585e20b53f8c6f3306920acf22bf9d2d Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Thu, 10 Aug 2023 18:16:31 +1200 Subject: [PATCH v5 02/10] Give SMgrRelation pointers a well-defined lifetime. XXX This is a stand-in version of this patch, see https://www.postgresql.org/message-id/CA%2BhUKGJ8NTvqLHz6dqbQnt2c8XCki4r2QvXjBQcXpVwxTY_pvA%40mail.gmail.com for later verisons being discussed separately. --- src/backend/access/transam/xlogutils.c | 2 +- src/backend/postmaster/bgwriter.c | 8 +++-- src/backend/postmaster/checkpointer.c | 15 ++++---- src/backend/storage/smgr/smgr.c | 49 ++++++++++++++++---------- src/include/storage/smgr.h | 4 +-- src/include/utils/rel.h | 6 ---- 6 files changed, 46 insertions(+), 38 deletions(-) diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c index aa8667abd1..8775b5789b 100644 --- a/src/backend/access/transam/xlogutils.c +++ b/src/backend/access/transam/xlogutils.c @@ -657,7 +657,7 @@ XLogDropDatabase(Oid dbid) * This is unnecessarily heavy-handed, as it will close SMgrRelation * objects for other databases as well. DROP DATABASE occurs seldom enough * that it's not worth introducing a variant of smgrclose for just this - * purpose. XXX: Or should we rather leave the smgr entries dangling? + * purpose. */ smgrcloseall(); diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c index d7d6cc0cd7..13e5376619 100644 --- a/src/backend/postmaster/bgwriter.c +++ b/src/backend/postmaster/bgwriter.c @@ -246,10 +246,12 @@ BackgroundWriterMain(void) if (FirstCallSinceLastCheckpoint()) { /* - * After any checkpoint, close all smgr files. This is so we - * won't hang onto smgr references to deleted files indefinitely. + * After any checkpoint, free all smgr objects. Otherwise we + * would never do so for dropped relations, as the bgwriter does + * not process shared invalidation messages or call + * AtEOXact_SMgr(). */ - smgrcloseall(); + smgrdestroyall(); } /* diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index 5e949fc885..5d843b6142 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -469,10 +469,12 @@ CheckpointerMain(void) ckpt_performed = CreateRestartPoint(flags); /* - * After any checkpoint, close all smgr files. This is so we - * won't hang onto smgr references to deleted files indefinitely. + * After any checkpoint, free all smgr objects. Otherwise we + * would never do so for dropped relations, as the checkpointer + * does not process shared invalidation messages or call + * AtEOXact_SMgr(). */ - smgrcloseall(); + smgrdestroyall(); /* * Indicate checkpoint completion to any waiting backends. @@ -958,11 +960,8 @@ RequestCheckpoint(int flags) */ CreateCheckPoint(flags | CHECKPOINT_IMMEDIATE); - /* - * After any checkpoint, close all smgr files. This is so we won't - * hang onto smgr references to deleted files indefinitely. - */ - smgrcloseall(); + /* Free all smgr objects, as CheckpointerMain() normally would. */ + smgrdestroyall(); return; } diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c index 563a0be5c7..0d7272e796 100644 --- a/src/backend/storage/smgr/smgr.c +++ b/src/backend/storage/smgr/smgr.c @@ -147,7 +147,9 @@ smgrshutdown(int code, Datum arg) /* * smgropen() -- Return an SMgrRelation object, creating it if need be. * - * This does not attempt to actually open the underlying file. + * This does not attempt to actually open the underlying files. The returned + * object remains valid at least until AtEOXact_SMgr() is called, or until + * smgrdestroy() is called in non-transaction backends. */ SMgrRelation smgropen(RelFileLocator rlocator, BackendId backend) @@ -259,10 +261,10 @@ smgrexists(SMgrRelation reln, ForkNumber forknum) } /* - * smgrclose() -- Close and delete an SMgrRelation object. + * smgrdestroy() -- Delete an SMgrRelation object. */ void -smgrclose(SMgrRelation reln) +smgrdestroy(SMgrRelation reln) { SMgrRelation *owner; ForkNumber forknum; @@ -289,12 +291,14 @@ smgrclose(SMgrRelation reln) } /* - * smgrrelease() -- Release all resources used by this object. + * smgrclose() -- Release all resources used by this object. * - * The object remains valid. + * The object remains valid, but is moved to the unknown list where it will + * be destroyed by AtEOXact_SMgr(). It may be re-owned if it is accessed by a + * relation before then. */ void -smgrrelease(SMgrRelation reln) +smgrclose(SMgrRelation reln) { for (ForkNumber forknum = 0; forknum <= MAX_FORKNUM; forknum++) { @@ -302,15 +306,20 @@ smgrrelease(SMgrRelation reln) reln->smgr_cached_nblocks[forknum] = InvalidBlockNumber; } reln->smgr_targblock = InvalidBlockNumber; + + if (reln->smgr_owner) + { + *reln->smgr_owner = NULL; + reln->smgr_owner = NULL; + dlist_push_tail(&unowned_relns, &reln->node); + } } /* - * smgrreleaseall() -- Release resources used by all objects. - * - * This is called for PROCSIGNAL_BARRIER_SMGRRELEASE. + * smgrcloseall() -- Close all objects. */ void -smgrreleaseall(void) +smgrcloseall(void) { HASH_SEQ_STATUS status; SMgrRelation reln; @@ -322,14 +331,17 @@ smgrreleaseall(void) hash_seq_init(&status, SMgrRelationHash); while ((reln = (SMgrRelation) hash_seq_search(&status)) != NULL) - smgrrelease(reln); + smgrclose(reln); } /* - * smgrcloseall() -- Close all existing SMgrRelation objects. + * smgrdestroyall() -- Destroy all SMgrRelation objects. + * + * It must be known that there are no pointers to SMgrRelations, other than + * those registered with smgrsetowner(). */ void -smgrcloseall(void) +smgrdestroyall(void) { HASH_SEQ_STATUS status; SMgrRelation reln; @@ -341,7 +353,7 @@ smgrcloseall(void) hash_seq_init(&status, SMgrRelationHash); while ((reln = (SMgrRelation) hash_seq_search(&status)) != NULL) - smgrclose(reln); + smgrdestroy(reln); } /* @@ -733,7 +745,8 @@ smgrimmedsync(SMgrRelation reln, ForkNumber forknum) * AtEOXact_SMgr * * This routine is called during transaction commit or abort (it doesn't - * particularly care which). All transient SMgrRelation objects are closed. + * particularly care which). All transient SMgrRelation objects are + * destroyed. * * We do this as a compromise between wanting transient SMgrRelations to * live awhile (to amortize the costs of blind writes of multiple blocks) @@ -747,7 +760,7 @@ AtEOXact_SMgr(void) dlist_mutable_iter iter; /* - * Zap all unowned SMgrRelations. We rely on smgrclose() to remove each + * Zap all unowned SMgrRelations. We rely on smgrdestroy() to remove each * one from the list. */ dlist_foreach_modify(iter, &unowned_relns) @@ -757,7 +770,7 @@ AtEOXact_SMgr(void) Assert(rel->smgr_owner == NULL); - smgrclose(rel); + smgrdestroy(rel); } } @@ -768,6 +781,6 @@ AtEOXact_SMgr(void) bool ProcessBarrierSmgrRelease(void) { - smgrreleaseall(); + smgrcloseall(); return true; } diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h index 527cd2a056..d8ffe397fa 100644 --- a/src/include/storage/smgr.h +++ b/src/include/storage/smgr.h @@ -85,8 +85,8 @@ extern void smgrclearowner(SMgrRelation *owner, SMgrRelation reln); extern void smgrclose(SMgrRelation reln); extern void smgrcloseall(void); extern void smgrcloserellocator(RelFileLocatorBackend rlocator); -extern void smgrrelease(SMgrRelation reln); -extern void smgrreleaseall(void); +extern void smgrdestroy(SMgrRelation reln); +extern void smgrdestroyall(void); extern void smgrcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo); extern void smgrdosyncall(SMgrRelation *rels, int nrels); extern void smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo); diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index a584b1ddff..6636cc82c0 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -561,12 +561,6 @@ typedef struct ViewOptions * * Very little code is authorized to touch rel->rd_smgr directly. Instead * use this function to fetch its value. - * - * Note: since a relcache flush can cause the file handle to be closed again, - * it's unwise to hold onto the pointer returned by this function for any - * long period. Recommended practice is to just re-execute RelationGetSmgr - * each time you need to access the SMgrRelation. It's quite cheap in - * comparison to whatever an smgr function is going to do. */ static inline SMgrRelation RelationGetSmgr(Relation rel) -- 2.39.2