Subject: Improving the heapgetpage function improves performance in common scenarios

From: Quan Zongliang <[email-masked]>
Date: Thu, 24 Aug 2023 18:55:28 +0800

Hi

In the function heapgetpage. If a table is not updated very frequently. 
Many actions in tuple loops are superfluous. For all_visible pages, 
loctup does not need to be assigned, nor does the "valid" variable. 
CheckForSerializableConflictOutNeeded from 
HeapCheckForSerializableConflictOut function, it only need to inspect at 
the beginning of the cycle only once. Using vtune you can clearly see 
the result (attached heapgetpage.jpg).

So by splitting the loop logic into two parts, the vtune results show 
significant improvement (attached heapgetpage-allvis.jpg).

The test data uses TPC-H's table "orders" with a scale=20, 30 million rows.

Quan Zongliang

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 7ed72abe59..21ac1da8ab 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -451,30 +451,43 @@ heapgetpage(TableScanDesc sscan, BlockNumber block)
 	 */
 	all_visible = PageIsAllVisible(page) && !snapshot->takenDuringRecovery;
 
-	for (lineoff = FirstOffsetNumber; lineoff <= lines; lineoff++)
+	if (all_visible && !CheckForSerializableConflictOutNeeded(scan->rs_base.rs_rd, snapshot))
 	{
-		ItemId		lpp = PageGetItemId(page, lineoff);
-		HeapTupleData loctup;
-		bool		valid;
+		for (lineoff = FirstOffsetNumber; lineoff <= lines; lineoff++)
+		{
+			if (!ItemIdIsNormal(PageGetItemId(page, lineoff)))
+				continue;
 
-		if (!ItemIdIsNormal(lpp))
-			continue;
+			scan->rs_vistuples[ntup++] = lineoff;
+		}
+	}
+	else
+	{
+		for (lineoff = FirstOffsetNumber; lineoff <= lines; lineoff++)
+		{
+			ItemId		lpp = PageGetItemId(page, lineoff);
+			HeapTupleData loctup;
+			bool		valid;
 
-		loctup.t_tableOid = RelationGetRelid(scan->rs_base.rs_rd);
-		loctup.t_data = (HeapTupleHeader) PageGetItem(page, lpp);
-		loctup.t_len = ItemIdGetLength(lpp);
-		ItemPointerSet(&(loctup.t_self), block, lineoff);
+			if (!ItemIdIsNormal(lpp))
+				continue;
 
-		if (all_visible)
-			valid = true;
-		else
-			valid = HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer);
+			loctup.t_tableOid = RelationGetRelid(scan->rs_base.rs_rd);
+			loctup.t_data = (HeapTupleHeader) PageGetItem(page, lpp);
+			loctup.t_len = ItemIdGetLength(lpp);
+			ItemPointerSet(&(loctup.t_self), block, lineoff);
+
+			if (all_visible)
+				valid = true;
+			else
+				valid = HeapTupleSatisfiesVisibility(&loctup, snapshot, buffer);
 
-		HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
-											&loctup, buffer, snapshot);
+			HeapCheckForSerializableConflictOut(valid, scan->rs_base.rs_rd,
+												&loctup, buffer, snapshot);
 
-		if (valid)
-			scan->rs_vistuples[ntup++] = lineoff;
+			if (valid)
+				scan->rs_vistuples[ntup++] = lineoff;
+		}
 	}
 
 	LockBuffer(buffer, BUFFER_LOCK_UNLOCK);

---

From: John Naylor <[email-masked]>
Date: Tue, 5 Sep 2023 15:15:55 +0700

> HeapCheckForSerializableConflictOut function, it only need to inspect at

Thanks for submitting! A few weeks before this, there was another proposal,
which specializes code for all paths, not just one. That patch also does so
without duplicating the loop:

https://www.postgresql.org/message-id/[email-masked]

> significant improvement (attached heapgetpage-allvis.jpg).

For future reference, it's not clear at all from the screenshots what the
improvement will be for the user. In the above thread, the author shares
testing methodology as well as timing measurements. This is useful for
reproducibilty, as well as convincing others that the change is important.

--
John Naylor
EDB: http://www.enterprisedb.com

---

From: Quan Zongliang <[email-masked]>
Date: Tue, 5 Sep 2023 17:27:03 +0800

On 2023/9/5 16:15, John Naylor wrote:
> 
> On Thu, Aug 24, 2023 at 5:55 PM Quan Zongliang <[email-masked] 
> <mailto:[email-masked]>> wrote:
> 
> Thanks for submitting! A few weeks before this, there was another 
> proposal, which specializes code for all paths, not just one. That patch 
> also does so without duplicating the loop:
> 
> https://www.postgresql.org/message-id/[email-masked] <https://www.postgresql.org/message-id/[email-masked]>
> 
Nice patch. I'm sorry I didn't notice it before.

> For future reference, it's not clear at all from the screenshots what 
> the improvement will be for the user. In the above thread, the author 
> shares testing methodology as well as timing measurements. This is 
> useful for reproducibilty, as well as convincing others that the change 
> is important.
> 
Here's how I test it
    EXPLAIN ANALYZE SELECT * FROM orders;
Maybe the test wasn't good enough. Although the modified optimal result 
looks good. Because it fluctuates a lot. It's hard to compare. The 
results of vtune are therefore used.

My patch is mainly to eliminate:
1, Assignment of "loctup" struct variable (in vtune you can see that 
these 4 lines have a significant overhead: 0.4 1.0 0.2 0.4).
2. Assignment of the "valid" variable.(overhead 0.6)
3. HeapCheckForSerializableConflictOut function call.(overhead 0.6)

Although these are not the same overhead from test to test. But all are 
too obvious to ignore. The screenshots are mainly to show the three 
improvements mentioned above.

I'll also try Andres Freund's test method next.

> --
> John Naylor
> EDB: http://www.enterprisedb.com <http://www.enterprisedb.com>