Fix visimap consults for unique checks during UPDATEs

src/backend/access/aocs/aocsam_handler.c

@@ -326,8 +326,8 @@ aoco_dml_finish(Relation relation, CmdType operation)
 		state->uniqueCheckDesc->blockDirectory = NULL;
 
 		/*
-		 * If this fetch is a part of an update, then we have been reusing the
-		 * visimap used by the delete half of the update, which would have
+		 * If this fetch is a part of an UPDATE, then we have been reusing the
+		 * visimapDelete used by the delete half of the UPDATE, which would have
 		 * already been cleaned up above. Clean up otherwise.
 		 */
 		if (!had_delete_desc)
@@ -336,6 +336,7 @@ aoco_dml_finish(Relation relation, CmdType operation)
 			pfree(state->uniqueCheckDesc->visimap);
 		}
 		state->uniqueCheckDesc->visimap = NULL;
+		state->uniqueCheckDesc->visiMapDelete = NULL;
 
 		pfree(state->uniqueCheckDesc);
 		state->uniqueCheckDesc = NULL;
@@ -499,18 +500,29 @@ get_or_create_unique_check_desc(Relation relation, Snapshot snapshot)
 													  relation,
 													  relation->rd_att->natts, /* numColGroups */
 													  snapshot);
+
 		/*
-		 * If this is part of an update, we need to reuse the visimap used by
-		 * the delete half of the update. This is to avoid spurious conflicts
-		 * when the key's previous and new value are identical. Using the
-		 * visimap from the delete half ensures that the visimap can recognize
-		 * any tuples deleted by us prior to this insert, within this command.
+		 * If this is part of an UPDATE, we need to reuse the visimapDelete
+		 * support structure from the delete half of the update. This is to
+		 * avoid spurious conflicts when the key's previous and new value are
+		 * identical. Using it ensures that we can recognize any tuples deleted
+		 * by us prior to this insert, within this command.
+		 *
+		 * Note: It is important that we reuse the visimapDelete structure and
+		 * not the visimap structure. This is because, when a uniqueness check
+		 * is performed as part of an UPDATE, visimap changes aren't persisted
+		 * yet (they are persisted at dml_finish() time, see
+		 * AppendOnlyVisimapDelete_Finish()). So, if we use the visimap
+		 * structure, we would not necessarily see all the changes.
 		 */
 		if (state->deleteDesc)
