amcheck: Refactoring the storage of the last visible entry

This commit introduces a new data structure BtreeLastVisibleEntry comprising
information about the last visible heap entry with the current value of key.
Usage of this data structure allows us to avoid passing all this information
as individual function arguments.

Reported-by: Alexander Korotkov
Discussion: https://www.postgresql.org/message-id/CAPpHfdsVbB9ToriaB1UHuOKwjKxiZmTFQcEF%3DjuzzC_nby31uA%40mail.gmail.com
Author: Pavel Borisov, Alexander Korotkov

Author: akorotkov

contrib/amcheck/verify_nbtree.c

@@ -145,6 +145,19 @@ typedef struct BtreeLevel
 	bool		istruerootlevel;
 } BtreeLevel;
 
+/*
+ * Information about the last visible entry with current B-tree key.  Used
+ * for validation of the unique constraint.
+ */
+typedef struct BtreeLastVisibleEntry
+{
+	BlockNumber blkno;			/* Index block */
+	OffsetNumber offset;		/* Offset on index block */
+	int			postingIndex;	/* Number in the posting list (-1 for
+								 * non-deduplicated tuples) */
+	ItemPointer tid;			/* Heap tid */
+} BtreeLastVisibleEntry;
+
 PG_FUNCTION_INFO_V1(bt_index_check);
 PG_FUNCTION_INFO_V1(bt_index_parent_check);
 
@@ -165,17 +178,14 @@ static void bt_recheck_sibling_links(BtreeCheckState *state,
 									 BlockNumber btpo_prev_from_target,
 									 BlockNumber leftcurrent);
 static bool heap_entry_is_visible(BtreeCheckState *state, ItemPointer tid);
-static void bt_report_duplicate(BtreeCheckState *state, ItemPointer tid,
-								BlockNumber block, OffsetNumber offset,
-								int posting, ItemPointer nexttid,
+static void bt_report_duplicate(BtreeCheckState *state,
+								BtreeLastVisibleEntry *lVis,
+								ItemPointer nexttid,
 								BlockNumber nblock, OffsetNumber noffset,
 								int nposting);
 static void bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup,
-								  BlockNumber targetblock,
-								  OffsetNumber offset, int *lVis_i,
-								  ItemPointer *lVis_tid,
-								  OffsetNumber *lVis_offset,
-								  BlockNumber *lVis_block);
+								  BlockNumber targetblock, OffsetNumber offset,
+								  BtreeLastVisibleEntry *lVis);
 static void bt_target_page_check(BtreeCheckState *state);
 static BTScanInsert bt_right_page_check_scankey(BtreeCheckState *state,
 												OffsetNumber *rightfirstoffset);
