From 4a6529a73cae4338d97dcc7e9923bce3411cef31 Mon Sep 17 00:00:00 2001
From: Rishu Bagga <bagrishu@amazon.com>
Date: Wed, 14 Dec 2022 23:18:17 +0000
Subject: [PATCH 2/4] slru_to_buf_crit_section_fix

fix remaining critical section issues by calling ResourceOwnerEnlargeBuffers and pre allocating space for clog in smgr hash table
---
 src/backend/access/transam/clog.c      |  13 +++
 src/backend/access/transam/multixact.c | 131 ++++++++++++++++++-------
 src/backend/access/transam/twophase.c  |   6 ++
 src/backend/access/transam/xact.c      |  22 +++++
 src/backend/storage/buffer/bufmgr.c    |   3 +-
 src/backend/utils/mmgr/mcxt.c          |   6 +-
 src/include/access/clog.h              |   1 +
 src/include/utils/memutils.h           |   1 +
 8 files changed, 141 insertions(+), 42 deletions(-)

diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index c74d1af62af..ec226ffd7e8 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -896,6 +896,19 @@ WriteTruncateXlogRec(int pageno, TransactionId oldestXact, Oid oldestXactDb)
 	XLogFlush(recptr);
 }
 
+
+/*
+ * verify that the clog relfilelocator
+ * has an entry in the storage manager hash table
+ */
+bool VerifyClogLocatorInHashTable(void)
+{
+
+       RelFileLocator rlocator = NrelRelFileLocator(NREL_CLOG_REL_ID);
+       return !((smgropen(rlocator, InvalidBackendId) == NULL));
+}
+
+
 /*
  * CLOG resource manager's routines
  */
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index b9b6a1ca774..9d7b2e1ce94 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -332,8 +332,8 @@ static MemoryContext MXactContext = NULL;
 /* internal MultiXactId management */
 static void MultiXactIdSetOldestVisible(void);
 static void RecordNewMultiXact(MultiXactId multi, MultiXactOffset offset,
-							   int nmembers, MultiXactMember *members);
-static MultiXactId GetNewMultiXactId(int nmembers, MultiXactOffset *offset);
+							   int nmembers, MultiXactMember *members, Buffer * offset_buf_ptr, Buffer * member_bufs);
+static MultiXactId GetNewMultiXactId(int nmembers, MultiXactOffset *offset, Buffer * offset_buf, Buffer ** member_bufs);
 
 /* MultiXact cache management */
 static int	mxactMemberComparator(const void *arg1, const void *arg2);
@@ -350,8 +350,8 @@ static Buffer ZeroMultiXactMemberPage(int pageno, bool writeXlog);
 static bool MultiXactOffsetPagePrecedes(int page1, int page2);
 static bool MultiXactOffsetPrecedes(MultiXactOffset offset1,
 									MultiXactOffset offset2);
-static void ExtendMultiXactOffset(MultiXactId multi);
-static void ExtendMultiXactMember(MultiXactOffset offset, int nmembers);
+static void ExtendMultiXactOffset(MultiXactId multi, Buffer * buffer);
+static void ExtendMultiXactMember(MultiXactOffset offset, int nmembers, Buffer ** member_buffers);
 static bool MultiXactOffsetWouldWrap(MultiXactOffset boundary,
 									 MultiXactOffset start, uint32 distance);
 static bool SetOffsetVacuumLimit(bool is_startup);
@@ -757,6 +757,9 @@ ReadMultiXactIdRange(MultiXactId *oldest, MultiXactId *next)
 MultiXactId
 MultiXactIdCreateFromMembers(int nmembers, MultiXactMember *members)
 {
+	Buffer * member_bufs;
+	Buffer offset_buff;
+
 	MultiXactId multi;
 	MultiXactOffset offset;
 	xl_multixact_create xlrec;
@@ -810,7 +813,8 @@ MultiXactIdCreateFromMembers(int nmembers, MultiXactMember *members)
 	 * in vacuum.  During vacuum, in particular, it would be unacceptable to
 	 * keep OldestMulti set, in case it runs for long.
 	 */
-	multi = GetNewMultiXactId(nmembers, &offset);
+
+	multi = GetNewMultiXactId(nmembers, &offset, &offset_buff, &member_bufs);
 
 	/* Make an XLOG entry describing the new MXID. */
 	xlrec.mid = multi;
@@ -830,8 +834,8 @@ MultiXactIdCreateFromMembers(int nmembers, MultiXactMember *members)
 	(void) XLogInsert(RM_MULTIXACT_ID, XLOG_MULTIXACT_CREATE_ID);
 
 	/* Now enter the information into the OFFSETs and MEMBERs logs */
-	RecordNewMultiXact(multi, offset, nmembers, members);
-
+	RecordNewMultiXact(multi, offset, nmembers, members, &offset_buff, member_bufs);
+	
 	/* Done with critical section */
 	END_CRIT_SECTION();
 
@@ -852,31 +856,37 @@ MultiXactIdCreateFromMembers(int nmembers, MultiXactMember *members)
  */
 static void
 RecordNewMultiXact(MultiXactId multi, MultiXactOffset offset,
-				   int nmembers, MultiXactMember *members)
+				   int nmembers, MultiXactMember *members, Buffer * offset_buf_ptr, Buffer * member_bufs)
 {
 	int			pageno;
 	int			prev_pageno;
+	int 		min_pageno;
 	int			entryno;
 	MultiXactOffset *offptr;
 	int			i;
 	Buffer		buffer;
+	Buffer		offset_buf;
 
 	pageno = MultiXactIdToOffsetPage(multi);
 	entryno = MultiXactIdToOffsetEntry(multi);
 
-	/* XXX set up error context? */
-	buffer = ReadSlruBuffer(SLRU_MULTIXACT_OFFSET_REL_ID, pageno);
-	LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
-	offptr = (MultiXactOffset *) BufferGetPage(buffer);
+	if (offset_buf_ptr)
+		offset_buf = *offset_buf_ptr;
+	else 
+		offset_buf = ReadSlruBuffer(SLRU_MULTIXACT_OFFSET_ID, pageno);
+
+	LockBuffer(offset_buf, BUFFER_LOCK_EXCLUSIVE);
+						
+	offptr = (MultiXactOffset *) BufferGetPage(offset_buf);
 	offptr += entryno;
 
 	*offptr = offset;
 
-	MarkBufferDirty(buffer);
-
-	UnlockReleaseBuffer(buffer);
+	MarkBufferDirty(offset_buf);
+	UnlockReleaseBuffer(offset_buf);
 	buffer = InvalidBuffer;
 
+	min_pageno = MXOffsetToMemberPage(offset);
 	prev_pageno = -1;
 
 	for (i = 0; i < nmembers; i++, offset++)
@@ -899,7 +909,16 @@ RecordNewMultiXact(MultiXactId multi, MultiXactOffset offset,
 		{
 			if (BufferIsValid(buffer))
 				UnlockReleaseBuffer(buffer);
-			buffer = ReadSlruBuffer(SLRU_MULTIXACT_MEMBER_REL_ID, pageno);
+			
+			/* 
+			 * If we were in a critical section, member_bufs should be provided beforehand
+			 * so we don't have to make ReadBuffer calls
+			 */	
+			if (member_bufs)
+				buffer = member_bufs[pageno - min_pageno];
+			else
+				buffer = ReadSlruBuffer(SLRU_MULTIXACT_MEMBER_ID, pageno);
+
 			LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
 			prev_pageno = pageno;
 		}
@@ -919,14 +938,19 @@ RecordNewMultiXact(MultiXactId multi, MultiXactOffset offset,
 	}
 
 	UnlockReleaseBuffer(buffer);
+	if (member_bufs != NULL)
+		pfree(member_bufs);
 }
 
 /*
  * GetNewMultiXactId
  *		Get the next MultiXactId.
  *
- * Also, reserve the needed amount of space in the "members" area.  The
- * starting offset of the reserved space is returned in *offset.
+ * Also, reserve the needed amount of space in the "members" area. The
+ * starting offset of the reserved space is returned in *offset. A pointer
+ * to the array where the space for the members is allocated is returned
+ * via member_bufs.
+ *
  *
  * This may generate XLOG records for expansion of the offsets and/or members
  * files.  Unfortunately, we have to do that while holding MultiXactGenLock
@@ -937,8 +961,11 @@ RecordNewMultiXact(MultiXactId multi, MultiXactOffset offset,
  * caller must end the critical section after writing SLRU data.
  */
 static MultiXactId
-GetNewMultiXactId(int nmembers, MultiXactOffset *offset)
+GetNewMultiXactId(int nmembers, MultiXactOffset *offset, Buffer * offset_buf, Buffer ** member_bufs)
 {
+	int min_pageno;
+	int max_pageno;
+
 	MultiXactId result;
 	MultiXactOffset nextOffset;
 
@@ -1056,7 +1083,7 @@ GetNewMultiXactId(int nmembers, MultiXactOffset *offset)
 	}
 
 	/* Make sure there is room for the MXID in the file.  */
-	ExtendMultiXactOffset(result);
+	ExtendMultiXactOffset(result, offset_buf);
 
 	/*
 	 * Reserve the members space, similarly to above.  Also, be careful not to
@@ -1144,7 +1171,16 @@ GetNewMultiXactId(int nmembers, MultiXactOffset *offset)
 							   MultiXactState->offsetStopLimit - nextOffset + nmembers),
 				 errhint("Execute a database-wide VACUUM in that database with reduced vacuum_multixact_freeze_min_age and vacuum_multixact_freeze_table_age settings.")));
 
-	ExtendMultiXactMember(nextOffset, nmembers);
+	min_pageno = MXOffsetToMemberPage(nextOffset);
+	max_pageno = MXOffsetToMemberPage(nextOffset + nmembers - 1);
+
+	
+	/* pre allocate memory here before critical section for needed buffers to avoid 
+	 * having to make ReadBuffer calls which may need to allocate memory.
+	 */
+	*member_bufs = (Buffer *) palloc(sizeof(Buffer) * (max_pageno - min_pageno + 1));
+
+	ExtendMultiXactMember(nextOffset, nmembers, member_bufs);
 
 	/*
 	 * Critical section from here until caller has written the data into the
@@ -2300,22 +2336,30 @@ MultiXactAdvanceOldest(MultiXactId oldestMulti, Oid oldestMultiDB)
  * room in shared memory.
  */
 static void
-ExtendMultiXactOffset(MultiXactId multi)
+ExtendMultiXactOffset(MultiXactId multi, Buffer * buffer)
 {
 	int			pageno;
 
+
 	/*
-	 * No work except at first MultiXactId of a page.  But beware: just after
-	 * wraparound, the first MultiXactId of page zero is FirstMultiXactId.
+	 * Make a ReadBuffer call for the page we need beforehand so that we don't need
+	 * to malloc later.
+	 * If we're at the first MultiXactId of a page, make sure we also zero the page
 	 */
+
+	pageno = MultiXactIdToOffsetPage(multi);
 	if (MultiXactIdToOffsetEntry(multi) != 0 &&
 		multi != FirstMultiXactId)
+	{
+		/* make a read buffer call to enlarge the resource owner */
+		*buffer = ReadSlruBuffer(SLRU_MULTIXACT_OFFSET_ID, pageno);
 		return;
-
-	pageno = MultiXactIdToOffsetPage(multi);
-
-	/* Zero the page and make an XLOG entry about it */
-	UnlockReleaseBuffer(ZeroMultiXactOffsetPage(pageno, true));
+	} else
+	{
+		/* Zero the page and make an XLOG entry about it */
+		*buffer = ZeroMultiXactOffsetPage(pageno, true);
+		LockBuffer(*buffer, BUFFER_LOCK_UNLOCK); /* release lock but don't unpin */
+	}
 }
 
 /*
@@ -2326,7 +2370,7 @@ ExtendMultiXactOffset(MultiXactId multi)
  * same comments apply.
  */
 static void
-ExtendMultiXactMember(MultiXactOffset offset, int nmembers)
+ExtendMultiXactMember(MultiXactOffset offset, int nmembers, Buffer ** buffers)
 {
 	/*
 	 * It's possible that the members span more than one page of the members
@@ -2334,10 +2378,17 @@ ExtendMultiXactMember(MultiXactOffset offset, int nmembers)
 	 * optimal if the members span several pages, but that seems unusual
 	 * enough to not worry much about.
 	 */
+	int min_pageno;
+
+	min_pageno = MXOffsetToMemberPage(offset);
 	while (nmembers > 0)
 	{
+		Buffer buf;
+
 		int			flagsoff;
 		int			flagsbit;
+		int			pageno;
+
 		uint32		difference;
 
 		/*
@@ -2345,16 +2396,24 @@ ExtendMultiXactMember(MultiXactOffset offset, int nmembers)
 		 */
 		flagsoff = MXOffsetToFlagsOffset(offset);
 		flagsbit = MXOffsetToFlagsBitShift(offset);
+		pageno = MXOffsetToMemberPage(offset);
+		
+		
+		
 		if (flagsoff == 0 && flagsbit == 0)
 		{
-			int			pageno;
-
-			pageno = MXOffsetToMemberPage(offset);
-
 			/* Zero the page and make an XLOG entry about it */
-			UnlockReleaseBuffer(ZeroMultiXactMemberPage(pageno, true));
+			buf = ZeroMultiXactMemberPage(pageno, true);
+			LockBuffer(buf, BUFFER_LOCK_UNLOCK);
+		} else
+		{
+			/* do a read buffer call to allocate space beforehand */
+			buf = ReadSlruBuffer(SLRU_MULTIXACT_MEMBER_ID, pageno);
 		}
 
+		if (buffers)
+			(*buffers)[pageno - min_pageno] = buf;
+
 		/*
 		 * Compute the number of items till end of current page.  Careful: if
 		 * addition of unsigned ints wraps around, we're at the last page of
@@ -3139,7 +3198,7 @@ multixact_redo(XLogReaderState *record)
 
 		/* Store the data back into the SLRU files */
 		RecordNewMultiXact(xlrec->mid, xlrec->moff, xlrec->nmembers,
-						   xlrec->members);
+						   xlrec->members, NULL, NULL);
 
 		/* Make sure nextMXact/nextOffset are beyond what this record has */
 		MultiXactAdvanceNextMXact(xlrec->mid + 1,
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index c6af8cfd7e2..a77acf53196 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -2296,6 +2296,10 @@ RecordTransactionCommitPrepared(TransactionId xid,
 	TimestampTz committs = GetCurrentTimestamp();
 	bool		replorigin;
 
+	//set resource owner already enlarged flag
+	ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
+
+	
 	/*
 	 * Are we using the replication origins feature?  Or, in other words, are
 	 * we replaying remote actions?
@@ -2360,6 +2364,8 @@ RecordTransactionCommitPrepared(TransactionId xid,
 
 	END_CRIT_SECTION();
 
+	//reset resource owner already enlarged flag 
+
 	/*
 	 * Wait for synchronous replication, if required.
 	 *
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 8daaa535edf..5aebd2ec20f 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -68,6 +68,7 @@
 #include "utils/inval.h"
 #include "utils/memutils.h"
 #include "utils/relmapper.h"
+#include "utils/resowner_private.h"
 #include "utils/snapmgr.h"
 #include "utils/timeout.h"
 #include "utils/timestamp.h"
@@ -1391,6 +1392,24 @@ RecordTransactionCommit(void)
 		 * are delaying the checkpoint a bit fuzzy, but it doesn't matter.
 		 */
 		Assert((MyProc->delayChkptFlags & DELAY_CHKPT_START) == 0);
+		
+		/*
+	 	 * After moving the commit log to the buffer cache, we may need to
+		 * create a hashtable entry in the storage manager hash table for the
+		 * commit log which will require allocating memory. We want to do this
+		 * before we start the critical section.
+		 */
+		Assert(VerifyClogLocatorInHashTable());
+
+
+		/*
+		* Similar to above - we may need to allocate memory for a new buffer pin
+		* for ReadBuffer while getting a clog page, so ensure we have enough memory
+		* before starting critical section.
+		*/
+		ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
+
+		
 		START_CRIT_SECTION();
 		MyProc->delayChkptFlags |= DELAY_CHKPT_START;
 
@@ -1426,6 +1445,8 @@ RecordTransactionCommit(void)
 		TransactionTreeSetCommitTsData(xid, nchildren, children,
 									   replorigin_session_origin_timestamp,
 									   replorigin_session_origin);
+
+		//reset resource owner already enlarged flag
 	}
 
 	/*
@@ -1754,6 +1775,7 @@ RecordTransactionAbort(bool isSubXact)
 	ndroppedstats = pgstat_get_transactional_drops(false, &droppedstats);
 
 	/* XXX do we really need a critical section here? */
+	ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
 	START_CRIT_SECTION();
 
 	/* Write the ABORT record */
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index c2944fb68f0..c590f05a0d9 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -647,9 +647,8 @@ ReadRecentBuffer(RelFileLocator rlocator, ForkNumber forkNum, BlockNumber blockN
 	BufferTag	tag;
 	uint32		buf_state;
 	bool		have_private_ref;
-
+	
 	Assert(BufferIsValid(recent_buffer));
-
 	ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
 	ReservePrivateRefCountEntry();
 	InitBufferTag(&tag, &rlocator, forkNum, blockNum);
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 59412ba8a60..bfddd944122 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -1023,9 +1023,7 @@ MemoryContextAlloc(MemoryContext context, Size size)
 	void	   *ret;
 
 	Assert(MemoryContextIsValid(context));
-	// XXX horrible hack: while pinning clog buffers during commit, resowner
-	// stuff allocates!
-	//AssertNotInCriticalSection(context);
+	AssertNotInCriticalSection(context);
 
 	if (!AllocSizeIsValid(size))
 		elog(ERROR, "invalid memory alloc request size %zu", size);
@@ -1238,7 +1236,7 @@ palloc(Size size)
 	 * CLOG buffers during commit, in a critical section (md.c allocates
 	 * memory to build paths...); FIXME!
 	 */
-	//AssertNotInCriticalSection(context);
+	AssertNotInCriticalSection(context);
 
 	if (!AllocSizeIsValid(size))
 		elog(ERROR, "invalid memory alloc request size %zu", size);
diff --git a/src/include/access/clog.h b/src/include/access/clog.h
index aacf10ca522..4d53481bd9e 100644
--- a/src/include/access/clog.h
+++ b/src/include/access/clog.h
@@ -45,6 +45,7 @@ extern void StartupCLOG(void);
 extern void TrimCLOG(void);
 extern void ExtendCLOG(TransactionId newestXact);
 extern void TruncateCLOG(TransactionId oldestXact, Oid oldestxid_datoid);
+extern bool VerifyClogLocatorInHashTable(void);
 
 /* XLOG stuff */
 #define CLOG_ZEROPAGE		0x00
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 21640d62a64..c4b1a6f08de 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -62,6 +62,7 @@ extern PGDLLIMPORT MemoryContext CacheMemoryContext;
 extern PGDLLIMPORT MemoryContext MessageContext;
 extern PGDLLIMPORT MemoryContext TopTransactionContext;
 extern PGDLLIMPORT MemoryContext CurTransactionContext;
+extern PGDLLIMPORT MemoryContext SlruMemoryContext;
 
 /* This is a transient link to the active portal's memory context: */
 extern PGDLLIMPORT MemoryContext PortalContext;
-- 
2.40.1

