Обсуждение: ENOSPC FailedAssertion("!(RefCountErrors == 0)"

Поиск
Список
Период
Сортировка

ENOSPC FailedAssertion("!(RefCountErrors == 0)"

От
Justin Pryzby
Дата:
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


Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"

От
Michael Paquier
Дата:
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

Вложения

Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"

От
Tom Lane
Дата:
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();

Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"

От
Andres Freund
Дата:
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


Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"

От
Tom Lane
Дата:
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


Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"

От
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,

Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"

От
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.

> 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);

Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"

От
Robert Haas
Дата:
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


Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"

От
Tom Lane
Дата:
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


Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"

От
Robert Haas
Дата:
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


Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"

От
Tom Lane
Дата:
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


Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"

От
Kyotaro HORIGUCHI
Дата:
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



Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"

От
Tom Lane
Дата:
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


Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"

От
Robert Haas
Дата:
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


Re: ENOSPC FailedAssertion("!(RefCountErrors == 0)"

От
Tom Lane
Дата:
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