Subject: Re: ALTER TABLE uses a bistate but not for toast tables

From: Justin Pryzby <[email-masked]>
Date: Mon, 15 Jul 2024 15:43:24 -0500

@cfbot: rebased
>From bc92a05ba35115ba0df278d553a5c0e4e303fe23 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Tue, 21 Jun 2022 22:28:06 -0500
Subject: [PATCH] WIP: use BulkInsertState for toast tuples, too

DONE: ALTER, CLUSTER
TODO: copyto, copyfrom?

slot_getsomeattrs slot_deform_heap_tuple fetchatt
heap_getnextslot => heapgettup => heapgetpage => ReadBufferExtended
initscan
table_beginscan table_scan_getnextslot
RelationCopyStorageUsingBuffer ReadBufferWithoutRelcache

(gdb) bt
 #0  table_open (relationId=relationId@entry=16390, lockmode=lockmode@entry=1) at table.c:40
 #1  0x000056444cb23d3c in toast_fetch_datum (attr=attr@entry=0x7f67933cc6cc) at detoast.c:372
 #2  0x000056444cb24217 in detoast_attr (attr=attr@entry=0x7f67933cc6cc) at detoast.c:123
 #3  0x000056444d07a4c8 in pg_detoast_datum_packed (datum=datum@entry=0x7f67933cc6cc) at fmgr.c:1743
 #4  0x000056444d042c8d in text_to_cstring (t=0x7f67933cc6cc) at varlena.c:224
 #5  0x000056444d0434f9 in textout (fcinfo=<optimized out>) at varlena.c:573
 #6  0x000056444d078f10 in FunctionCall1Coll (flinfo=flinfo@entry=0x56444e4706b0, collation=collation@entry=0, arg1=arg1@entry=140082828592844) at fmgr.c:1124
 #7  0x000056444d079d7f in OutputFunctionCall (flinfo=flinfo@entry=0x56444e4706b0, val=val@entry=140082828592844) at fmgr.c:1561
 #8  0x000056444ccb1665 in CopyOneRowTo (cstate=cstate@entry=0x56444e470898, slot=slot@entry=0x56444e396d20) at copyto.c:975
 #9  0x000056444ccb2c7d in DoCopyTo (cstate=cstate@entry=0x56444e470898) at copyto.c:891
 #10 0x000056444ccab4c2 in DoCopy (pstate=pstate@entry=0x56444e396bb0, stmt=stmt@entry=0x56444e3759b0, stmt_location=0, stmt_len=48, processed=processed@entry=0x7ffc212a6310) at copy.c:308

cluster:
heapam_relation_copy_for_cluster
reform_and_rewrite_tuple
rewrite_heap_tuple
raw_heap_insert

//-os-only: linux
---
 src/backend/access/common/toast_internals.c |  6 ++++--
 src/backend/access/heap/heapam.c            | 24 +++++++++++++++------
 src/backend/access/heap/heaptoast.c         | 11 ++++++----
 src/backend/access/heap/rewriteheap.c       |  8 +++++--
 src/backend/access/table/toast_helper.c     |  6 ++++--
 src/include/access/heaptoast.h              |  4 +++-
 src/include/access/hio.h                    |  2 ++
 src/include/access/toast_helper.h           |  3 ++-
 src/include/access/toast_internals.h        |  4 +++-
 9 files changed, 49 insertions(+), 19 deletions(-)