-			uniqueCheckDesc->visimap = &state->deleteDesc->visibilityMap;
+		{
+			uniqueCheckDesc->visiMapDelete = &state->deleteDesc->visiMapDelete;
+			uniqueCheckDesc->visimap = NULL;
+		}
 		else
 		{
-			/* Initialize the visimap */
+			/* COPY/INSERT: Initialize the visimap */
 			uniqueCheckDesc->visimap = palloc0(sizeof(AppendOnlyVisimap));
 			AppendOnlyVisimap_Init_forUniqueCheck(uniqueCheckDesc->visimap,
 												  relation,
@@ -994,8 +1006,9 @@ aoco_index_unique_check(Relation rel,
 	if (TransactionIdIsValid(snapshot->xmin) || TransactionIdIsValid(snapshot->xmax))
 		return true;
 
-	/* Now, consult the visimap */
-	visible = AppendOnlyVisimap_UniqueCheck(uniqueCheckDesc->visimap,
+	/* Now, perform a visibility check against the visimap infrastructure */
+	visible = AppendOnlyVisimap_UniqueCheck(uniqueCheckDesc->visiMapDelete,
+											uniqueCheckDesc->visimap,
 											aoTupleId,
 											snapshot);
 

src/backend/access/appendonly/appendonly_visimap.c

@@ -79,6 +79,13 @@ static void AppendOnlyVisimap_Find(
 					   AppendOnlyVisimap *visiMap,
 					   AOTupleId *tupleId);
 
+static void AppendOnlyVisimapDelete_Stash(
+						AppendOnlyVisimapDelete *visiMapDelete);
+
+static void AppendOnlyVisimapDelete_Find(
+						AppendOnlyVisimapDelete *visiMapDelete,
+						AOTupleId *aoTupleId);
+
 /*
  * Finishes the visimap operations.
  * No other function should be called with the given
@@ -238,6 +245,30 @@ AppendOnlyVisimap_Store(
 
 }
 
+/*
+ * If the tuple is not in the current visimap range, the current visimap entry
+ * is stashed away and the correct one is loaded or read from the spill file.
+ */
+void
+AppendOnlyVisimapDelete_LoadTuple(AppendOnlyVisimapDelete *visiMapDelete,
+								  AOTupleId *aoTupleId)
+{
+	Assert(visiMapDelete);
+	Assert(visiMapDelete->visiMap);
+	Assert(aoTupleId);
+
+	/* if the tuple is already covered, we are done */
+	if (AppendOnlyVisimapEntry_CoversTuple(&visiMapDelete->visiMap->visimapEntry,
+											aoTupleId))
+		return;
+
+	/* if necessary persist the current entry before moving. */
+	if (AppendOnlyVisimapEntry_HasChanged(&visiMapDelete->visiMap->visimapEntry))
+		AppendOnlyVisimapDelete_Stash(visiMapDelete);
+
+	AppendOnlyVisimapDelete_Find(visiMapDelete, aoTupleId);
+}
+
 /*
  * Deletes all visibility information for the given segment file.
  */
@@ -670,11 +701,8 @@ AppendOnlyVisimapDelete_Stash(
 
 /*
  * Hides a given tuple id.
- * If the tuple is not in the current visimap range, the current
- * visimap entry is stashed away and the correct one is loaded or
- * read from the spill file.
  *
- * Then, the bit of the tuple is set.
+ * Loads the entry for aoTupleId and sets the visibility bit for it.
  *
  * Should only be called when in-order delete of tuples can
  * be guranteed. This means that the tuples are deleted in increasing order.
@@ -685,32 +713,18 @@ AppendOnlyVisimapDelete_Stash(
 TM_Result
 AppendOnlyVisimapDelete_Hide(AppendOnlyVisimapDelete *visiMapDelete, AOTupleId *aoTupleId)
 {
-	AppendOnlyVisimap *visiMap;
-
 	Assert(visiMapDelete);
+	Assert(visiMapDelete->visiMap);
 	Assert(aoTupleId);
 
 	elogif(Debug_appendonly_print_visimap, LOG,
 		   "Append-only visi map delete: Hide tuple "
 		   "(tupleId) = %s",
 		   AOTupleIdToString(aoTupleId));
 
-	visiMap = visiMapDelete->visiMap;
-	Assert(visiMap);
+	AppendOnlyVisimapDelete_LoadTuple(visiMapDelete, aoTupleId);
 
-	if (!AppendOnlyVisimapEntry_CoversTuple(&visiMap->visimapEntry,
-											aoTupleId))
-	{
-		/* if necessary persist the current entry before moving. */
-		if (AppendOnlyVisimapEntry_HasChanged(&visiMap->visimapEntry))
-		{
-			AppendOnlyVisimapDelete_Stash(visiMapDelete);
-		}
-
-		AppendOnlyVisimapDelete_Find(visiMapDelete, aoTupleId);
-	}
-
-	return AppendOnlyVisimapEntry_HideTuple(&visiMap->visimapEntry, aoTupleId);
+	return AppendOnlyVisimapEntry_HideTuple(&visiMapDelete->visiMap->visimapEntry, aoTupleId);
 }
 
 static void
@@ -814,6 +828,36 @@ AppendOnlyVisimapDelete_WriteBackStashedEntries(AppendOnlyVisimapDelete *visiMap
 	}
 }
 
+/*
+ * Checks if the given tuple id is visible according to the visimapDelete
+ * support structure.
+ * A positive result is a necessary but not sufficient condition for
+ * a tuple to be visible to the user.
+ *
+ * Loads the entry for the tuple id before checking the bit.
+ */
+bool
+AppendOnlyVisimapDelete_IsVisible(AppendOnlyVisimapDelete *visiMapDelete,
+								  AOTupleId *aoTupleId)
+{
+	AppendOnlyVisimap *visiMap;
+
+	Assert(visiMapDelete);
+	Assert(aoTupleId);
+
+	elogif(Debug_appendonly_print_visimap, LOG,
+		   "Append-only visi map delete: IsVisible check "
+		   "(tupleId) = %s",
+		   AOTupleIdToString(aoTupleId));
+
+	visiMap = visiMapDelete->visiMap;
+	Assert(visiMap);
+
+	AppendOnlyVisimapDelete_LoadTuple(visiMapDelete, aoTupleId);
+
+	return AppendOnlyVisimapEntry_IsVisible(&visiMap->visimapEntry, aoTupleId);
+}
+
 
 /*
  * Finishes the delete operation.
@@ -914,8 +958,8 @@ AppendOnlyVisimap_Finish_forUniquenessChecks(
 {
 	AppendOnlyVisimapStore *visimapStore = &visiMap->visimapStore;
 	/*
-	 * The snapshot was either reset to NULL in between calls or already cleaned
-	 * up (if this was part of an update command)
+	 * The snapshot was never set or reset to NULL in between calls to
+	 * AppendOnlyVisimap_UniqueCheck().
 	 */
 	Assert(visimapStore->snapshot == InvalidSnapshot);
 

src/backend/access/appendonly/appendonlyam_handler.c

@@ -297,7 +297,7 @@ appendonly_dml_finish(Relation relation, CmdType operation)
 
 		/*
 		 * If this fetch is a part of an update, then we have been reusing the
-		 * visimap used by the delete half of the update, which would have
+		 * visimapDelete used by the delete half of the update, which would have
 		 * already been cleaned up above. Clean up otherwise.
 		 */
 		if (!had_delete_desc)
@@ -306,6 +306,7 @@ appendonly_dml_finish(Relation relation, CmdType operation)
 			pfree(state->uniqueCheckDesc->visimap);
 		}
 		state->uniqueCheckDesc->visimap = NULL;
+		state->uniqueCheckDesc->visiMapDelete = NULL;
 
 		pfree(state->uniqueCheckDesc);
 		state->uniqueCheckDesc = NULL;
@@ -465,17 +466,27 @@ get_or_create_unique_check_desc(Relation relation, Snapshot snapshot)
 													  snapshot);
 
 		/*
-		 * If this is part of an update, we need to reuse the visimap used by
-		 * the delete half of the update. This is to avoid spurious conflicts
-		 * when the key's previous and new value are identical. Using the
-		 * visimap from the delete half ensures that the visimap can recognize
-		 * any tuples deleted by us prior to this insert, within this command.
+		 * If this is part of an UPDATE, we need to reuse the visimapDelete
+		 * support structure from the delete half of the update. This is to
+		 * avoid spurious conflicts when the key's previous and new value are
+		 * identical. Using it ensures that we can recognize any tuples deleted
+		 * by us prior to this insert, within this command.
+		 *
+		 * Note: It is important that we reuse the visimapDelete structure and
+		 * not the visimap structure. This is because, when a uniqueness check
+		 * is performed as part of an UPDATE, visimap changes aren't persisted
+		 * yet (they are persisted at dml_finish() time, see
+		 * AppendOnlyVisimapDelete_Finish()). So, if we use the visimap
+		 * structure, we would not necessarily see all the changes.
 		 */
 		if (state->deleteDesc)
