v6-0003-Various-debug-stuff-and-fix-in-xidlist.xid.patch

application/octet-stream

Filename: v6-0003-Various-debug-stuff-and-fix-in-xidlist.xid.patch
Type: application/octet-stream
Part: 2
Message: Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

Patch

Same data as JSON: GET /api/v1/attachments/:id/patch the parsed metadata as JSON — format, series position, per-file stats; never the diff bytes. API reference →
Format: format-patch
Series: patch v6-0003
Subject: Various debug stuff and fix in xidlist.xid
File+
contrib/amcheck/verify_nbtree.c 17 2
src/backend/commands/indexcmds.c 9 1
src/backend/storage/lmgr/lock.c 19 11
From b7fda517fe7041c28b0bbd85ae19bb1a557eafb5 Mon Sep 17 00:00:00 2001
From: Andrey Borodin <amborodin@acm.org>
Date: Thu, 29 Jul 2021 17:26:44 +0500
Subject: [PATCH v6 3/3] Various debug stuff and fix in xidlist.xid

---
 contrib/amcheck/verify_nbtree.c  | 19 +++++++++++++++++--
 src/backend/commands/indexcmds.c | 10 +++++++++-
 src/backend/storage/lmgr/lock.c  | 30 +++++++++++++++++++-----------
 3 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index fdfc320e84..3e1ac42051 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -2453,6 +2453,8 @@ bt_downlink_missing_check(BtreeCheckState *state, bool rightsplit,
  * REINDEX to repair corruption when there was an (undetected) broken HOT chain
  * also allows us to detect the corruption in many cases.
  */
+
+#include "storage/bufpage.h"
 static void
 bt_tuple_present_callback(Relation index, ItemPointer tid, Datum *values,
 						  bool *isnull, bool tupleIsAlive, void *checkstate)
@@ -2471,16 +2473,29 @@ bt_tuple_present_callback(Relation index, ItemPointer tid, Datum *values,
 	/* Probe Bloom filter -- tuple should be present */
 	if (bloom_lacks_element(state->filter, (unsigned char *) norm,
 							IndexTupleSize(norm)))
+	{
+		BlockIdData bid = tid->ip_blkid;
+		Buffer buf = ReadBufferExtended(state->heaprel, MAIN_FORKNUM, BlockIdGetBlockNumber(&bid), RBM_NORMAL, NULL);
+		LockBuffer(buf, BUFFER_LOCK_SHARE);
+		Page page = BufferGetPage(buf);
+
+		HeapTupleHeader tuphdr = (HeapTupleHeader) PageGetItem(page, PageGetItemId(page,tid->ip_posid));
+		
+		LockBuffer(buf, BUFFER_LOCK_UNLOCK);
+		ReleaseBuffer(buf);
 		ereport(ERROR,
 				(errcode(ERRCODE_DATA_CORRUPTED),
-				 errmsg("heap tuple (%u,%u) from table \"%s\" lacks matching index tuple within index \"%s\"",
+				 errmsg("heap tuple (%u,%u) from table \"%s\" lacks matching index tuple within index \"%s\" xmin %d xmax %d",
 						ItemPointerGetBlockNumber(&(itup->t_tid)),
 						ItemPointerGetOffsetNumber(&(itup->t_tid)),
 						RelationGetRelationName(state->heaprel),
-						RelationGetRelationName(state->rel)),
+						RelationGetRelationName(state->rel),
+						HeapTupleHeaderGetRawXmin(tuphdr),
+						HeapTupleHeaderGetRawXmax(tuphdr)),
 				 !state->readonly
 				 ? errhint("Retrying verification using the function bt_index_parent_check() might provide a more specific error.")
 				 : 0));
+	}
 
 	state->heaptuplespresent++;
 	pfree(itup);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 76774dce06..51cc484e42 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1536,7 +1536,6 @@ DefineIndex(Oid relationId,
 	 */
 	pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 								 PROGRESS_CREATEIDX_PHASE_WAIT_2);
-	WaitForLockers(heaplocktag, ShareLock, true);
 
 	/*
 	 * Now take the "reference snapshot" that will be used by validate_index()
@@ -1556,6 +1555,8 @@ DefineIndex(Oid relationId,
 	snapshot = RegisterSnapshot(GetTransactionSnapshot());
 	PushActiveSnapshot(snapshot);
 
+	WaitForLockers(heaplocktag, ShareLock, true);
+
 	/*
 	 * Scan the index and the heap, insert any missing index entries.
 	 */
@@ -3519,6 +3520,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 	 * for efficiency.
 	 */
 
+	elog(WARNING, "Phase 1");
 	/*
 	 * Phase 1 of REINDEX CONCURRENTLY
 	 *
@@ -3669,6 +3671,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 	 * to set the PROC_IN_SAFE_IC flag here.
 	 */
 
+	elog(WARNING, "Phase 2");
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3730,6 +3733,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 	 * need to set the PROC_IN_SAFE_IC flag here.
 	 */
 
+	elog(WARNING, "Phase 3");
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3816,6 +3820,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 		CommitTransactionCommand();
 	}
 
+	elog(WARNING, "Phase 4");
 	/*
 	 * Phase 4 of REINDEX CONCURRENTLY
 	 *
@@ -3889,6 +3894,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 	 * done, and that lasts for a very short period.
 	 */
 
+	elog(WARNING, "Phase 5");
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3925,6 +3931,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 	 * done, and that lasts for a very short period.
 	 */
 
+	elog(WARNING, "Phase 6");
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
@@ -3962,6 +3969,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 
 	PopActiveSnapshot();
 	CommitTransactionCommand();
+	elog(WARNING, "Phase Final");
 
 	/*
 	 * Finally, release the session-level lock on the table.
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 55efb33a7b..3644d3f4cf 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -3019,13 +3019,13 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
 				GET_VXID_FROM_PGPROC(vxid, *proc);
 
 				/* Prefer real Xid over local Xid */
-				if (TransactionIdIsValid(proc->xid))
+				/*if (TransactionIdIsValid(proc->xid))
 				{
 					vxids[count].backendId = InvalidBackendId;
 					vxids[count].localTransactionId = proc->xid;
 					count++;
 				}
-				else if (VirtualTransactionIdIsValid(vxid))
+				else */if (VirtualTransactionIdIsValid(vxid))
 					vxids[count++] = vxid;
 				/* else, xact already committed or aborted */
 
@@ -3087,13 +3087,13 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
 				GET_VXID_FROM_PGPROC(vxid, *proc);
 
 				/* Prefer real Xid over local Xid */
-				if (TransactionIdIsValid(proc->xid))
+				/*if (TransactionIdIsValid(proc->xid))
 				{
 					vxids[count].backendId = InvalidBackendId;
 					vxids[count].localTransactionId = proc->xid;
 					count++;
 				}
-				else if (VirtualTransactionIdIsValid(vxid))
+				else */if (VirtualTransactionIdIsValid(vxid))
 				{
 					int			i;
 
@@ -4468,7 +4468,7 @@ VirtualXactLockTableCleanup(void)
  *		Wait for xid completition if have xid. Otherwise try to find xid among
  *		fake procarray entries.
  */
-static bool PreparedXactLock(VirtualTransactionId vxid, TransactionId xid, bool wait)
+static bool PreparedXactLock(VirtualTransactionId vxid, TransactionId xidx, bool wait)
 {
 	LockAcquireResult	lar;
 	LOCKTAG				tag;
@@ -4477,10 +4477,11 @@ static bool PreparedXactLock(VirtualTransactionId vxid, TransactionId xid, bool
 	/* Questionable heuristics */
 	if (max_prepared_xacts == 0)
 		return true;
+	elog(WARNING,"XXX: PreparedXactLock xid %d, vxid %d/%d",xidx, vxid.backendId, vxid.localTransactionId);	// Remove this line
 
-	if (TransactionIdIsValid(xid))
+	if (TransactionIdIsValid(xidx))
 	{
-		xidlist.xid = xid;
+		xidlist.xid = xidx;
 		xidlist.next = NULL;
 	}
 	else
@@ -4488,18 +4489,19 @@ static bool PreparedXactLock(VirtualTransactionId vxid, TransactionId xid, bool
 		/* We must search for vxids in 2pc state */
 		/* XXX: O(N*N) complexity where N is number of prepared xacts */
 		xidlist = TwoPhaseGetXidByVXid(vxid);
-		elog(NOTICE,"XXX: Sucessfully found xid by vxid");	// Remove this line
+		elog(WARNING,"XXX: Sucessfully found xid by vxid %d", xidlist.xid);	// Remove this line
 	}
 
 	while (true)
 	{
 		if (TransactionIdIsValid(xidlist.xid))
 		{
-			SET_LOCKTAG_TRANSACTION(tag, xid);
+			SET_LOCKTAG_TRANSACTION(tag, xidlist.xid);
 			lar = LockAcquire(&tag, ShareLock, false, !wait);
 			if (lar != LOCKACQUIRE_NOT_AVAIL)
 				LockRelease(&tag, ShareLock, false);
-			return lar != LOCKACQUIRE_NOT_AVAIL;
+			if (lar == LOCKACQUIRE_NOT_AVAIL)
+				return false;
 		}
 		if (xidlist.next == NULL)
 			return true;
@@ -4524,6 +4526,7 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
 	TransactionId	xid = InvalidTransactionId;
 
 	Assert(VirtualTransactionIdIsValid(vxid));
+	elog(WARNING,"XXX: VirtualXactLock vxid %d/%d", vxid.backendId, vxid.localTransactionId);	// Remove this line
 
 	/*
 	 * Already prepared transactions don't hold vxid locks.  The
@@ -4544,7 +4547,10 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
 	 */
 	proc = BackendIdGetProc(vxid.backendId);
 	if (proc == NULL)
+	{
+		elog(WARNING,"Backend is gone");
 		return PreparedXactLock(vxid, InvalidTransactionId, wait);
+	}
 
 	/*
 	 * We must acquire this lock before checking the backendId and lxid
@@ -4557,12 +4563,13 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
 	if (proc->backendId != vxid.backendId
 		|| proc->fpLocalTransactionId != vxid.localTransactionId)
 	{
+		elog(WARNING,"Backend is doing something else");
 		LWLockRelease(&proc->fpInfoLock);
 		return PreparedXactLock(vxid, InvalidTransactionId, wait);
 	}
 
 	/* Save the xid to test if transaction coverted to 2pc later */
-	xid = proc->xid;
+	xid = proc->xid;	
 
 	/*
 	 * If we aren't asked to wait, there's no need to set up a lock table
@@ -4615,6 +4622,7 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
 	(void) LockAcquire(&tag, ShareLock, false, false);
 
 	LockRelease(&tag, ShareLock, false);
+	elog(WARNING,"Finally waiting on xid %d", xid);
 	return PreparedXactLock(vxid, xid, wait);;
 }
 
-- 
2.24.3 (Apple Git-128)