Subject: [PATCH 3/6] Minor cleanup

From: Heikki Linnakangas <[email-masked]>
Date: Mon, 11 Mar 2019 15:15:43 +0200

I renamed gistXLogSetDeleted to gistXLogPageDelete. I think that's better,
the WAL record also includes information about removing the downlink from
the parent, not just setting the flag on the child.
---
 src/backend/access/gist/gist.c       |  7 +--
 src/backend/access/gist/gistvacuum.c |  9 +--
 src/backend/access/gist/gistxlog.c   | 88 +++++++++++++++-------------
 src/include/access/gist_private.h    |  6 +-
 src/include/access/gistxlog.h        | 10 ++--
 5 files changed, 62 insertions(+), 58 deletions(-)

diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 6f87b4b5044..e260c4df4e7 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -844,11 +844,10 @@ gistdoinsert(Relation r, IndexTuple itup, Size freespace,
 			}
 
 			/*
-			 * Leaf pages can be left deleted but still referenced
-			 * until it's space is reused. Downlink to this page may be already
-			 * removed from the internal page, but this scan can posess it.
+			 * The page might have been deleted after we scanned the parent
+			 * and saw the downlink.
 			 */
-			if(GistPageIsDeleted(stack->page))
+			if (GistPageIsDeleted(stack->page))
 			{
 				UnlockReleaseBuffer(stack->buffer);
 				xlocked = false;
diff --git a/src/backend/access/gist/gistvacuum.c b/src/backend/access/gist/gistvacuum.c
index fc606ac823c..eb90b2077d3 100644
--- a/src/backend/access/gist/gistvacuum.c
+++ b/src/backend/access/gist/gistvacuum.c
@@ -20,11 +20,9 @@
 #include "commands/vacuum.h"
 #include "lib/blockset.h"
 #include "miscadmin.h"
-#include "nodes/bitmapset.h"
 #include "storage/indexfsm.h"
 #include "storage/lmgr.h"
 
-
 /* Working state needed by gistbulkdelete */
 typedef struct
 {
@@ -58,7 +56,6 @@ gistbulkdelete(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
 
 	gistvacuumscan(info, stats, callback, callback_state);
 
-
 	return stats;
 }
 
@@ -112,7 +109,7 @@ gistvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
  * while the index is being expanded, leaving an all-zeros page behind.
  *
  * The caller is responsible for initially allocating/zeroing a stats struct.
- * 
+ *
  * Bulk deletion of all index entries pointing to a set of heap tuples and
  * check invalid tuples left after upgrade.
  * The set of target tuples is specified via a callback routine that tells
@@ -213,7 +210,7 @@ gistvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
 	stats->num_pages = num_pages;
 	stats->pages_free = vstate.totFreePages;
 
-	/* rescan all inner pages to find those that has empty child pages */
+	/* rescan all inner pages to find those that have empty child pages */
 	if (vstate.emptyPages > 0)
 	{
 		BlockNumber			x;
@@ -305,7 +302,7 @@ gistvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
 					LockBuffer(leafBuffer, GIST_EXCLUSIVE);
 					gistcheckpage(rel, leafBuffer);
 					leafPage = (Page) BufferGetPage(leafBuffer);
-					if (!GistPageIsLeaf(leafPage) /* not a leaf anymore */ 
+					if (!GistPageIsLeaf(leafPage) /* not a leaf anymore */
 						|| PageGetMaxOffsetNumber(leafPage) != InvalidOffsetNumber /* Page is not empry */
 						|| (GistFollowRight(leafPage) || GistPageGetNSN(page) < GistPageGetNSN(leafPage)) /* No follow-right */
 						)
diff --git a/src/backend/access/gist/gistxlog.c b/src/backend/access/gist/gistxlog.c
index 3213ea98ea3..7110c70451e 100644
--- a/src/backend/access/gist/gistxlog.c
+++ b/src/backend/access/gist/gistxlog.c
@@ -64,39 +64,6 @@ gistRedoClearFollowRight(XLogReaderState *record, uint8 block_id)
 		UnlockReleaseBuffer(buffer);
 }
 
-static void
-gistRedoPageSetDeleted(XLogReaderState *record)
-{
-	XLogRecPtr	lsn = record->EndRecPtr;
-	gistxlogPageDelete *xldata = (gistxlogPageDelete *) XLogRecGetData(record);
-	Buffer		buffer;
-	Page		page;
-
-	if (XLogReadBufferForRedo(record, 0, &buffer) == BLK_NEEDS_REDO)
-	{
-		page = (Page) BufferGetPage(buffer);
-
-		GistPageSetDeleteXid(page, xldata->deleteXid);
-		GistPageSetDeleted(page);
-
-		PageSetLSN(page, lsn);
-		MarkBufferDirty(buffer);
-	}
-	if (BufferIsValid(buffer))
-		UnlockReleaseBuffer(buffer);
-
-	if (XLogReadBufferForRedo(record, 1, &buffer) == BLK_NEEDS_REDO)
-	{
-		page = (Page) BufferGetPage(buffer);
-
-		PageIndexTupleDelete(page, xldata->downlinkOffset);
-
-		PageSetLSN(page, lsn);
-		MarkBufferDirty(buffer);
-	}
-	if (BufferIsValid(buffer))
-		UnlockReleaseBuffer(buffer);
-}
 /*
  * redo any page update (except page split)
  */
@@ -542,6 +509,43 @@ gistRedoCreateIndex(XLogReaderState *record)
 	UnlockReleaseBuffer(buffer);
 }
 
+/* redo page deletion */
+static void
+gistRedoPageDelete(XLogReaderState *record)
+{
+	XLogRecPtr	lsn = record->EndRecPtr;
+	gistxlogPageDelete *xldata = (gistxlogPageDelete *) XLogRecGetData(record);
+	Buffer		buffer;
+	Page		page;
+
+	/* FIXME: Are we locking the pages in correct order, for hot standby? */
+
+	if (XLogReadBufferForRedo(record, 0, &buffer) == BLK_NEEDS_REDO)
+	{
+		page = (Page) BufferGetPage(buffer);
+
+		GistPageSetDeleteXid(page, xldata->deleteXid);
+		GistPageSetDeleted(page);
+
+		PageSetLSN(page, lsn);
+		MarkBufferDirty(buffer);
+	}
+	if (BufferIsValid(buffer))
+		UnlockReleaseBuffer(buffer);
+
+	if (XLogReadBufferForRedo(record, 1, &buffer) == BLK_NEEDS_REDO)
+	{
+		page = (Page) BufferGetPage(buffer);
+
+		PageIndexTupleDelete(page, xldata->downlinkOffset);
+
+		PageSetLSN(page, lsn);
+		MarkBufferDirty(buffer);
+	}
+	if (BufferIsValid(buffer))
+		UnlockReleaseBuffer(buffer);
+}
+
 void
 gist_redo(XLogReaderState *record)
 {
@@ -570,7 +574,7 @@ gist_redo(XLogReaderState *record)
 			gistRedoCreateIndex(record);
 			break;
 		case XLOG_GIST_PAGE_DELETE:
-			gistRedoPageSetDeleted(record);
+			gistRedoPageDelete(record);
 			break;
 		default:
 			elog(PANIC, "gist_redo: unknown op code %u", info);
@@ -691,25 +695,27 @@ gistXLogSplit(bool page_is_leaf,
 }
 
 /*
- * Write XLOG record describing a page delete. This also includes removal of
- * downlink from internal page.
+ * Write XLOG record describing a page deletion. This also includes removal of
+ * downlink from the parent page.
  */
 XLogRecPtr
-gistXLogSetDeleted(RelFileNode node, Buffer buffer, TransactionId xid,
-					Buffer internalPageBuffer, OffsetNumber internalPageOffset) {
+gistXLogPageDelete(Buffer buffer, TransactionId xid,
+				   Buffer parentBuffer, OffsetNumber downlinkOffset)
+{
 	gistxlogPageDelete xlrec;
 	XLogRecPtr	recptr;
 
 	xlrec.deleteXid = xid;
-	xlrec.downlinkOffset = internalPageOffset;
+	xlrec.downlinkOffset = downlinkOffset;
 
 	XLogBeginInsert();
 	XLogRegisterData((char *) &xlrec, sizeof(gistxlogPageDelete));
 
 	XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
-	XLogRegisterBuffer(1, internalPageBuffer, REGBUF_STANDARD);
-	/* new tuples */
+	XLogRegisterBuffer(1, parentBuffer, REGBUF_STANDARD);
+
 	recptr = XLogInsert(RM_GIST_ID, XLOG_GIST_PAGE_DELETE);
+
 	return recptr;
 }
 
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 943163ccce7..c77d0b4dd81 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -415,9 +415,9 @@ extern SplitedPageLayout *gistSplit(Relation r, Page page, IndexTuple *itup,
 		  int len, GISTSTATE *giststate);
 
 /* gistxlog.c */
-extern XLogRecPtr gistXLogSetDeleted(RelFileNode node, Buffer buffer,
-					TransactionId xid, Buffer internalPageBuffer,
-					OffsetNumber internalPageOffset);
+extern XLogRecPtr gistXLogPageDelete(Buffer buffer,
+				   TransactionId xid, Buffer parentBuffer,
+				   OffsetNumber downlinkOffset);
 
 extern XLogRecPtr gistXLogUpdate(Buffer buffer,
 			   OffsetNumber *todelete, int ntodelete,
diff --git a/src/include/access/gistxlog.h b/src/include/access/gistxlog.h
index 127cff5cb78..939a1ea7559 100644
--- a/src/include/access/gistxlog.h
+++ b/src/include/access/gistxlog.h
@@ -17,8 +17,6 @@
 #include "access/xlogreader.h"
 #include "lib/stringinfo.h"
 
-/* XLog stuff */
-
 #define XLOG_GIST_PAGE_UPDATE		0x00
 #define XLOG_GIST_DELETE			0x10 /* delete leaf index tuples for a page */
  /* #define XLOG_GIST_NEW_ROOT			 0x20 */	/* not used anymore */
@@ -78,10 +76,14 @@ typedef struct gistxlogPageSplit
 	 */
 } gistxlogPageSplit;
 
+/*
+ * Backup Blk 0: page that was deleted.
+ * Backup Blk 1: parent page, containing the downlink to the deleted page.
+ */
 typedef struct gistxlogPageDelete
 {
-   TransactionId deleteXid; /* last Xid which could see page in scan */
-   OffsetNumber downlinkOffset; /* Offset of the downlink referencing this page */
+	TransactionId deleteXid;	/* last Xid which could see page in scan */
+	OffsetNumber downlinkOffset; /* Offset of downlink referencing this page */
 } gistxlogPageDelete;
 
 extern void gist_redo(XLogReaderState *record);
-- 
2.20.1

--------------AF9416BC788B4CF5E4BF929D
Content-Type: text/x-patch;
 name="0004-Move-the-page-deletion-logic-to-separate-function.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="0004-Move-the-page-deletion-logic-to-separate-function.patch"