diff --git a/src/backend/access/common/toast_internals.c b/src/backend/access/common/toast_internals.c
index 90d0654e629..b66d32a26e3 100644
--- a/src/backend/access/common/toast_internals.c
+++ b/src/backend/access/common/toast_internals.c
@@ -117,7 +117,8 @@ toast_compress_datum(Datum value, char cmethod)
  */
 Datum
 toast_save_datum(Relation rel, Datum value,
-				 struct varlena *oldexternal, int options)
+				 struct varlena *oldexternal, int options,
+				 BulkInsertState bistate)
 {
 	Relation	toastrel;
 	Relation   *toastidxs;
@@ -320,7 +321,8 @@ toast_save_datum(Relation rel, Datum value,
 		memcpy(VARDATA(&chunk_data), data_p, chunk_size);
 		toasttup = heap_form_tuple(toasttupDesc, t_values, t_isnull);
 
-		heap_insert(toastrel, toasttup, mycid, options, NULL);
+		heap_insert(toastrel, toasttup, mycid, options, bistate ?
+					bistate->toast_state : NULL);
 
 		/*
 		 * Create the index entry.  We cheat a little here by not using
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 91b20147a00..18b05d46735 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -71,7 +71,8 @@

 static HeapTuple heap_prepare_insert(Relation relation, HeapTuple tup,
-									 TransactionId xid, CommandId cid, int options);
+									 TransactionId xid, CommandId cid, int options,
+									 BulkInsertState bistate);
 static XLogRecPtr log_heap_update(Relation reln, Buffer oldbuf,
 								  Buffer newbuf, HeapTuple oldtup,
 								  HeapTuple newtup, HeapTuple old_key_tuple,
@@ -1931,9 +1932,15 @@ GetBulkInsertState(void)
 	bistate = (BulkInsertState) palloc(sizeof(BulkInsertStateData));
 	bistate->strategy = GetAccessStrategy(BAS_BULKWRITE);
 	bistate->current_buf = InvalidBuffer;
+
 	bistate->next_free = InvalidBlockNumber;
 	bistate->last_free = InvalidBlockNumber;
 	bistate->already_extended_by = 0;
+
+	bistate->toast_state = (BulkInsertState) palloc(sizeof(BulkInsertStateData));
+	bistate->toast_state->strategy = GetAccessStrategy(BAS_BULKWRITE);
+	bistate->toast_state->current_buf = InvalidBuffer;
+
 	return bistate;
 }
 
@@ -1945,6 +1952,10 @@ FreeBulkInsertState(BulkInsertState bistate)
 {
 	if (bistate->current_buf != InvalidBuffer)
 		ReleaseBuffer(bistate->current_buf);
+	if (bistate->toast_state->current_buf != InvalidBuffer)
+		ReleaseBuffer(bistate->toast_state->current_buf);
+
+	FreeAccessStrategy(bistate->toast_state->strategy);
 	FreeAccessStrategy(bistate->strategy);
 	pfree(bistate);
 }
@@ -2010,7 +2021,8 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
 	 * Note: below this point, heaptup is the data we actually intend to store
 	 * into the relation; tup is the caller's original untoasted data.
 	 */
-	heaptup = heap_prepare_insert(relation, tup, xid, cid, options);
+
+	heaptup = heap_prepare_insert(relation, tup, xid, cid, options, bistate);
 
 	/*
 	 * Find buffer to insert this tuple into.  If the page is all visible,
@@ -2181,7 +2193,7 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
  */
 static HeapTuple
 heap_prepare_insert(Relation relation, HeapTuple tup, TransactionId xid,
-					CommandId cid, int options)
+					CommandId cid, int options, BulkInsertState bistate)
 {
 	/*
 	 * To allow parallel inserts, we need to ensure that they are safe to be
@@ -2217,7 +2229,7 @@ heap_prepare_insert(Relation relation, HeapTuple tup, TransactionId xid,
 		return tup;
 	}
 	else if (HeapTupleHasExternal(tup) || tup->t_len > TOAST_TUPLE_THRESHOLD)
-		return heap_toast_insert_or_update(relation, tup, NULL, options);
+		return heap_toast_insert_or_update(relation, tup, NULL, options, bistate);
 	else
 		return tup;
 }
@@ -2295,7 +2307,7 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
 		slots[i]->tts_tableOid = RelationGetRelid(relation);
 		tuple->t_tableOid = slots[i]->tts_tableOid;
 		heaptuples[i] = heap_prepare_insert(relation, tuple, xid, cid,
-											options);
+											options, NULL);
 	}
 
 	/*
@@ -3767,7 +3779,7 @@ l2:
 		if (need_toast)
 		{
 			/* Note we always use WAL and FSM during updates */
-			heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, 0);
+			heaptup = heap_toast_insert_or_update(relation, newtup, &oldtup, 0, NULL);
 			newtupsize = MAXALIGN(heaptup->t_len);
 		}
 		else
diff --git a/src/backend/access/heap/heaptoast.c b/src/backend/access/heap/heaptoast.c
index a420e165304..bcb012c579d 100644
--- a/src/backend/access/heap/heaptoast.c
+++ b/src/backend/access/heap/heaptoast.c
@@ -94,7 +94,7 @@ heap_toast_delete(Relation rel, HeapTuple oldtup, bool is_speculative)
  */
 HeapTuple
 heap_toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
-							int options)
+							int options, BulkInsertState bistate)
 {
 	HeapTuple	result_tuple;
 	TupleDesc	tupleDesc;
@@ -110,6 +110,9 @@ heap_toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
 	ToastAttrInfo toast_attr[MaxHeapAttributeNumber];
 	ToastTupleContext ttc;
 
+	/* Bulk insert is not supported for updates, only inserts. */
+	Assert(bistate == NULL || oldtup == NULL);
+
 	/*
 	 * Ignore the INSERT_SPECULATIVE option. Speculative insertions/super
 	 * deletions just normally insert/delete the toast values. It seems
@@ -214,7 +217,7 @@ heap_toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
 		 */
 		if (toast_attr[biggest_attno].tai_size > maxDataLen &&
 			rel->rd_rel->reltoastrelid != InvalidOid)
-			toast_tuple_externalize(&ttc, biggest_attno, options);
+			toast_tuple_externalize(&ttc, biggest_attno, options, bistate);
 	}
 
 	/*
@@ -231,7 +234,7 @@ heap_toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
 		biggest_attno = toast_tuple_find_biggest_attribute(&ttc, false, false);
 		if (biggest_attno < 0)
 			break;
-		toast_tuple_externalize(&ttc, biggest_attno, options);
+		toast_tuple_externalize(&ttc, biggest_attno, options, bistate);
 	}
 
 	/*
@@ -267,7 +270,7 @@ heap_toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
 		if (biggest_attno < 0)
 			break;
 
-		toast_tuple_externalize(&ttc, biggest_attno, options);
+		toast_tuple_externalize(&ttc, biggest_attno, options, bistate);
 	}
 
 	/*
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 473f3aa9bef..d313ad64ee7 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -150,6 +150,7 @@ typedef struct RewriteStateData
 	HTAB	   *rs_old_new_tid_map; /* unmatched B tuples */
 	HTAB	   *rs_logical_mappings;	/* logical remapping files */
 	uint32		rs_num_rewrite_mappings;	/* # in memory mappings */
+	BulkInsertState rs_bistate;
 }			RewriteStateData;
 
 /*
@@ -260,7 +261,8 @@ begin_heap_rewrite(Relation old_heap, Relation new_heap, TransactionId oldest_xm
 	state->rs_freeze_xid = freeze_xid;
 	state->rs_cutoff_multi = cutoff_multi;
 	state->rs_cxt = rw_cxt;
-	state->rs_bulkstate = smgr_bulk_start_rel(new_heap, MAIN_FORKNUM);
+	state->rs_bulkstate = smgr_bulk_start_rel(new_heap, MAIN_FORKNUM); /* This is a BulkWriteState */
+	state->rs_bistate = GetBulkInsertState(); /* This is a BulkInsertState ... */
 
 	/* Initialize hash tables used to track update chains */
 	hash_ctl.keysize = sizeof(TidHashKey);
@@ -311,6 +313,8 @@ end_heap_rewrite(RewriteState state)
 		raw_heap_insert(state, unresolved->tuple);
 	}
 
+	FreeBulkInsertState(state->rs_bistate);
+
 	/* Write the last page, if any */
 	if (state->rs_buffer)
 	{
@@ -624,7 +628,7 @@ raw_heap_insert(RewriteState state, HeapTuple tup)
 		options |= HEAP_INSERT_NO_LOGICAL;
 
 		heaptup = heap_toast_insert_or_update(state->rs_new_rel, tup, NULL,
-											  options);
+											  options, state->rs_bistate);
 	}
 	else
 		heaptup = tup;
