From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"
Date: Mon, 16 Jul 2018 16:36:51 -0400
Message-ID: <4993.1531773411@sss.pgh.pa.us> (raw)
In-Reply-To: <20436.1531751966@sss.pgh.pa.us>
References: <20180627233939.GA10276@telsasoft.com>
<20180628020432.GB11054@paquier.xyz>
<7317.1531694923@sss.pgh.pa.us>
<20180716131337.usestwjviwwocpyz@alap3.anarazel.de>
<20436.1531751966@sss.pgh.pa.us>
I wrote:
> So I said I didn't want to do extra work on this, but I am looking into
> fixing it by having these aux process types run a ResourceOwner that can
> be told to clean up any open buffer pins at exit. We could be sure the
> coverage is complete by dint of removing the special-case code in
> resowner.c that allows buffer pins to be taken with no active resowner.
> Then CheckForBufferLeaks can be left as-is, ie something we do only
> in assert builds.
That turned out to be a larger can of worms than I'd anticipated, as it
soon emerged that we'd acquired a whole lot of cargo-cult programming
around ResourceOwners. Having a ResourceOwner in itself does nothing
for you: there has to be code someplace that ensures we'll call
ResourceOwnerRelease at an appropriate time. There was basically noplace
outside xact.c and portalmem.c that got this completely right. bgwriter.c
and a couple of other places at least tried a little, by doing a
ResourceOwnerRelease in their sigsetjmp-catching stanzas, but that didn't
account for the process-exit code path. Other places just created a
ResourceOwner and did *nothing* about cleaning it up.
I decided that the most expedient way of dealing with this was to create
a self-contained facility in resowner.c that would create a standalone
ResourceOwner and register a shmem-exit callback to clean it up.
That way it's easier (less code) to do it right than to do it wrong.
That led to the attached, which passes check-world as well as Michael's
full-disk test script.
I'm mostly pretty happy with this, but I think there are a couple of
loose ends in logicalfuncs.c and slotfuncs.c: those are creating
non-standalone ResourceOwners (children of whatever the active
ResourceOwner is) and doing nothing much to clean them up. That seems
pretty bogus. It's not a permanent resource leak, because sooner or
later the parent ResourceOwner will get cleaned up and that will
recurse to the child ... but then why bother with a child ResourceOwner
at all? I added asserts to these calls to verify that there was a
parent resowner (if there isn't, the code is just broken), but I think
that we should either add more code to clean up the child resowner
promptly, or just not bother with a child at all.
Not very sure what to do with this. I don't think we should try to
backpatch it, because it's essentially changing the API requirements
around buffer access in auxiliary processes --- but it might not be
too late to squeeze it into v11. It is a bug fix, in that the current
code can leak buffer pins in some code paths and we won't notice in
non-assert builds; but we've not really seen any field reports of that
happening, so I'm not sure how important this is.
regards, tom lane
Attachments:use-resowner-in-all-aux-processes-1.patch (text/x-diff, 16.2K) downloaddiff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.cindex 4e32cff..3a5cd0e 100644--- a/src/backend/access/transam/parallel.c+++ b/src/backend/access/transam/parallel.c@@ -1221,8 +1221,7 @@ ParallelWorkerMain(Datum main_arg)
memcpy(&ParallelWorkerNumber, MyBgworkerEntry->bgw_extra, sizeof(int));
/* Set up a memory context and resource owner. */
- Assert(CurrentResourceOwner == NULL);- CurrentResourceOwner = ResourceOwnerCreate(NULL, "parallel toplevel");+ CreateAuxProcessResourceOwner();
CurrentMemoryContext = AllocSetContextCreate(TopMemoryContext,
"Parallel worker",
ALLOCSET_DEFAULT_SIZES);
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.cindex 4049deb..5d01edd 100644--- a/src/backend/access/transam/xlog.c+++ b/src/backend/access/transam/xlog.c@@ -6356,6 +6356,15 @@ StartupXLOG(void)
struct stat st;
/*
+ * We should have an aux process resource owner to use, and we should not+ * be in a transaction that's installed some other resowner.+ */+ Assert(AuxProcessResourceOwner != NULL);+ Assert(CurrentResourceOwner == NULL ||+ CurrentResourceOwner == AuxProcessResourceOwner);+ CurrentResourceOwner = AuxProcessResourceOwner;++ /*
* Verify XLOG status looks valid.
*/
if (ControlFile->state < DB_SHUTDOWNED ||
@@ -8467,6 +8476,15 @@ GetNextXidAndEpoch(TransactionId *xid, uint32 *epoch)
void
ShutdownXLOG(int code, Datum arg)
{
+ /*+ * We should have an aux process resource owner to use, and we should not+ * be in a transaction that's installed some other resowner.+ */+ Assert(AuxProcessResourceOwner != NULL);+ Assert(CurrentResourceOwner == NULL ||+ CurrentResourceOwner == AuxProcessResourceOwner);+ CurrentResourceOwner = AuxProcessResourceOwner;+
/* Don't be chatty in standalone mode */
ereport(IsPostmasterEnvironment ? LOG : NOTICE,
(errmsg("shutting down")));
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.cindex 7e34bee..cdd71a9 100644--- a/src/backend/bootstrap/bootstrap.c+++ b/src/backend/bootstrap/bootstrap.c@@ -403,6 +403,13 @@ AuxiliaryProcessMain(int argc, char *argv[])
/* finish setting up bufmgr.c */
InitBufferPoolBackend();
+ /*+ * Auxiliary processes don't run transactions, but they may need a+ * resource owner anyway to manage buffer pins acquired outside+ * transactions (and, perhaps, other things in future).+ */+ CreateAuxProcessResourceOwner();+
/* Initialize backend status information */
pgstat_initialize();
pgstat_bestart();
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.cindex 78e4c85..1d9cfc6 100644--- a/src/backend/postmaster/autovacuum.c+++ b/src/backend/postmaster/autovacuum.c@@ -522,13 +522,9 @@ AutoVacLauncherMain(int argc, char *argv[])
pgstat_report_wait_end();
AbortBufferIO();
UnlockBuffers();
- if (CurrentResourceOwner)- {- ResourceOwnerRelease(CurrentResourceOwner,- RESOURCE_RELEASE_BEFORE_LOCKS,- false, true);- /* we needn't bother with the other ResourceOwnerRelease phases */- }+ /* this is probably dead code, but let's be safe: */+ if (AuxProcessResourceOwner)+ ReleaseAuxProcessResources(false);
AtEOXact_Buffers(false);
AtEOXact_SMgr();
AtEOXact_Files(false);
diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.cindex d5ce685..960d3de 100644--- a/src/backend/postmaster/bgwriter.c+++ b/src/backend/postmaster/bgwriter.c@@ -142,12 +142,6 @@ BackgroundWriterMain(void)
sigdelset(&BlockSig, SIGQUIT);
/*
- * Create a resource owner to keep track of our resources (currently only- * buffer pins).- */- CurrentResourceOwner = ResourceOwnerCreate(NULL, "Background Writer");-- /*
* We just started, assume there has been either a shutdown or
* end-of-recovery snapshot.
*/
@@ -191,11 +185,7 @@ BackgroundWriterMain(void)
ConditionVariableCancelSleep();
AbortBufferIO();
UnlockBuffers();
- /* buffer pins are released here: */- ResourceOwnerRelease(CurrentResourceOwner,- RESOURCE_RELEASE_BEFORE_LOCKS,- false, true);- /* we needn't bother with the other ResourceOwnerRelease phases */+ ReleaseAuxProcessResources(false);
AtEOXact_Buffers(false);
AtEOXact_SMgr();
AtEOXact_Files(false);
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.cindex 0950ada..de1b22d 100644--- a/src/backend/postmaster/checkpointer.c+++ b/src/backend/postmaster/checkpointer.c@@ -232,12 +232,6 @@ CheckpointerMain(void)
last_checkpoint_time = last_xlog_switch_time = (pg_time_t) time(NULL);
/*
- * Create a resource owner to keep track of our resources (currently only- * buffer pins).- */- CurrentResourceOwner = ResourceOwnerCreate(NULL, "Checkpointer");-- /*
* Create a memory context that we will do all our work in. We do this so
* that we can reset the context during error recovery and thereby avoid
* possible memory leaks. Formerly this code just ran in
@@ -275,11 +269,7 @@ CheckpointerMain(void)
pgstat_report_wait_end();
AbortBufferIO();
UnlockBuffers();
- /* buffer pins are released here: */- ResourceOwnerRelease(CurrentResourceOwner,- RESOURCE_RELEASE_BEFORE_LOCKS,- false, true);- /* we needn't bother with the other ResourceOwnerRelease phases */+ ReleaseAuxProcessResources(false);
AtEOXact_Buffers(false);
AtEOXact_SMgr();
AtEOXact_Files(false);
diff --git a/src/backend/postmaster/walwriter.c b/src/backend/postmaster/walwriter.cindex 688d5b5..eceed1b 100644--- a/src/backend/postmaster/walwriter.c+++ b/src/backend/postmaster/walwriter.c@@ -130,12 +130,6 @@ WalWriterMain(void)
sigdelset(&BlockSig, SIGQUIT);
/*
- * Create a resource owner to keep track of our resources (not clear that- * we need this, but may as well have one).- */- CurrentResourceOwner = ResourceOwnerCreate(NULL, "Wal Writer");-- /*
* Create a memory context that we will do all our work in. We do this so
* that we can reset the context during error recovery and thereby avoid
* possible memory leaks. Formerly this code just ran in
@@ -172,11 +166,7 @@ WalWriterMain(void)
pgstat_report_wait_end();
AbortBufferIO();
UnlockBuffers();
- /* buffer pins are released here: */- ResourceOwnerRelease(CurrentResourceOwner,- RESOURCE_RELEASE_BEFORE_LOCKS,- false, true);- /* we needn't bother with the other ResourceOwnerRelease phases */+ ReleaseAuxProcessResources(false);
AtEOXact_Buffers(false);
AtEOXact_SMgr();
AtEOXact_Files(false);
diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.cindex 54c25f1..6fac37b 100644--- a/src/backend/replication/logical/logicalfuncs.c+++ b/src/backend/replication/logical/logicalfuncs.c@@ -279,6 +279,10 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
*/
startptr = MyReplicationSlot->data.restart_lsn;
+ /*+ * Record our resource usage in a child of the current resowner.+ */+ Assert(CurrentResourceOwner != NULL);
CurrentResourceOwner = ResourceOwnerCreate(CurrentResourceOwner, "logical decoding");
/* invalidate non-timetravel entries */
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.cindex 0d2b795..62c7d66 100644--- a/src/backend/replication/logical/worker.c+++ b/src/backend/replication/logical/worker.c@@ -1602,9 +1602,8 @@ ApplyWorkerMain(Datum main_arg)
/* Load the libpq-specific functions */
load_file("libpqwalreceiver", false);
- Assert(CurrentResourceOwner == NULL);- CurrentResourceOwner = ResourceOwnerCreate(NULL,- "logical replication apply");+ /* Set up resource owner */+ CreateAuxProcessResourceOwner();
/* Run as replica session replication role. */
SetConfigOption("session_replication_role", "replica",
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.cindex 450f737..f2985ef 100644--- a/src/backend/replication/slotfuncs.c+++ b/src/backend/replication/slotfuncs.c@@ -365,6 +365,10 @@ pg_logical_replication_slot_advance(XLogRecPtr moveto)
logical_read_local_xlog_page,
NULL, NULL, NULL);
+ /*+ * Record our resource usage in a child of the current resowner.+ */+ Assert(CurrentResourceOwner != NULL);
CurrentResourceOwner = ResourceOwnerCreate(CurrentResourceOwner,
"logical decoding");
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.cindex 987bb84..7c292d8 100644--- a/src/backend/replication/walreceiver.c+++ b/src/backend/replication/walreceiver.c@@ -292,12 +292,6 @@ WalReceiverMain(void)
if (WalReceiverFunctions == NULL)
elog(ERROR, "libpqwalreceiver didn't initialize correctly");
- /*- * Create a resource owner to keep track of our resources (not clear that- * we need this, but may as well have one).- */- CurrentResourceOwner = ResourceOwnerCreate(NULL, "Wal Receiver");-
/* Unblock signals (they were blocked when the postmaster forked us) */
PG_SETMASK(&UnBlockSig);
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.cindex 3a0106b..8e9e47b 100644--- a/src/backend/replication/walsender.c+++ b/src/backend/replication/walsender.c@@ -265,7 +265,7 @@ InitWalSender(void)
InitWalSenderSlot();
/* Set up resource owner */
- CurrentResourceOwner = ResourceOwnerCreate(NULL, "walsender top-level resource owner");+ CreateAuxProcessResourceOwner();
/*
* Let postmaster know that we're a WAL sender. Once we've declared us as
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.cindex 09e0df2..5ef6315 100644--- a/src/backend/utils/init/postinit.c+++ b/src/backend/utils/init/postinit.c@@ -632,8 +632,19 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
* We are either a bootstrap process or a standalone backend. Either
* way, start up the XLOG machinery, and register to have it closed
* down at exit.
+ *+ * We don't yet have an aux-process resource owner, but StartupXLOG+ * and ShutdownXLOG will need one. Hence, create said resource owner+ * (and register a callback to clean it up after ShutdownXLOG runs).
*/
+ CreateAuxProcessResourceOwner();+
StartupXLOG();
+ /* Release (and warn about) any buffer pins leaked in StartupXLOG */+ ReleaseAuxProcessResources(true);+ /* Reset CurrentResourceOwner to nothing for the moment */+ CurrentResourceOwner = NULL;+
on_shmem_exit(ShutdownXLOG, 0);
}
diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.cindex bce021e..c708ebd 100644--- a/src/backend/utils/resowner/resowner.c+++ b/src/backend/utils/resowner/resowner.c@@ -22,6 +22,7 @@
#include "access/hash.h"
#include "jit/jit.h"
+#include "storage/ipc.h"
#include "storage/predicate.h"
#include "storage/proc.h"
#include "utils/memutils.h"
@@ -140,6 +141,7 @@ typedef struct ResourceOwnerData
ResourceOwner CurrentResourceOwner = NULL;
ResourceOwner CurTransactionResourceOwner = NULL;
ResourceOwner TopTransactionResourceOwner = NULL;
+ResourceOwner AuxProcessResourceOwner = NULL;
/*
* List of add-on callbacks for resource releasing
@@ -165,6 +167,7 @@ static void ResourceOwnerReleaseInternal(ResourceOwner owner,
ResourceReleasePhase phase,
bool isCommit,
bool isTopLevel);
+static void ReleaseAuxProcessResourcesCallback(int code, Datum arg);
static void PrintRelCacheLeakWarning(Relation rel);
static void PrintPlanCacheLeakWarning(CachedPlan *plan);
static void PrintTupleDescLeakWarning(TupleDesc tupdesc);
@@ -823,6 +826,60 @@ UnregisterResourceReleaseCallback(ResourceReleaseCallback callback, void *arg)
}
}
+/*+ * Establish an AuxProcessResourceOwner for the current process.+ */+void+CreateAuxProcessResourceOwner(void)+{+ Assert(AuxProcessResourceOwner == NULL);+ Assert(CurrentResourceOwner == NULL);+ AuxProcessResourceOwner = ResourceOwnerCreate(NULL, "AuxiliaryProcess");+ CurrentResourceOwner = AuxProcessResourceOwner;++ /*+ * Register a shmem-exit callback for cleanup of aux-process resource+ * owner. (This needs to run after, e.g., ShutdownXLOG.)+ */+ on_shmem_exit(ReleaseAuxProcessResourcesCallback, 0);++}++/*+ * Convenience routine to release all resources tracked in+ * AuxProcessResourceOwner (but that resowner is not destroyed here).+ * Warn about leaked resources if isCommit is true.+ */+void+ReleaseAuxProcessResources(bool isCommit)+{+ /*+ * At this writing, the only thing that could actually get released is+ * buffer pins; but we may as well do the full release protocol.+ */+ ResourceOwnerRelease(AuxProcessResourceOwner,+ RESOURCE_RELEASE_BEFORE_LOCKS,+ isCommit, true);+ ResourceOwnerRelease(AuxProcessResourceOwner,+ RESOURCE_RELEASE_LOCKS,+ isCommit, true);+ ResourceOwnerRelease(AuxProcessResourceOwner,+ RESOURCE_RELEASE_AFTER_LOCKS,+ isCommit, true);+}++/*+ * Shmem-exit callback for the same.+ * Warn about leaked resources if process exit code is zero (ie normal).+ */+static void+ReleaseAuxProcessResourcesCallback(int code, Datum arg)+{+ bool isCommit = (code == 0);++ ReleaseAuxProcessResources(isCommit);+}+
/*
* Make sure there is room for at least one more entry in a ResourceOwner's
@@ -830,15 +887,10 @@ UnregisterResourceReleaseCallback(ResourceReleaseCallback callback, void *arg)
*
* This is separate from actually inserting an entry because if we run out
* of memory, it's critical to do so *before* acquiring the resource.
- *- * We allow the case owner == NULL because the bufmgr is sometimes invoked- * outside any transaction (for example, during WAL recovery).
*/
void
ResourceOwnerEnlargeBuffers(ResourceOwner owner)
{
- if (owner == NULL)- return;
ResourceArrayEnlarge(&(owner->bufferarr));
}
@@ -846,29 +898,19 @@ ResourceOwnerEnlargeBuffers(ResourceOwner owner)
* Remember that a buffer pin is owned by a ResourceOwner
*
* Caller must have previously done ResourceOwnerEnlargeBuffers()
- *- * We allow the case owner == NULL because the bufmgr is sometimes invoked- * outside any transaction (for example, during WAL recovery).
*/
void
ResourceOwnerRememberBuffer(ResourceOwner owner, Buffer buffer)
{
- if (owner == NULL)- return;
ResourceArrayAdd(&(owner->bufferarr), BufferGetDatum(buffer));
}
/*
* Forget that a buffer pin is owned by a ResourceOwner
- *- * We allow the case owner == NULL because the bufmgr is sometimes invoked- * outside any transaction (for example, during WAL recovery).
*/
void
ResourceOwnerForgetBuffer(ResourceOwner owner, Buffer buffer)
{
- if (owner == NULL)- return;
if (!ResourceArrayRemove(&(owner->bufferarr), BufferGetDatum(buffer)))
elog(ERROR, "buffer %d is not owned by resource owner %s",
buffer, owner->name);
diff --git a/src/include/utils/resowner.h b/src/include/utils/resowner.hindex fe7f491..fa03942 100644--- a/src/include/utils/resowner.h+++ b/src/include/utils/resowner.h@@ -33,6 +33,7 @@ typedef struct ResourceOwnerData *ResourceOwner;
extern PGDLLIMPORT ResourceOwner CurrentResourceOwner;
extern PGDLLIMPORT ResourceOwner CurTransactionResourceOwner;
extern PGDLLIMPORT ResourceOwner TopTransactionResourceOwner;
+extern PGDLLIMPORT ResourceOwner AuxProcessResourceOwner;
/*
* Resource releasing is done in three phases: pre-locks, locks, and
@@ -78,5 +79,7 @@ extern void RegisterResourceReleaseCallback(ResourceReleaseCallback callback,
void *arg);
extern void UnregisterResourceReleaseCallback(ResourceReleaseCallback callback,
void *arg);
+extern void CreateAuxProcessResourceOwner(void);+extern void ReleaseAuxProcessResources(bool isCommit);
#endif /* RESOWNER_H */
diff --git a/src/test/modules/test_shm_mq/worker.c b/src/test/modules/test_shm_mq/worker.cindex bcb992e..e9bc30d 100644--- a/src/test/modules/test_shm_mq/worker.c+++ b/src/test/modules/test_shm_mq/worker.c@@ -75,7 +75,7 @@ test_shm_mq_main(Datum main_arg)
* of contents so we can locate the various data structures we'll need to
* find within the segment.
*/
- CurrentResourceOwner = ResourceOwnerCreate(NULL, "test_shm_mq worker");+ CreateAuxProcessResourceOwner();
seg = dsm_attach(DatumGetInt32(main_arg));
if (seg == NULL)
ereport(ERROR,
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, andres@anarazel.de, michael@paquier.xyz, pryzby@telsasoft.com
Subject: Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"
In-Reply-To: <4993.1531773411@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox