On Fri, Aug 18, 2023 at 2:30 AM Robert Haas <robertmh...@gmail.com> wrote:
> I think this direction makes a lot of sense. The lack of a defined
> lifetime for SMgrRelation objects makes correct programming difficult,
> makes efficient programming difficult, and doesn't really have any
> advantages.
Thanks for looking!
> I know this is just a WIP patch but for the final version
> I think it would make sense to try to do a bit more work on the
> comments. For instance:
>
> - In src/include/utils/rel.h, instead of just deleting that comment,
> how about documenting the new object lifetime? Or maybe that comment
> belongs elsewhere, but I think it would definitely good to spell it
> out very explicitly at some suitable place.
Right, let's one find one good place. I think smgropen() would be best.
> - When we change smgrcloseall() to smgrdestroyall(), maybe it's worth
> spelling out why destroying is needed and not just closing. For
> example, the second hunk in bgwriter.c includes a comment that says
> "After any checkpoint, close all smgr files. This is so we won't hang
> onto smgr references to deleted files indefinitely." But maybe it
> should say something like "After any checkpoint, close all smgr files
> and destroy the associated smgr objects. This guarantees that we close
> the actual file descriptors, that we close the File objects as managed
> by fd.c, and that we also destroy the smgr objects. We don't want any
> of these resources to stick around indefinitely after a relation file
> has been deleted."
There are several similar comments. I think they can be divided into
two categories:
1. The error-path ones, that we should now just delete along with the
code they describe, because the "various strange errors" should have
been fixed comprehensively by PROCSIGNAL_BARRIER_SMGRRELEASE. Here is
a separate patch to do that.
2. The per-checkpoint ones that still make sense to avoid unbounded
resource usage. Here is a new attempt at explaining:
/*
- * 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();
> - Maybe it's worth adding comments around some of the smgrclose[all]
> calls to mentioning that in those cases we want the file descriptors
> (and File objects?) to get closed but don't want to invalidate
> pointers. But I'm less sure that this is necessary. I don't want to
> have a million comments everywhere, just enough that someone looking
> at this stuff in the future can orient themselves about what's going
> on without too much difficulty.
I covered that with the following comment for smgrclose():
+ * 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.
From fa3beb19e6fafccb0d75d2e3e60d75412757b326 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Wed, 23 Aug 2023 14:38:38 +1200
Subject: [PATCH v3 1/2] Remove some obsolete smgrcloseall() calls.
Before the advent of PROCSIGNAL_BARRIER_SMGRRELEASE, we didn't have a
comprehensive way to deal with Windows file handles that get in the way
of unlinking directories. We had smgrcloseall() calls in a few places
to try to mitigate.
It's still a good idea for bgwriter and checkpointer to do that once per
checkpoint so they don't accumulate unbounded SMgrRelation objects, but
there is no longer any reason to close them at other random places such
as the error path, and the explanation as given in the comments is now
obsolete.
Discussion: https://postgr.es/m/message-id/CA%2BhUKGJ8NTvqLHz6dqbQnt2c8XCki4r2QvXjBQcXpVwxTY_pvA%40mail.gmail.com
---
src/backend/postmaster/bgwriter.c | 7 -------
src/backend/postmaster/checkpointer.c | 7 -------
src/backend/postmaster/walwriter.c | 7 -------
3 files changed, 21 deletions(-)
diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c
index caad642ec9..093cd034ea 100644
--- a/src/backend/postmaster/bgwriter.c
+++ b/src/backend/postmaster/bgwriter.c
@@ -197,13 +197,6 @@ BackgroundWriterMain(void)
*/
pg_usleep(1000000L);
- /*
- * Close all open files after any error. This is helpful on Windows,
- * where holding deleted files open causes various strange errors.
- * It's not clear we need it elsewhere, but shouldn't hurt.
- */
- smgrcloseall();
-
/* Report wait end here, when there is no further possibility of wait */
pgstat_report_wait_end();
}
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index ace9893d95..0e1c60ca72 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -310,13 +310,6 @@ CheckpointerMain(void)
* fast as we can.
*/
pg_usleep(1000000L);
-
- /*
- * Close all open files after any error. This is helpful on Windows,
- * where holding deleted files open causes various strange errors.
- * It's not clear we need it elsewhere, but shouldn't hurt.
- */
- smgrcloseall();
}
/* We can now handle ereport(ERROR) */
diff --git a/src/backend/postmaster/walwriter.c b/src/backend/postmaster/walwriter.c
index 266fbc2339..74526cf133 100644
--- a/src/backend/postmaster/walwriter.c
+++ b/src/backend/postmaster/walwriter.c
@@ -189,13 +189,6 @@ WalWriterMain(void)
* fast as we can.
*/
pg_usleep(1000000L);
-
- /*
- * Close all open files after any error. This is helpful on Windows,
- * where holding deleted files open causes various strange errors.
- * It's not clear we need it elsewhere, but shouldn't hurt.
- */
- smgrcloseall();
}
/* We can now handle ereport(ERROR) */
--
2.39.2
From 620f328d645742d94a352258d081dad1a55f495f 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 v3 2/2] 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>
Reviewed-by: Robert Haas <robertmh...@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKGJ8NTvqLHz6dqbQnt2c8XCki4r2QvXjBQcXpVwxTY_pvA%40mail.gmail.com
---
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 43f7b31205..bdc0f7e1da 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 093cd034ea..5eea8b7af0 100644
--- a/src/backend/postmaster/bgwriter.c
+++ b/src/backend/postmaster/bgwriter.c
@@ -238,10 +238,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 0e1c60ca72..0947ec62e2 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -451,10 +451,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.
@@ -937,11 +939,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 5d0f3d515c..d8e28c543e 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -144,7 +144,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-transactional backends.
*/
SMgrRelation
smgropen(RelFileLocator rlocator, BackendId backend)
@@ -254,10 +256,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,12 +286,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++)
{
@@ -297,15 +301,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;
@@ -317,14 +326,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;
@@ -336,7 +348,7 @@ smgrcloseall(void)
hash_seq_init(&status, SMgrRelationHash);
while ((reln = (SMgrRelation) hash_seq_search(&status)) != NULL)
- smgrclose(reln);
+ smgrdestroy(reln);
}
/*
@@ -727,7 +739,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)
@@ -741,7 +754,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)
@@ -751,7 +764,7 @@ AtEOXact_SMgr(void)
Assert(rel->smgr_owner == NULL);
- smgrclose(rel);
+ smgrdestroy(rel);
}
}
@@ -762,6 +775,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