diff --git a/src/backend/access/table/toast_helper.c b/src/backend/access/table/toast_helper.c
index 53224932f0d..2adf2dfaa9a 100644
--- a/src/backend/access/table/toast_helper.c
+++ b/src/backend/access/table/toast_helper.c
@@ -15,6 +15,7 @@
 #include "postgres.h"
 
 #include "access/detoast.h"
+#include "access/hio.h"
 #include "access/toast_helper.h"
 #include "access/toast_internals.h"
 #include "catalog/pg_type_d.h"
@@ -253,7 +254,8 @@ toast_tuple_try_compression(ToastTupleContext *ttc, int attribute)
  * Move an attribute to external storage.
  */
 void
-toast_tuple_externalize(ToastTupleContext *ttc, int attribute, int options)
+toast_tuple_externalize(ToastTupleContext *ttc, int attribute, int options,
+						BulkInsertStateData *bistate)
 {
 	Datum	   *value = &ttc->ttc_values[attribute];
 	Datum		old_value = *value;
@@ -261,7 +263,7 @@ toast_tuple_externalize(ToastTupleContext *ttc, int attribute, int options)
 
 	attr->tai_colflags |= TOASTCOL_IGNORE;
 	*value = toast_save_datum(ttc->ttc_rel, old_value, attr->tai_oldexternal,
-							  options);
+							  options, bistate);
 	if ((attr->tai_colflags & TOASTCOL_NEEDS_FREE) != 0)
 		pfree(DatumGetPointer(old_value));
 	attr->tai_colflags |= TOASTCOL_NEEDS_FREE;
diff --git a/src/include/access/heaptoast.h b/src/include/access/heaptoast.h
index c376dff48d7..a993aea5857 100644
--- a/src/include/access/heaptoast.h
+++ b/src/include/access/heaptoast.h
@@ -13,6 +13,7 @@
 #ifndef HEAPTOAST_H
 #define HEAPTOAST_H
 
+#include "access/hio.h"
 #include "access/htup_details.h"
 #include "storage/lockdefs.h"
 #include "utils/relcache.h"