@@ -997,8 +1007,7 @@ heap_entry_is_visible(BtreeCheckState *state, ItemPointer tid)
  */
 static void
 bt_report_duplicate(BtreeCheckState *state,
-					ItemPointer tid, BlockNumber block, OffsetNumber offset,
-					int posting,
+					BtreeLastVisibleEntry *lVis,
 					ItemPointer nexttid, BlockNumber nblock, OffsetNumber noffset,
 					int nposting)
 {
@@ -1010,18 +1019,18 @@ bt_report_duplicate(BtreeCheckState *state,
 			   *pnposting = "";
 
 	htid = psprintf("tid=(%u,%u)",
-					ItemPointerGetBlockNumberNoCheck(tid),
-					ItemPointerGetOffsetNumberNoCheck(tid));
+					ItemPointerGetBlockNumberNoCheck(lVis->tid),
+					ItemPointerGetOffsetNumberNoCheck(lVis->tid));
 	nhtid = psprintf("tid=(%u,%u)",
 					 ItemPointerGetBlockNumberNoCheck(nexttid),
 					 ItemPointerGetOffsetNumberNoCheck(nexttid));
-	itid = psprintf("tid=(%u,%u)", block, offset);
+	itid = psprintf("tid=(%u,%u)", lVis->blkno, lVis->offset);
 
-	if (nblock != block || noffset != offset)
+	if (nblock != lVis->blkno || noffset != lVis->offset)
 		nitid = psprintf(" tid=(%u,%u)", nblock, noffset);
 
-	if (posting >= 0)
-		pposting = psprintf(" posting %u", posting);
+	if (lVis->postingIndex >= 0)
+		pposting = psprintf(" posting %u", lVis->postingIndex);
 
 	if (nposting >= 0)
 		pnposting = psprintf(" posting %u", nposting);
@@ -1038,9 +1047,8 @@ bt_report_duplicate(BtreeCheckState *state,
 /* Check if current nbtree leaf entry complies with UNIQUE constraint */
 static void
 bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup,
-					  BlockNumber targetblock, OffsetNumber offset, int *lVis_i,
-					  ItemPointer *lVis_tid, OffsetNumber *lVis_offset,
-					  BlockNumber *lVis_block)
+					  BlockNumber targetblock, OffsetNumber offset,
+					  BtreeLastVisibleEntry *lVis)
 {
 	ItemPointer tid;
 	bool		has_visible_entry = false;
@@ -1049,7 +1057,7 @@ bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup,
 
 	/*
 	 * Current tuple has posting list. Report duplicate if TID of any posting
-	 * list entry is visible and lVis_tid is valid.
+	 * list entry is visible and lVis->tid is valid.
 	 */
 	if (BTreeTupleIsPosting(itup))
 	{
@@ -1059,11 +1067,10 @@ bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup,
 			if (heap_entry_is_visible(state, tid))
 			{
 				has_visible_entry = true;
-				if (ItemPointerIsValid(*lVis_tid))
+				if (ItemPointerIsValid(lVis->tid))
 				{
 					bt_report_duplicate(state,
-										*lVis_tid, *lVis_block,
-										*lVis_offset, *lVis_i,
+										lVis,
 										tid, targetblock,
 										offset, i);
 				}
@@ -1073,59 +1080,60 @@ bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup,
 				 * between the posting list entries of the first tuple on the
 				 * page after cross-page check.
 				 */
-				if (*lVis_block != targetblock && ItemPointerIsValid(*lVis_tid))
+				if (lVis->blkno != targetblock && ItemPointerIsValid(lVis->tid))
 					return;
 
-				*lVis_i = i;
-				*lVis_tid = tid;
-				*lVis_offset = offset;
-				*lVis_block = targetblock;
+				lVis->blkno = targetblock;
+				lVis->offset = offset;
+				lVis->postingIndex = i;
+				lVis->tid = tid;
 			}
 		}
 	}
 
 	/*
 	 * Current tuple has no posting list. If TID is visible save info about it
 	 * for the next comparisons in the loop in bt_target_page_check(). Report
-	 * duplicate if lVis_tid is already valid.
+	 * duplicate if lVis->tid is already valid.
 	 */
 	else
 	{
 		tid = BTreeTupleGetHeapTID(itup);
 		if (heap_entry_is_visible(state, tid))
 		{
 			has_visible_entry = true;
-			if (ItemPointerIsValid(*lVis_tid))
+			if (ItemPointerIsValid(lVis->tid))
 			{
 				bt_report_duplicate(state,
-									*lVis_tid, *lVis_block,
-									*lVis_offset, *lVis_i,
+									lVis,
 									tid, targetblock,
 									offset, -1);
 			}
-			*lVis_i = -1;
-			*lVis_tid = tid;
-			*lVis_offset = offset;
-			*lVis_block = targetblock;
+
+			lVis->blkno = targetblock;
+			lVis->offset = offset;
+			lVis->tid = tid;
+			lVis->postingIndex = -1;
 		}
 	}
 
