From a6f6b78196a701702ec4ff6df56c346bdcf9abd2 Mon Sep 17 00:00:00 2001
From: Kevin Grittner <kgrittn@postgresql.org>
Date: Mon, 11 Apr 2016 16:47:50 -0500
Subject: [PATCH] Use static inline function for BufferGetPage()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

I was initially concerned that the some of the hundreds of
references to BufferGetPage() where the literal
BGP_NO_SNAPSHOT_TEST were passed might not optimize as well as a
macro, leading to some hard-to-find performance regressions in
corner cases.  Inspection of disassembled code has shown identical
code at all inspected locations, and the size difference doesn't
amount to even one byte per such call.  So make it readable.

Per gripes from Álvaro Herrera and Tom Lane
---
 src/backend/storage/buffer/bufmgr.c |  4 +--
 src/include/storage/bufmgr.h        | 50 ++++++++++++++---------------
 2 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 29f10e5956..f402e0dd4b 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4295,7 +4295,7 @@ IssuePendingWritebacks(WritebackContext *context)
  * For best performance, keep the tests that are fastest and/or most likely to
  * exclude a page from old snapshot testing near the front.
  */
-extern Page
+extern void
 TestForOldSnapshot(Snapshot snapshot, Relation relation, Page page)
 {
 	Assert(relation != NULL);
@@ -4311,6 +4311,4 @@ TestForOldSnapshot(Snapshot snapshot, Relation relation, Page page)
 		ereport(ERROR,
 				(errcode(ERRCODE_SNAPSHOT_TOO_OLD),
 				 errmsg("snapshot too old")));
-
-	return page;
 }
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 6fea1bc13d..afd8a847ef 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -177,31 +177,6 @@ extern PGDLLIMPORT int32 *LocalRefCount;
 	(Size)BLCKSZ \
 )
 
-/*
- * BufferGetPage
- *		Returns the page associated with a buffer.
- *
- * agetest will normally be a literal, so use a macro at the outer level to
- * give the compiler a chance to optimize away the runtime code to check it.
- *
- * TestForOldSnapshot(), if it doesn't throw an error, will return the page
- * argument it is passed, so the same result will go back to this macro's
- * caller for either agetest value; it is a matter of whether to call the
- * function to perform the test.  For call sites where the check is not needed
- * (which is the vast majority of them), the snapshot and relation parameters
- * can, and generally should, be NULL.
- */
-#define BufferGetPage(buffer, snapshot, relation, agetest) \
-( \
-	( \
-		AssertMacro((agetest) == BGP_NO_SNAPSHOT_TEST || (agetest) == BGP_TEST_FOR_OLD_SNAPSHOT), \
-		((agetest) == BGP_NO_SNAPSHOT_TEST) \
-	) ? \
-		((Page)BufferGetBlock(buffer)) \
-	: \
-		(TestForOldSnapshot(snapshot, relation, (Page)BufferGetBlock(buffer))) \
-)
-
 /*
  * prototypes for functions in bufmgr.c
  */
@@ -268,10 +243,33 @@ extern bool BgBufferSync(struct WritebackContext *wb_context);
 
 extern void AtProcExit_LocalBuffers(void);
 
-extern Page TestForOldSnapshot(Snapshot snapshot, Relation relation, Page page);
+extern void TestForOldSnapshot(Snapshot snapshot, Relation relation, Page page);
 
 /* in freelist.c */
 extern BufferAccessStrategy GetAccessStrategy(BufferAccessStrategyType btype);
 extern void FreeAccessStrategy(BufferAccessStrategy strategy);
 
+
+/* inline functions */
+
+/*
+ * BufferGetPage
+ *		Returns the page associated with a buffer.
+ *
+ * For call sites where the check is not needed (which is the vast majority of
+ * them), the snapshot and relation parameters can, and generally should, be
+ * NULL.
+ */
+static inline Page
+BufferGetPage(Buffer buffer, Snapshot snapshot, Relation relation,
+			  BufferGetPageAgeTest agetest)
+{
+	Page		page = (Page) BufferGetBlock(buffer);
+
+	if (agetest == BGP_TEST_FOR_OLD_SNAPSHOT)
+		TestForOldSnapshot(snapshot, relation, page);
+
+	return page;
+}
+
 #endif