@@ -95,7 +96,8 @@
  * ----------
  */
 extern HeapTuple heap_toast_insert_or_update(Relation rel, HeapTuple newtup,
-											 HeapTuple oldtup, int options);
+											 HeapTuple oldtup, int options,
+											 BulkInsertStateData *bistate);
 
 /* ----------
  * heap_toast_delete -
diff --git a/src/include/access/hio.h b/src/include/access/hio.h
index 24621cf22b2..e47fefe2ab9 100644
--- a/src/include/access/hio.h
+++ b/src/include/access/hio.h
@@ -48,6 +48,8 @@ typedef struct BulkInsertStateData
 	BlockNumber next_free;
 	BlockNumber last_free;
 	uint32		already_extended_by;
+
+	struct BulkInsertStateData *toast_state;
 } BulkInsertStateData;

diff --git a/src/include/access/toast_helper.h b/src/include/access/toast_helper.h
index 349a513f77d..04ff56b7cc7 100644
--- a/src/include/access/toast_helper.h
+++ b/src/include/access/toast_helper.h
@@ -14,6 +14,7 @@
 #ifndef TOAST_HELPER_H
 #define TOAST_HELPER_H
 
+#include "access/hio.h"
 #include "utils/rel.h"
 
 /*
@@ -107,7 +108,7 @@ extern int	toast_tuple_find_biggest_attribute(ToastTupleContext *ttc,
 											   bool check_main);
 extern void toast_tuple_try_compression(ToastTupleContext *ttc, int attribute);
 extern void toast_tuple_externalize(ToastTupleContext *ttc, int attribute,
-									int options);
+									int options, BulkInsertStateData *bistate);
 extern void toast_tuple_cleanup(ToastTupleContext *ttc);
 
 extern void toast_delete_external(Relation rel, const Datum *values, const bool *isnull,
diff --git a/src/include/access/toast_internals.h b/src/include/access/toast_internals.h
index 0eeefe59fec..1be114bf24f 100644
--- a/src/include/access/toast_internals.h
+++ b/src/include/access/toast_internals.h
@@ -12,6 +12,7 @@
 #ifndef TOAST_INTERNALS_H
 #define TOAST_INTERNALS_H
 
+#include "access/hio.h"
 #include "access/toast_compression.h"
 #include "storage/lockdefs.h"
 #include "utils/relcache.h"
@@ -50,7 +51,8 @@ extern Oid	toast_get_valid_index(Oid toastoid, LOCKMODE lock);
 
 extern void toast_delete_datum(Relation rel, Datum value, bool is_speculative);
 extern Datum toast_save_datum(Relation rel, Datum value,
-							  struct varlena *oldexternal, int options);
+							  struct varlena *oldexternal, int options,
+							  BulkInsertStateData *bistate);
 
 extern int	toast_open_indexes(Relation toastrel,
 							   LOCKMODE lock,
-- 
2.42.0

---

From: Dmitry Dolgov <[email-masked]>
Date: Tue, 19 Nov 2024 15:45:19 +0100

> On Mon, Jul 15, 2024 at 03:43:24PM GMT, Justin Pryzby wrote:
> @cfbot: rebased

Hey Justin,

Thanks for rebasing. To help with review, could you also describe
current status of the patch? I have to admit, currently the commit
message doesn't tell much, and looks more like notes for the future you.
The patch numbering is somewhat confusing as well, should it be v5 now?
>From what I understand, the new patch does address the review feedback,
but you want to do more, something with copy to / copy from?

Since it's in the performance category, I'm also curious how much
overhead does this shave off? I mean, I get it that bulk insert strategy
helps with buffers usage, as you've implied in the thread -- but how
does it look like in benchmark numbers?

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 20 Nov 2024 06:43:58 -0600

> message doesn't tell much, and looks more like notes for the future you.

The patch does what it aims to do and AFAIK in a reasonable way.  I'm
not aware of any issue with it.  It's, uh, waiting for review.

I'm happy to expand on the message to describe something like design
choices, but the goal here is really simple: why should wide column
values escape the intention of the ring buffer?  AFAICT it's fixing an
omission.  If you have a question, please ask; that would help to
indicate what needs to be explained.

> The patch numbering is somewhat confusing as well, should it be v5 now?

The filename was 0001-WIP-use-BulkInsertState-for-toast-tuples-too.patch.
I guess you're referring to the previous filename: v4-*.
That shouldn't be so confusing -- I just didn't specify a version,
either by choice or by omission.

> but you want to do more, something with copy to / copy from?

If I were to do more, it'd be for a future patch, if the current patch
were to ever progress.

> does it look like in benchmark numbers?

The intent of using a bistate isn't to help the performance of the
process using the bistate.  Rather, the intent is to avoid harming the
performance of other processes.  If anything, I expect it could slow
down the process using bistate -- the same as for non-toast data.

https://www.postgresql.org/message-id/CA%2BTgmobC6RD2N8kbPPTvATpUY1kisY2wJLh2jsg%3DHGoCp2RiXw%40mail.gmail.com

-- 
Justin

---

From: Dmitry Dolgov <[email-masked]>
Date: Wed, 20 Nov 2024 21:11:00 +0100

> On Wed, Nov 20, 2024 at 06:43:58AM -0600, Justin Pryzby wrote:
>

Here is what I see in the commit message:

    DONE: ALTER, CLUSTER
    TODO: copyto, copyfrom?

    (gdb) bt

    cluster:

This gave me an impression, that the patch is deeply WIP, and it doesn't
make any sense to review it. I can imagine chances are good, that I'm
not alone who get such impression, and you loose potential reviewers.
Thus, shaping up a meaningful message might be helpful.

> https://www.postgresql.org/message-id/CA%2BTgmobC6RD2N8kbPPTvATpUY1kisY2wJLh2jsg%3DHGoCp2RiXw%40mail.gmail.com

Right, but the question is still there, how much does it bring? My point
is that if you demonstrate "under this and that load, we get so and so
many percents boost", this will hopefully attract more attention to the
patch.