On Wed, Aug 16, 2023 at 4:11 AM Heikki Linnakangas <hlinn...@iki.fi> wrote: > Makes sense.
Thanks for looking! > If you change smgrclose() to do what smgrrelease() does now, then it > will apply automatically to extensions. > > If an extension is currently using smgropen()/smgrclose() correctly, > this patch alone won't make it wrong, so it's not very critical for > extensions to adopt the change. However, if after this we consider it OK > to hold a pointer to SMgrRelation for longer, and start doing that in > the backend, then extensions need to be adapted too. Yeah, that sounds quite compelling. Let's try that way: * smgrrelease() is removed * smgrclose() now releases resources, but doesn't destroy until EOX * smgrdestroy() now frees memory, and should rarely be used Still WIP while I think about edge cases, but so far I think this is the better option. > > While studying this I noticed a minor thinko in smgrrelease() in > > 15+16, so here's a fix for that also. I haven't figured out a > > sequence that makes anything bad happen, but we should really > > invalidate smgr_targblock when a relfilenode is reused, since it might > > point past the end. This becomes more obvious once smgrrelease() is > > used for truncation, as proposed here. > > +1. You can move the smgr_targblock clearing out of the loop, though. Right, thanks. Pushed.
From 71648a5b137540320fe782116e81f80c053c7f2c Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Thu, 10 Aug 2023 18:16:31 +1200 Subject: [PATCH v2] 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. The existing smgrclose() function now closes files and forgets all state except the rlocator, like smgrrelease() used to do, and additionally "disowns" the object so that it is disconnected from any Relation and queued for destruction at AtEOXact_SMgr(), unless it is re-owned by a Relation again before then. A new smgrdestroy() function is used by rare places that know there should be no other references to the SMgrRelation. The short version: * smgrrelease() is removed * smgrclose() now 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. Reviewed-by: Heikki Linnakangas <hlinn...@iki.fi> Discussion: https://postgr.es/m/CA%2BhUKGJ8NTvqLHz6dqbQnt2c8XCki4r2QvXjBQcXpVwxTY_pvA%40mail.gmail.com --- src/backend/access/transam/xlogutils.c | 2 +- src/backend/postmaster/bgwriter.c | 4 +-- src/backend/postmaster/checkpointer.c | 4 +-- src/backend/storage/smgr/md.c | 2 +- src/backend/storage/smgr/smgr.c | 46 ++++++++++++++++---------- src/include/storage/smgr.h | 4 +-- src/include/utils/rel.h | 6 ---- 7 files changed, 37 insertions(+), 31 deletions(-) diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c index e174a2a891..bed15dad0f 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 caad642ec9..a0ecc7f841 100644 --- a/src/backend/postmaster/bgwriter.c +++ b/src/backend/postmaster/bgwriter.c @@ -202,7 +202,7 @@ BackgroundWriterMain(void) * where holding deleted files open causes various strange errors. * It's not clear we need it elsewhere, but shouldn't hurt. */ - smgrcloseall(); + smgrdestroyall(); /* Report wait end here, when there is no further possibility of wait */ pgstat_report_wait_end(); @@ -248,7 +248,7 @@ BackgroundWriterMain(void) * After any checkpoint, close all smgr files. This is so we * won't hang onto smgr references to deleted files indefinitely. */ - smgrcloseall(); + smgrdestroyall(); } /* diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index ace9893d95..6daccaab78 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -316,7 +316,7 @@ CheckpointerMain(void) * where holding deleted files open causes various strange errors. * It's not clear we need it elsewhere, but shouldn't hurt. */ - smgrcloseall(); + smgrdestroyall(); } /* We can now handle ereport(ERROR) */ @@ -461,7 +461,7 @@ CheckpointerMain(void) * After any checkpoint, close all smgr files. This is so we * won't hang onto smgr references to deleted files indefinitely. */ - smgrcloseall(); + smgrdestroyall(); /* * Indicate checkpoint completion to any waiting backends. diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c index fdecbad170..c54cbccb4c 100644 --- a/src/backend/storage/smgr/md.c +++ b/src/backend/storage/smgr/md.c @@ -1272,7 +1272,7 @@ DropRelationFiles(RelFileLocator *delrels, int ndelrels, bool isRedo) smgrdounlinkall(srels, ndelrels, isRedo); for (i = 0; i < ndelrels; i++) - smgrclose(srels[i]); + smgrdestroy(srels[i]); pfree(srels); } diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c index f76c4605db..63ebcd3a39 100644 --- a/src/backend/storage/smgr/smgr.c +++ b/src/backend/storage/smgr/smgr.c @@ -254,10 +254,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; @@ -284,27 +284,35 @@ 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 destroy 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++) { smgrsw[reln->smgr_which].smgr_close(reln, forknum); 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; @@ -316,14 +324,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; @@ -335,7 +346,7 @@ smgrcloseall(void) hash_seq_init(&status, SMgrRelationHash); while ((reln = (SMgrRelation) hash_seq_search(&status)) != NULL) - smgrclose(reln); + smgrdestroy(reln); } /* @@ -726,7 +737,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) @@ -740,7 +752,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) @@ -750,7 +762,7 @@ AtEOXact_SMgr(void) Assert(rel->smgr_owner == NULL); - smgrclose(rel); + smgrdestroy(rel); } } @@ -761,6 +773,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 a9a179aaba..18dd8168df 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 1426a353cd..a1402c1dd2 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
