Recycle deleted nbtree pages more aggressively.

petergeoghegan · petergeoghegan · commit e48c817395e1 · 2023-04-03T11:31:43.000-07:00
Commit 61b313e made nbtree consistently pass down a heaprel to low level routines like _bt_getbuf(). Although this was primarily intended as preparation for logical decoding on standbys, it also made it easy to correct an old deficiency in how nbtree VACUUM determines whether or not it's now safe to recycle deleted pages. Pass the heaprel to GlobalVisTestFor() in nbtree routines that deal with recycle safety. nbtree now makes less pessimistic assumptions about recycle safety within non-catalog relations. This enhancement complements the recycling enhancement added by commit 9dd963a. nbtree remains just as pessimistic as ever when it comes to recycle safety within indexes on catalog relations. There is no fundamental reason why we need to treat catalog relations differently, though. The behavioral inconsistency is a consequence of the way that nbtree uses nextXID values to implement what Lanin and Shasha call "the drain technique". Note in particular that it has nothing to do with whether or not index tuples might still be required for an older MVCC snapshot. Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com> Discussion: https://postgr.es/m/CAH2-WzkaiDxCje0yPuH=3Uh2p1V_2pFGY==xfbZoZu7Ax_NB8g@mail.gmail.com
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
@@ -935,7 +935,7 @@ _bt_getbuf(Relation rel, Relation heaprel, BlockNumber blkno, int access)
 					return buf;
 				}
 
-				if (BTPageIsRecyclable(page))
+				if (BTPageIsRecyclable(page, heaprel))
 				{
 					/*
 					 * If we are generating WAL for Hot Standby then create a
@@ -2963,6 +2963,7 @@ void
 _bt_pendingfsm_finalize(Relation rel, BTVacState *vstate)
 {
 	IndexBulkDeleteResult *stats = vstate->stats;
+	Relation    heaprel = vstate->info->heaprel;
 
 	Assert(stats->pages_newly_deleted >= vstate->npendingpages);
 
@@ -2995,7 +2996,7 @@ _bt_pendingfsm_finalize(Relation rel, BTVacState *vstate)
 	 * essential; GlobalVisCheckRemovableFullXid() will not reliably recognize
 	 * that it is now safe to recycle newly deleted pages without this step.
 	 */
-	GetOldestNonRemovableTransactionId(NULL);
+	GetOldestNonRemovableTransactionId(heaprel);
 
 	for (int i = 0; i < vstate->npendingpages; i++)
 	{
@@ -3010,7 +3011,7 @@ _bt_pendingfsm_finalize(Relation rel, BTVacState *vstate)
 		 * must be non-recyclable too, since _bt_pendingfsm_add() adds pages
 		 * to the array in safexid order.
 		 */
-		if (!GlobalVisCheckRemovableFullXid(NULL, safexid))
+		if (!GlobalVisCheckRemovableFullXid(heaprel, safexid))
 			break;
 
 		RecordFreeIndexPage(rel, target);
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
@@ -1039,6 +1039,7 @@ btvacuumpage(BTVacState *vstate, BlockNumber scanblkno)
 	IndexBulkDeleteCallback callback = vstate->callback;
 	void	   *callback_state = vstate->callback_state;
 	Relation	rel = info->index;
+	Relation	heaprel = info->heaprel;
 	bool		attempt_pagedel;
 	BlockNumber blkno,
 				backtrack_to;
@@ -1124,7 +1125,7 @@ btvacuumpage(BTVacState *vstate, BlockNumber scanblkno)
 		}
 	}
 
-	if (!opaque || BTPageIsRecyclable(page))
+	if (!opaque || BTPageIsRecyclable(page, heaprel))
 	{
 		/* Okay to recycle this page (which could be leaf or internal) */
 		RecordFreeIndexPage(rel, blkno);
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
@@ -288,7 +288,7 @@ BTPageGetDeleteXid(Page page)
  * well need special handling for new pages anyway.
  */
 static inline bool
-BTPageIsRecyclable(Page page)
+BTPageIsRecyclable(Page page, Relation heaprel)
 {
 	BTPageOpaque opaque;
 
@@ -307,12 +307,8 @@ BTPageIsRecyclable(Page page)
 		 * For that check if the deletion XID could still be visible to
 		 * anyone. If not, then no scan that's still in progress could have
 		 * seen its downlink, and we can recycle it.
-		 *
-		 * XXX: If we had the heap relation we could be more aggressive about
-		 * recycling deleted pages in non-catalog relations.  For now we just
-		 * pass NULL.  That is at least simple and consistent.
 		 */
-		return GlobalVisCheckRemovableFullXid(NULL, BTPageGetDeleteXid(page));
+		return GlobalVisCheckRemovableFullXid(heaprel, BTPageGetDeleteXid(page));
 	}
 
 	return false;

Original file line number	Diff line number	Diff line change
`@@ -1039,6 +1039,7 @@ btvacuumpage(BTVacState *vstate, BlockNumber scanblkno)`
`1039`	`1039`	`IndexBulkDeleteCallback callback = vstate->callback;`
`1040`	`1040`	`void *callback_state = vstate->callback_state;`
`1041`	`1041`	`Relation rel = info->index;`
	`1042`	`+ Relation heaprel = info->heaprel;`
`1042`	`1043`	`bool attempt_pagedel;`
`1043`	`1044`	`BlockNumber blkno,`
`1044`	`1045`	`backtrack_to;`
`@@ -1124,7 +1125,7 @@ btvacuumpage(BTVacState *vstate, BlockNumber scanblkno)`
`1124`	`1125`	`}`
`1125`	`1126`	`}`
`1126`	`1127`
`1127`		`- if (!opaque \|\| BTPageIsRecyclable(page))`
	`1128`	`+ if (!opaque \|\| BTPageIsRecyclable(page, heaprel))`
`1128`	`1129`	`{`
`1129`	`1130`	`/* Okay to recycle this page (which could be leaf or internal) */`
`1130`	`1131`	`RecordFreeIndexPage(rel, blkno);`