Обсуждение: ENOSPC FailedAssertion("!(RefCountErrors == 0)"
While grepping logs, I came across this crash, which I caused while adding many indices in a test environment. I don't know that there's any reason to believe one way or the other if this is specific to running on pg11b1. < 2018-06-17 11:38:45.465 CDT pryzbyj >FATAL: the database system is in recovery mode < 2018-06-17 11:38:45.466 CDT pryzbyj >FATAL: the database system is in recovery mode < 2018-06-17 11:38:45.468 CDT >FATAL: could not extend file "base/17379/38665798": No space left on device < 2018-06-17 11:38:45.468 CDT >HINT: Check free disk space. < 2018-06-17 11:38:45.468 CDT >CONTEXT: WAL redo at 2AA/239676B8 for Heap/INSERT+INIT: off 1 < 2018-06-17 11:38:45.468 CDT >WARNING: buffer refcount leak: [2366] (rel=base/17379/38665798, blockNum=1241, flags=0x8a000000,refcount=1 1) TRAP: FailedAssertion("!(RefCountErrors == 0)", File: "bufmgr.c", Line: 2523) Unfortunately, I have neither a core nor stacktrace to share, as the DB was (until after running out of space) on the same FS as /var :( Jun 17 11:38:42 localhost abrt[30526]: Aborting dump: only 30MiB is available on /var/spool/abrt Jun 17 11:38:45 localhost abrt[1261]: Aborting dump: only 16MiB is available on /var/spool/abrt Not sure how useful it's to crash test ENOSPC (although I see there's continuing patches for this case). Justin
On Wed, Jun 27, 2018 at 06:39:39PM -0500, Justin Pryzby wrote: > < 2018-06-17 11:38:45.465 CDT pryzbyj >FATAL: the database system is in recovery mode > < 2018-06-17 11:38:45.466 CDT pryzbyj >FATAL: the database system is in recovery mode > < 2018-06-17 11:38:45.468 CDT >FATAL: could not extend file "base/17379/38665798": No space left on device > < 2018-06-17 11:38:45.468 CDT >HINT: Check free disk space. > < 2018-06-17 11:38:45.468 CDT >CONTEXT: WAL redo at 2AA/239676B8 for Heap/INSERT+INIT: off 1 > < 2018-06-17 11:38:45.468 CDT >WARNING: buffer refcount leak: [2366] (rel=base/17379/38665798, blockNum=1241, flags=0x8a000000,refcount=1 1) > TRAP: FailedAssertion("!(RefCountErrors == 0)", File: "bufmgr.c", Line: 2523) > > Unfortunately, I have neither a core nor stacktrace to share, as the DB was > (until after running out of space) on the same FS as /var :( You may be interested in this thread, vintage 2017, where I also posted a test case, assuming that you can create an extra partition and make it run out of space: https://www.postgresql.org/message-id/19533.1497283908@sss.pgh.pa.us So that's a known problem. -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes: > On Wed, Jun 27, 2018 at 06:39:39PM -0500, Justin Pryzby wrote: >> < 2018-06-17 11:38:45.468 CDT >FATAL: could not extend file "base/17379/38665798": No space left on device >> < 2018-06-17 11:38:45.468 CDT >HINT: Check free disk space. >> < 2018-06-17 11:38:45.468 CDT >CONTEXT: WAL redo at 2AA/239676B8 for Heap/INSERT+INIT: off 1 >> < 2018-06-17 11:38:45.468 CDT >WARNING: buffer refcount leak: [2366] (rel=base/17379/38665798, blockNum=1241, flags=0x8a000000,refcount=1 1) >> TRAP: FailedAssertion("!(RefCountErrors == 0)", File: "bufmgr.c", Line: 2523) > You may be interested in this thread, vintage 2017, where I also posted > a test case, assuming that you can create an extra partition and make it > run out of space: > https://www.postgresql.org/message-id/19533.1497283908@sss.pgh.pa.us > So that's a known problem. I got around to poking into this today. It's easily reproducible with Michael's script, and capturing a stack trace from the assertion seems to be enough to explain the problem: #0 ExceptionalCondition (conditionName=0xa60137 "!(RefCountErrors == 0)", errorType=0x8e50f7 "FailedAssertion", fileName=0xa5ffa3 "bufmgr.c", lineNumber=2523) at assert.c:30 #1 0x000000000075829b in CheckForBufferLeaks () at bufmgr.c:2523 #2 0x00000000007582b3 in AtProcExit_Buffers (code=<value optimized out>, arg=<value optimized out>) at bufmgr.c:2476 #3 0x00000000007651f5 in shmem_exit (code=1) at ipc.c:272 #4 0x000000000076526e in proc_exit_prepare (code=1) at ipc.c:194 #5 0x00000000007652f8 in proc_exit (code=1) at ipc.c:107 #6 0x000000000089af5d in errfinish (dummy=<value optimized out>) at elog.c:543 #7 0x000000000078615b in mdextend (reln=<value optimized out>, forknum=MAIN_FORKNUM, blocknum=<value optimized out>, buffer=<value optimized out>, skipFsync=false) at md.c:549 #8 0x0000000000758bcf in ReadBuffer_common (smgr=<value optimized out>, relpersistence=112 'p', forkNum=MAIN_FORKNUM, blockNum=19603, mode=RBM_ZERO_AND_LOCK, strategy=0x0, hit=0x7fff70b892cf) at bufmgr.c:865 #9 0x0000000000759424 in ReadBufferWithoutRelcache (rnode=..., forkNum=MAIN_FORKNUM, blockNum=4294967295, mode=<value optimized out>, strategy=<value optimized out>) at bufmgr.c:692 #10 0x000000000052a09b in XLogReadBufferExtended (rnode=..., forknum=MAIN_FORKNUM, blkno=19603, mode=RBM_ZERO_AND_LOCK) at xlogutils.c:489 #11 0x000000000052a419 in XLogReadBufferForRedoExtended (record=0x13827e8, block_id=0 '\000', mode=RBM_ZERO_AND_LOCK, get_cleanup_lock=false, buf=0x7fff70b893ac) at xlogutils.c:390 #12 0x000000000052a649 in XLogInitBufferForRedo (record=<value optimized out>, block_id=<value optimized out>) at xlogutils.c:305 #13 0x00000000004bfa8a in heap_xlog_insert (record=0x13827e8) at heapam.c:8580 #14 0x00000000004c0b55 in heap_redo (record=0x13827e8) at heapam.c:9282 #15 0x000000000051fab0 in StartupXLOG () at xlog.c:7319 #16 0x0000000000710b71 in StartupProcessMain () at startup.c:217 #17 0x000000000052eeb5 in AuxiliaryProcessMain (argc=2, argv=0x7fff70b8c320) at bootstrap.c:441 #18 0x000000000070b6d7 in StartChildProcess (type=StartupProcess) at postmaster.c:5331 #19 0x000000000070fc25 in PostmasterMain (argc=3, argv=<value optimized out>) at postmaster.c:1371 So basically, WAL replay hits an error while holding a buffer pin, and nothing is done to release the buffer pin, but AtProcExit_Buffers thinks something should have been done. I'm not sure that it's worth trying to fix this "nicely". The startup process does not run a transaction and hence doesn't have any of the infrastructure that'd be needed to clean up in a nice way --- for instance, it has no CurrentResourceOwner. Perhaps in some distant future somebody will think it's worth trying to improve that, but I'm not interested in working on it right now, and even less interested in back-patching it. What seems like a better answer for now is to adjust AtProcExit_Buffers so that it doesn't cause an assertion failure in this case. I think we can define "this case" as being "failure exit from the startup process": if that happens, the postmaster is going to throw up its hands and force a panic shutdown anyway, so the failure to free a buffer pin shouldn't have any serious consequences. The attached one-liner patch does that, and is enough to get through Michael's test case without an assertion. This is just proof of concept though --- my inclination, if we go this route, is to make a slightly longer patch that would fix CheckForBufferLeaks to still print the WARNING messages if any, but not die with an assertion afterwards. Another question is whether this is really a sufficient band-aid. It looks to me like AtProcExit_Buffers will be called in any auxiliary process type, not only the startup process. Do, or should we, force a panic restart for nonzero-exit-code failures of all aux process types? If not, what are we going to do to clean up similar failures in other aux process types? BTW, this assertion has been there since fe548629; before that, there was code that would release any leaked buffer pins, relying on the PrivateRefCount data for that. So another idea is to restore some version of that capability, although I think we really really don't wanna scan the PrivateRefCount array if we can avoid it. Thoughts? regards, tom lane diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 01eabe5..d814229 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -2473,7 +2473,14 @@ AtProcExit_Buffers(int code, Datum arg) AbortBufferIO(); UnlockBuffers(); - CheckForBufferLeaks(); + /* + * The startup process might fail to release buffer pins if it fatals out. + * Since that will result in postmaster shutdown anyway, there's not + * really a reason to work hard to prevent it; but let's not raise an + * assertion failure when it happens. + */ + if (code == 0 || !AmStartupProcess()) + CheckForBufferLeaks(); /* localbuf.c needs a chance too */ AtProcExit_LocalBuffers();
Hi, On 2018-07-15 18:48:43 -0400, Tom Lane wrote: > So basically, WAL replay hits an error while holding a buffer pin, and > nothing is done to release the buffer pin, but AtProcExit_Buffers thinks > something should have been done. I think there's a few other cases where we hit this. I've seen something similar from inside checkpointer / BufferSync(). I'd be surprised if bgwriter couldn't be triggered into the same. > What seems like a better answer for now is to adjust AtProcExit_Buffers > so that it doesn't cause an assertion failure in this case. I think we > can define "this case" as being "failure exit from the startup process": > if that happens, the postmaster is going to throw up its hands and force > a panic shutdown anyway, so the failure to free a buffer pin shouldn't > have any serious consequences. > The attached one-liner patch does that, and is enough to get through > Michael's test case without an assertion. This is just proof of concept > though --- my inclination, if we go this route, is to make a slightly > longer patch that would fix CheckForBufferLeaks to still print the WARNING > messages if any, but not die with an assertion afterwards. > > Another question is whether this is really a sufficient band-aid. It > looks to me like AtProcExit_Buffers will be called in any auxiliary > process type, not only the startup process. We could just replace the Assert() with a PANIC? > Do, or should we, force a panic restart for nonzero-exit-code failures > of all aux process types? If not, what are we going to do to clean up > similar failures in other aux process types? I'm pretty sure that we do *not* force a panic on all nonzero-exit-code cases for other subprocesses. > BTW, this assertion has been there since fe548629; before that, there > was code that would release any leaked buffer pins, relying on the > PrivateRefCount data for that. So another idea is to restore some > version of that capability, although I think we really really don't > wanna scan the PrivateRefCount array if we can avoid it. I don't think scanning PrivateRefCount would be particularly problematic - in almost all cases it's going to be tiny. These day it's not NBuffers sized anymore, so I can't forsee any performance problems? I think we could invent something like like enter/leave "transactional mode" wherein we throw a PANIC when a buffer leaked. Processes that don't enter it - like startup, checkpointer, etc - would just WARN? Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2018-07-15 18:48:43 -0400, Tom Lane wrote: >> So basically, WAL replay hits an error while holding a buffer pin, and >> nothing is done to release the buffer pin, but AtProcExit_Buffers thinks >> something should have been done. > I think there's a few other cases where we hit this. I've seen something > similar from inside checkpointer / BufferSync(). I'd be surprised if > bgwriter couldn't be triggered into the same. Hm, yeah, on reflection it's pretty obvious that those are hazard cases. > I'm pretty sure that we do *not* force a panic on all nonzero-exit-code > cases for other subprocesses. That's my recollection as well -- mostly, we just start a new one. 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. regards, tom lane
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 diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index 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.c index 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.c index 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.c index 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.c index 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.c index 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.c index 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.c index 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.c index 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.c index 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.c index 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.c index 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.c index 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.c index 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.h index 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.c index 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,
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. > 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. ... > 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. Further investigation showed that the part of that code that was actually needed was not the creation of a child ResourceOwner, but rather restoration of the old CurrentResourceOwner setting after exiting the logical decoding loop. Apparently we can come out of that with the TopTransaction resowner being active. This still seems a bit bogus; maybe there should be a save-and-restore happening somewhere else? But I'm not really excited about doing more than commenting it. Also, most of the other random creations of ResourceOwners seem to just not be necessary at all, even with the new rule that you must have a resowner to acquire buffer pins. So the attached revised patch just gets rid of them, and improves some misleading/wrong comments on the topic. It'd still be easy to use CreateAuxProcessResourceOwner in any process where we discover we need one, but I don't see the value in adding useless overhead. At this point I'm leaning to just applying this in HEAD and calling it good. The potential for assertion failures isn't relevant to production builds, and my best guess at this point is that there isn't really any other user-visible bug. The resowners that were being created and not adequately cleaned up all seem to have been dead code anyway (ie they'd never acquire any resources). We could have a problem if, say, the bgwriter exited via the FATAL path while holding a pin, but I don't think there's a reason for it to do that except in a database-wide shutdown, where the consequences of a leaked pin seem pretty minimal. Any objections? Anyone want to do further review? regards, tom lane diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index 4e32cff..30ddf94 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -37,7 +37,6 @@ #include "utils/guc.h" #include "utils/inval.h" #include "utils/memutils.h" -#include "utils/resowner.h" #include "utils/snapmgr.h" #include "utils/typcache.h" @@ -1220,16 +1219,19 @@ ParallelWorkerMain(Datum main_arg) Assert(ParallelWorkerNumber == -1); memcpy(&ParallelWorkerNumber, MyBgworkerEntry->bgw_extra, sizeof(int)); - /* Set up a memory context and resource owner. */ - Assert(CurrentResourceOwner == NULL); - CurrentResourceOwner = ResourceOwnerCreate(NULL, "parallel toplevel"); + /* Set up a memory context to work in, just for cleanliness. */ CurrentMemoryContext = AllocSetContextCreate(TopMemoryContext, "Parallel worker", ALLOCSET_DEFAULT_SIZES); /* - * Now that we have a resource owner, we can attach to the dynamic shared - * memory segment and read the table of contents. + * Attach to the dynamic shared memory segment for the parallel query, and + * find its table of contents. + * + * Note: at this point, we have not created any ResourceOwner in this + * process. This will result in our DSM mapping surviving until process + * exit, which is fine. If there were a ResourceOwner, it would acquire + * ownership of the mapping, but we have no need for that. */ seg = dsm_attach(DatumGetUInt32(main_arg)); if (seg == NULL) @@ -1393,7 +1395,7 @@ ParallelWorkerMain(Datum main_arg) /* Must exit parallel mode to pop active snapshot. */ ExitParallelMode(); - /* Must pop active snapshot so resowner.c doesn't complain. */ + /* Must pop active snapshot so snapmgr.c doesn't complain. */ PopActiveSnapshot(); /* Shut down the parallel-worker transaction. */ diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 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.c index 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.c index 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.c index 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.c index 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.c index 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.c index 54c25f1..45aae71 100644 --- a/src/backend/replication/logical/logicalfuncs.c +++ b/src/backend/replication/logical/logicalfuncs.c @@ -279,8 +279,6 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin */ startptr = MyReplicationSlot->data.restart_lsn; - CurrentResourceOwner = ResourceOwnerCreate(CurrentResourceOwner, "logical decoding"); - /* invalidate non-timetravel entries */ InvalidateSystemCaches(); @@ -320,6 +318,11 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin tuplestore_donestoring(tupstore); + /* + * Logical decoding could have clobbered CurrentResourceOwner during + * transaction management, so restore the executor's value. (This is + * a kluge, but it's not worth cleaning up right now.) + */ CurrentResourceOwner = old_resowner; /* diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c index 0d2b795..6ca6cdc 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -1595,6 +1595,11 @@ ApplyWorkerMain(Datum main_arg) pqsignal(SIGTERM, die); BackgroundWorkerUnblockSignals(); + /* + * We don't currently need any ResourceOwner in a walreceiver process, but + * if we did, we could call CreateAuxProcessResourceOwner here. + */ + /* Initialise stats to a sanish value */ MyLogicalRepWorker->last_send_time = MyLogicalRepWorker->last_recv_time = MyLogicalRepWorker->reply_time = GetCurrentTimestamp(); @@ -1602,10 +1607,6 @@ ApplyWorkerMain(Datum main_arg) /* Load the libpq-specific functions */ load_file("libpqwalreceiver", false); - Assert(CurrentResourceOwner == NULL); - CurrentResourceOwner = ResourceOwnerCreate(NULL, - "logical replication apply"); - /* Run as replica session replication role. */ SetConfigOption("session_replication_role", "replica", PGC_SUSET, PGC_S_OVERRIDE); diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c index 450f737..6d7474a 100644 --- a/src/backend/replication/slotfuncs.c +++ b/src/backend/replication/slotfuncs.c @@ -365,9 +365,6 @@ pg_logical_replication_slot_advance(XLogRecPtr moveto) logical_read_local_xlog_page, NULL, NULL, NULL); - CurrentResourceOwner = ResourceOwnerCreate(CurrentResourceOwner, - "logical decoding"); - /* invalidate non-timetravel entries */ InvalidateSystemCaches(); @@ -402,6 +399,11 @@ pg_logical_replication_slot_advance(XLogRecPtr moveto) CHECK_FOR_INTERRUPTS(); } + /* + * Logical decoding could have clobbered CurrentResourceOwner during + * transaction management, so restore the executor's value. (This is + * a kluge, but it's not worth cleaning up right now.) + */ CurrentResourceOwner = old_resowner; if (ctx->reader->EndRecPtr != InvalidXLogRecPtr) diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c index 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.c index 3a0106b..28f2435 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -90,7 +90,6 @@ #include "utils/pg_lsn.h" #include "utils/portal.h" #include "utils/ps_status.h" -#include "utils/resowner.h" #include "utils/timeout.h" #include "utils/timestamp.h" @@ -264,8 +263,10 @@ InitWalSender(void) /* Create a per-walsender data structure in shared memory */ InitWalSenderSlot(); - /* Set up resource owner */ - CurrentResourceOwner = ResourceOwnerCreate(NULL, "walsender top-level resource owner"); + /* + * We don't currently need any ResourceOwner in a walsender process, but + * if we did, we could call CreateAuxProcessResourceOwner here. + */ /* * 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.c index 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.c index 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.h index 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.c index bcb992e..4e23419 100644 --- a/src/test/modules/test_shm_mq/worker.c +++ b/src/test/modules/test_shm_mq/worker.c @@ -24,7 +24,6 @@ #include "storage/procarray.h" #include "storage/shm_mq.h" #include "storage/shm_toc.h" -#include "utils/resowner.h" #include "test_shm_mq.h" @@ -69,13 +68,16 @@ test_shm_mq_main(Datum main_arg) * Connect to the dynamic shared memory segment. * * The backend that registered this worker passed us the ID of a shared - * memory segment to which we must attach for further instructions. In - * order to attach to dynamic shared memory, we need a resource owner. - * Once we've mapped the segment in our address space, attach to the table - * of contents so we can locate the various data structures we'll need to + * memory segment to which we must attach for further instructions. Once + * we've mapped the segment in our address space, attach to the table of + * contents so we can locate the various data structures we'll need to * find within the segment. + * + * Note: at this point, we have not created any ResourceOwner in this + * process. This will result in our DSM mapping surviving until process + * exit, which is fine. If there were a ResourceOwner, it would acquire + * ownership of the mapping, but we have no need for that. */ - CurrentResourceOwner = ResourceOwnerCreate(NULL, "test_shm_mq worker"); seg = dsm_attach(DatumGetInt32(main_arg)); if (seg == NULL) ereport(ERROR, @@ -133,10 +135,8 @@ test_shm_mq_main(Datum main_arg) copy_messages(inqh, outqh); /* - * We're done. Explicitly detach the shared memory segment so that we - * don't get a resource leak warning at commit time. This will fire any - * on_dsm_detach callbacks we've registered, as well. Once that's done, - * we can go ahead and exit. + * We're done. For cleanliness, explicitly detach from the shared memory + * segment (that would happen anyway during process exit, though). */ dsm_detach(seg); proc_exit(1);
On Tue, Jul 17, 2018 at 8:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Any objections? Anyone want to do further review? LGTM. I think this is an improvement. However, it seems like it might be a good idea for ResourceOwnerRememberBuffer and ResourceOwnerForgetBuffer to Assert(buffer != NULL), so that if somebody messes up it will trip an assertion rather than just seg faulting. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Tue, Jul 17, 2018 at 8:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Any objections? Anyone want to do further review? > LGTM. I think this is an improvement. However, it seems like it > might be a good idea for ResourceOwnerRememberBuffer and > ResourceOwnerForgetBuffer to Assert(buffer != NULL), so that if > somebody messes up it will trip an assertion rather than just seg > faulting. Uh, what? There are only a few callers of those, and they'd all have crashed already if they were somehow dealing with an invalid buffer. regards, tom lane
On Tue, Jul 17, 2018 at 8:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Jul 17, 2018 at 8:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Any objections? Anyone want to do further review? > >> LGTM. I think this is an improvement. However, it seems like it >> might be a good idea for ResourceOwnerRememberBuffer and >> ResourceOwnerForgetBuffer to Assert(buffer != NULL), so that if >> somebody messes up it will trip an assertion rather than just seg >> faulting. > > Uh, what? There are only a few callers of those, and they'd all have > crashed already if they were somehow dealing with an invalid buffer. Sorry, I meant Assert(owner != NULL). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Tue, Jul 17, 2018 at 8:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Uh, what? There are only a few callers of those, and they'd all have >> crashed already if they were somehow dealing with an invalid buffer. > Sorry, I meant Assert(owner != NULL). Oh, gotcha: so that if an external developer hits it, he can more easily see that this is from an (effective) API change and not some mysterious bug. Makes sense, especially if we include a comment: /* We used to allow pinning buffers without a resowner, but no more */ Assert(CurrentResourceOwner != NULL); I think it's sufficient to do this in ResourceOwnerEnlargeBuffers, though. The other two should be unreachable without having gone through that. regards, tom lane
Hello. I confirmed that this patch fixes the crash. At Tue, 17 Jul 2018 20:01:05 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <14892.1531872065@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. > > > 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. ... > > 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. > > Further investigation showed that the part of that code that was > actually needed was not the creation of a child ResourceOwner, but rather > restoration of the old CurrentResourceOwner setting after exiting the > logical decoding loop. Apparently we can come out of that with the > TopTransaction resowner being active. This still seems a bit bogus; > maybe there should be a save-and-restore happening somewhere else? > But I'm not really excited about doing more than commenting it. CurrentResourceOwner doesn't seem to be changed. It is just saved during snapshot export and used just as a flag only for checking for duplicate snapshot exporting. > Also, most of the other random creations of ResourceOwners seem to just > not be necessary at all, even with the new rule that you must have a > resowner to acquire buffer pins. So the attached revised patch just > gets rid of them, and improves some misleading/wrong comments on the > topic. It'd still be easy to use CreateAuxProcessResourceOwner in any > process where we discover we need one, but I don't see the value in adding > useless overhead. +1 for unifying to resowner for auxiliary processes. I found the comment below just before ending cleanup of auxiliary process main funcs. | * These operations are really just a minimal subset of | * AbortTransaction(). We don't have very many resources to worry | * about in checkpointer, but we do have LWLocks, buffers, and temp | * files. So couldn't we use TopTransactionResourceOwner instead of AuxProcessResrouceOwner? I feel a bit uneasy that bootstrap and standalone-backend have *AuxProcess*ResourceOwner. It's not about this ptch, but while looking this closer, I found the following comment on ShutdownXLOG(). | /* | * This must be called ONCE during postmaster or standalone-backend shutdown | */ Is the "postmaster" typo of "bootstrap process"? It is also called from checkpointer when non-standlone mode. > At this point I'm leaning to just applying this in HEAD and calling it > good. The potential for assertion failures isn't relevant to production > builds, and my best guess at this point is that there isn't really any > other user-visible bug. The resowners that were being created and not > adequately cleaned up all seem to have been dead code anyway (ie they'd > never acquire any resources). Agreed. > We could have a problem if, say, the > bgwriter exited via the FATAL path while holding a pin, but I don't think > there's a reason for it to do that except in a database-wide shutdown, > where the consequences of a leaked pin seem pretty minimal. > > Any objections? Anyone want to do further review? FWIW I think we won't be concerned about leaked pins after FATAL. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes: > Hello. I confirmed that this patch fixes the crash. Thanks for double-checking. > At Tue, 17 Jul 2018 20:01:05 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <14892.1531872065@sss.pgh.pa.us> >> Further investigation showed that the part of that code that was >> actually needed was not the creation of a child ResourceOwner, but rather >> restoration of the old CurrentResourceOwner setting after exiting the >> logical decoding loop. Apparently we can come out of that with the >> TopTransaction resowner being active. > CurrentResourceOwner doesn't seem to be changed. It is just saved > during snapshot export and used just as a flag only for checking > for duplicate snapshot exporting. I tried to remove "CurrentResourceOwner = old_resowner" from logicalfuncs.c, and got a failure in the contrib/test_decoding test. I investigated the reason and found that it was because CurrentResourceOwner was equal to TopTransactionResourceOwner when we come out of the decoding loop. If we don't restore it then subsequent executor activity gets logged in the wrong resowner. It is true that the one in slotfuncs.c seems to be unnecessary, but I thought it best to leave it there; it's cheap, and maybe there's a problem in cases not exercised in the regression tests. > So couldn't we use TopTransactionResourceOwner instead of > AuxProcessResrouceOwner? I feel a bit uneasy that bootstrap and > standalone-backend have *AuxProcess*ResourceOwner. Since the aux processes aren't running transactions, I didn't think that TopTransactionResourceOwner was appropriate. There's also a problem for bootstrap and standalone backend cases: those do run transactions and therefore create/destroy TopTransactionResourceOwner, leaving nothing behind for ShutdownXLOG to use if it tries to use that. We need an extra resowner somewhere. It's true that if you adopt a narrow definition of "aux process" as being "one that goes through AuxiliaryProcessMain", calling this extra resowner AuxProcessResourceOwner is a bit of a misnomer. I couldn't think of something better to call it, though. With a slightly wider understanding of "auxiliary process" as being anything that's not the postmaster or a client-session backend, it's fine. > It's not about this ptch, but while looking this closer, I found > the following comment on ShutdownXLOG(). > | /* > | * This must be called ONCE during postmaster or standalone-backend shutdown > | */ > Is the "postmaster" typo of "bootstrap process"? It is also > called from checkpointer when non-standlone mode. Well, it's actually executed by the checkpointer these days, but that's an implementation detail. I think this comment is fine: at some point while the postmaster is shutting down, this should be done, and done just once. regards, tom lane
On Wed, Jul 18, 2018 at 10:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> So couldn't we use TopTransactionResourceOwner instead of >> AuxProcessResrouceOwner? I feel a bit uneasy that bootstrap and >> standalone-backend have *AuxProcess*ResourceOwner. > > Since the aux processes aren't running transactions, I didn't think > that TopTransactionResourceOwner was appropriate. There's also > a problem for bootstrap and standalone backend cases: those do run > transactions and therefore create/destroy TopTransactionResourceOwner, > leaving nothing behind for ShutdownXLOG to use if it tries to use > that. We need an extra resowner somewhere. FallbackResourceOwner? DefaultResourceOwner? SessionResourceOwner? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Wed, Jul 18, 2018 at 10:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> So couldn't we use TopTransactionResourceOwner instead of >>> AuxProcessResrouceOwner? I feel a bit uneasy that bootstrap and >>> standalone-backend have *AuxProcess*ResourceOwner. >> Since the aux processes aren't running transactions, I didn't think >> that TopTransactionResourceOwner was appropriate. There's also >> a problem for bootstrap and standalone backend cases: those do run >> transactions and therefore create/destroy TopTransactionResourceOwner, >> leaving nothing behind for ShutdownXLOG to use if it tries to use >> that. We need an extra resowner somewhere. > FallbackResourceOwner? DefaultResourceOwner? SessionResourceOwner? Those names all suggest (to me anyway) that this resowner exists in all, or at least most, processes. That's not the situation as of this patch, although I could imagine an alternate universe where it's true; for example, if we decided there were a reason for normal backends to have a session-lifespan resowner. But even then, it might be better to distinguish that from aux processes' use of resowners. (I'm not really convinced that it'd be a good idea for normal backends to have a session-lifespan resowner; that could mask bugs involving trying to acquire resources outside a transaction.) regards, tom lane