postgres.git / summary / log / commit / refs

commit    d7f59347bc06cb26fe3eff283f9104ac1b26d177
Author:   Tom Lane <tgl@sss.pgh.pa.us>
Date:     Wed Nov 08 21:50:13 2017 +0000

    Fix two violations of the ResourceOwnerEnlarge/Remember protocol.
    
    The point of having separate ResourceOwnerEnlargeFoo and
    ResourceOwnerRememberFoo functions is so that resource allocation
    can happen in between.  Doing it in some other order is just wrong.
    
    OpenTemporaryFile() did open(), enlarge, remember, which would leak the
    open file if the enlarge step ran out of memory.  Because fd.c has its own
    layer of resource-remembering, the consequences look like they'd be limited
    to an intratransaction FD leak, but it's still not good.
    
    IncrBufferRefCount() did enlarge, remember, incr-refcount, which would blow
    up if the incr-refcount step ever failed.  It was safe enough when written,
    but since the introduction of PrivateRefCountHash, I think the assumption
    that no error could happen there is pretty shaky.
    
    The odds of real problems from either bug are probably small, but still,
    back-patch to supported branches.
    
    Thomas Munro and Tom Lane, per a comment from Andres Freund


 src/backend/storage/buffer/bufmgr.c |  2 +-
 src/backend/storage/file/fd.c       | 10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index d00a60fab76..ddd92e75a01 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -2999,7 +2999,6 @@ IncrBufferRefCount(Buffer buffer)
 {
 	Assert(BufferIsPinned(buffer));
 	ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
-	ResourceOwnerRememberBuffer(CurrentResourceOwner, buffer);
 	if (BufferIsLocal(buffer))
 		LocalRefCount[-buffer - 1]++;
 	else
@@ -3010,6 +3009,7 @@ IncrBufferRefCount(Buffer buffer)
 		Assert(ref != NULL);
 		ref->refcount++;
 	}
+	ResourceOwnerRememberBuffer(CurrentResourceOwner, buffer);
 }
 
 /*
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 33955270b42..bb81acbd1d0 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -1194,6 +1194,13 @@ OpenTemporaryFile(bool interXact)
 {
 	File		file = 0;
 
+	/*
+	 * Make sure the current resource owner has space for this File before we
+	 * open it, if we'll be registering it below.
+	 */
+	if (!interXact)
+		ResourceOwnerEnlargeFiles(CurrentResourceOwner);
+
 	/*
 	 * If some temp tablespace(s) have been given to us, try to use the next
 	 * one.  If a given tablespace can't be found, we silently fall back to
@@ -1230,9 +1237,8 @@ OpenTemporaryFile(bool interXact)
 	{
 		VfdCache[file].fdstate |= FD_XACT_TEMPORARY;
 
-		ResourceOwnerEnlargeFiles(CurrentResourceOwner);
-		ResourceOwnerRememberFile(CurrentResourceOwner, file);
 		VfdCache[file].resowner = CurrentResourceOwner;
+		ResourceOwnerRememberFile(CurrentResourceOwner, file);
 
 		/* ensure cleanup happens at eoxact */
 		have_xact_temporary_files = true;


[parent: 941602da1f61]