-			uniqueCheckDesc->visimap = &state->deleteDesc->visibilityMap;
+		{
+			uniqueCheckDesc->visiMapDelete = &state->deleteDesc->visiMapDelete;
+			uniqueCheckDesc->visimap = NULL;
+		}
 		else
 		{
-			/* Initialize the visimap */
+			/* COPY/INSERT: Initialize the visimap */
 			uniqueCheckDesc->visimap = palloc0(sizeof(AppendOnlyVisimap));
 			AppendOnlyVisimap_Init_forUniqueCheck(uniqueCheckDesc->visimap,
 												  relation,
@@ -761,8 +772,9 @@ appendonly_index_unique_check(Relation rel,
 	if (TransactionIdIsValid(snapshot->xmin) || TransactionIdIsValid(snapshot->xmax))
 		return true;
 
-	/* Now, consult the visimap */
-	visible = AppendOnlyVisimap_UniqueCheck(uniqueCheckDesc->visimap,
+	/* Now, perform a visibility check against the visimap infrastructure */
+	visible = AppendOnlyVisimap_UniqueCheck(uniqueCheckDesc->visiMapDelete,
+											uniqueCheckDesc->visimap,
 											aoTupleId,
 											snapshot);
 

src/include/access/appendonly_visimap.h

@@ -158,33 +158,73 @@ TM_Result AppendOnlyVisimapDelete_Hide(
 void AppendOnlyVisimapDelete_Finish(
 							   AppendOnlyVisimapDelete *visiMapDelete);
 
+void
+AppendOnlyVisimapDelete_LoadTuple(AppendOnlyVisimapDelete *visiMapDelete,
+								   AOTupleId *aoTupleId);
+
+bool
+AppendOnlyVisimapDelete_IsVisible(AppendOnlyVisimapDelete *visiMapDelete,
+								  AOTupleId *aoTupleId);
+
 /*
  * AppendOnlyVisimap_UniqueCheck
  *
  * During a uniqueness check, look up the visimap to see if a tuple was deleted
  * by a *committed* transaction.
  *
- * Note: We need to use the passed in per-tuple snapshot to perform the block
- * directory lookup. See AppendOnlyVisimap_Init_forUniqueCheck() for details on
- * why we can't set up the metadata snapshot at init time.
- * If this is part of an update, we are reusing the visimap from the delete half
- * of the update, so better restore its snapshot once we are done.
+ * If this uniqueness check is part of an UPDATE, we consult the visiMapDelete
+ * structure. Otherwise, we consult the visiMap structure. Only one of these
+ * arguments should be supplied.
  */
-static inline bool AppendOnlyVisimap_UniqueCheck(
-											AppendOnlyVisimap *visiMap,
-											AOTupleId *aoTupleId,
-											Snapshot appendOnlyMetaDataSnapshot)
+static inline bool AppendOnlyVisimap_UniqueCheck(AppendOnlyVisimapDelete *visiMapDelete,
+												 AppendOnlyVisimap *visiMap,
+												 AOTupleId *aoTupleId,
+												 Snapshot appendOnlyMetaDataSnapshot)
 {
-	Snapshot save_snapshot;
-	bool visible;
+	Snapshot          save_snapshot;
+	bool              visible;
 
 	Assert(appendOnlyMetaDataSnapshot->snapshot_type == SNAPSHOT_DIRTY ||
-			appendOnlyMetaDataSnapshot->snapshot_type == SNAPSHOT_SELF);
+			   appendOnlyMetaDataSnapshot->snapshot_type == SNAPSHOT_SELF);
+
+	if (visiMapDelete)
+	{
+		/* part of an UPDATE */
+		Assert(!visiMap);
+		Assert(visiMapDelete->visiMap);
+
+		/* Save the snapshot used for the delete half of the UPDATE */
+		save_snapshot = visiMapDelete->visiMap->visimapStore.snapshot;
+
+		/*
+		 * Replace with per-tuple snapshot meant for uniqueness checks. See
+		 * AppendOnlyVisimap_Init_forUniqueCheck() for details on why we can't
+		 * set up the metadata snapshot at init time.
+		 */
+		visiMapDelete->visiMap->visimapStore.snapshot = appendOnlyMetaDataSnapshot;
+
+		visible = AppendOnlyVisimapDelete_IsVisible(visiMapDelete, aoTupleId);
+
+		/* Restore the snapshot used for the delete half of the UPDATE */
+		visiMapDelete->visiMap->visimapStore.snapshot = save_snapshot;
+	}
+	else
+	{
+		/* part of a COPY/INSERT */
+		Assert(visiMap);
+
+		/*
+		 * Set up per-tuple snapshot meant for uniqueness checks. See
+		 * AppendOnlyVisimap_Init_forUniqueCheck() for details on why we can't
+		 * set up the metadata snapshot at init time.
+		 */
+		visiMap->visimapStore.snapshot = appendOnlyMetaDataSnapshot;
+
+		visible = AppendOnlyVisimap_IsVisible(visiMap, aoTupleId);
+
+		visiMap->visimapStore.snapshot = NULL; /* be a good citizen */
+	}
 
-	save_snapshot = visiMap->visimapStore.snapshot;
-	visiMap->visimapStore.snapshot = appendOnlyMetaDataSnapshot;
-	visible = AppendOnlyVisimap_IsVisible(visiMap, aoTupleId);
-	visiMap->visimapStore.snapshot = save_snapshot;
 	return visible;
 }
 

src/include/cdb/cdbaocsam.h

@@ -288,7 +288,10 @@ typedef struct AOCSDeleteDescData *AOCSDeleteDesc;
 typedef struct AOCSUniqueCheckDescData
 {
 	AppendOnlyBlockDirectory *blockDirectory;
+	/* visimap to check for deleted tuples as part of INSERT/COPY */
 	AppendOnlyVisimap 		 *visimap;
+	/* visimap support structure to check for deleted tuples as part of UPDATE */
+	AppendOnlyVisimapDelete  *visiMapDelete;
 } AOCSUniqueCheckDescData;
 
 typedef struct AOCSUniqueCheckDescData *AOCSUniqueCheckDesc;

src/include/cdb/cdbappendonlyam.h

@@ -402,7 +402,10 @@ typedef struct AppendOnlyDeleteDescData *AppendOnlyDeleteDesc;
 typedef struct AppendOnlyUniqueCheckDescData
 {
 	AppendOnlyBlockDirectory *blockDirectory;
+	/* visimap to check for deleted tuples as part of INSERT/COPY */
 	AppendOnlyVisimap 		 *visimap;
+	/* visimap support structure to check for deleted tuples as part of UPDATE */
+	AppendOnlyVisimapDelete  *visiMapDelete;
 } AppendOnlyUniqueCheckDescData;
 
 typedef struct AppendOnlyUniqueCheckDescData *AppendOnlyUniqueCheckDesc;