-	if (!has_visible_entry && *lVis_block != InvalidBlockNumber &&
-		*lVis_block != targetblock)
+	if (!has_visible_entry &&
+		lVis->blkno != InvalidBlockNumber &&
+		lVis->blkno != targetblock)
 	{
 		char	   *posting = "";
 
-		if (*lVis_i >= 0)
-			posting = psprintf(" posting %u", *lVis_i);
+		if (lVis->postingIndex >= 0)
+			posting = psprintf(" posting %u", lVis->postingIndex);
 		ereport(DEBUG1,
 				(errcode(ERRCODE_NO_DATA),
 				 errmsg("index uniqueness can not be checked for index tid=(%u,%u) in index \"%s\"",
 						targetblock, offset,
 						RelationGetRelationName(state->rel)),
 				 errdetail("It doesn't have visible heap tids and key is equal to the tid=(%u,%u)%s (points to heap tid=(%u,%u)).",
-						   *lVis_block, *lVis_offset, posting,
-						   ItemPointerGetBlockNumberNoCheck(*lVis_tid),
-						   ItemPointerGetOffsetNumberNoCheck(*lVis_tid)),
+						   lVis->blkno, lVis->offset, posting,
+						   ItemPointerGetBlockNumberNoCheck(lVis->tid),
+						   ItemPointerGetOffsetNumberNoCheck(lVis->tid)),
 				 errhint("VACUUM the table and repeat the check.")));
 	}
 }
@@ -1372,12 +1380,8 @@ bt_target_page_check(BtreeCheckState *state)
 	OffsetNumber max;
 	BTPageOpaque topaque;
 
-	/* last visible entry info for checking indexes with unique constraint */
-	int			lVis_i = -1;	/* the position of last visible item for
-								 * posting tuple. for non-posting tuple (-1) */
-	ItemPointer lVis_tid = NULL;
-	BlockNumber lVis_block = InvalidBlockNumber;
-	OffsetNumber lVis_offset = InvalidOffsetNumber;
+	/* Last visible entry info for checking indexes with unique constraint */
+	BtreeLastVisibleEntry lVis = {InvalidBlockNumber, InvalidOffsetNumber, -1, NULL};
 
 	topaque = BTPageGetOpaque(state->target);
 	max = PageGetMaxOffsetNumber(state->target);
@@ -1776,8 +1780,7 @@ bt_target_page_check(BtreeCheckState *state)
 		if (state->checkunique && state->indexinfo->ii_Unique &&
 			P_ISLEAF(topaque) && !skey->anynullkeys)
 			bt_entry_unique_check(state, itup, state->targetblock, offset,
-								  &lVis_i, &lVis_tid, &lVis_offset,
-								  &lVis_block);
+								  &lVis);
 
 		if (state->checkunique && state->indexinfo->ii_Unique &&
 			P_ISLEAF(topaque) && OffsetNumberNext(offset) <= max)
@@ -1800,10 +1803,10 @@ bt_target_page_check(BtreeCheckState *state)
 			if (_bt_compare(state->rel, skey, state->target,
 							OffsetNumberNext(offset)) != 0 || skey->anynullkeys)
 			{
-				lVis_i = -1;
-				lVis_tid = NULL;
-				lVis_block = InvalidBlockNumber;
-				lVis_offset = InvalidOffsetNumber;
+				lVis.blkno = InvalidBlockNumber;
+				lVis.offset = InvalidOffsetNumber;
+				lVis.postingIndex = -1;
+				lVis.tid = NULL;
 			}
 			skey->scantid = scantid;	/* Restore saved scan key state */
 		}
@@ -1902,9 +1905,7 @@ bt_target_page_check(BtreeCheckState *state)
 												  rightfirstoffset);
 					itup = (IndexTuple) PageGetItem(state->target, itemid);
 
-					bt_entry_unique_check(state, itup, rightblock_number, rightfirstoffset,
-										  &lVis_i, &lVis_tid, &lVis_offset,
-										  &lVis_block);
+					bt_entry_unique_check(state, itup, rightblock_number, rightfirstoffset, &lVis);
 				}
 			}
 		}

src/tools/pgindent/typedefs.list

@@ -322,6 +322,7 @@ BrinStatsData
 BrinTuple
 BrinValues
 BtreeCheckState
+BtreeLastVisibleEntry
 BtreeLevel
 Bucket
 BufFile