stringinfo_changes_v5.patch

text/plain

Filename: stringinfo_changes_v5.patch
Type: text/plain
Part: 0
Message: Re: Making aggregate deserialization (and WAL receive) functions slightly faster

Patch

Format: unified
Series: patch v5
File+
src/backend/replication/logical/applyparallelworker.c 1 4
src/backend/replication/logical/proto.c 9 9
src/backend/replication/logical/worker.c 1 4
src/backend/tcop/postgres.c 8 13
src/backend/utils/adt/arrayfuncs.c 3 14
src/backend/utils/adt/array_userfuncs.c 4 14
src/backend/utils/adt/rowtypes.c 7 16
src/common/stringinfo.c 9 0
src/include/lib/stringinfo.h 82 9
diff --git a/src/backend/replication/logical/applyparallelworker.c b/src/backend/replication/logical/applyparallelworker.c
index 82f48a488e..9b37736f8e 100644
--- a/src/backend/replication/logical/applyparallelworker.c
+++ b/src/backend/replication/logical/applyparallelworker.c
@@ -774,10 +774,7 @@ LogicalParallelApplyLoop(shm_mq_handle *mqh)
 			if (len == 0)
 				elog(ERROR, "invalid message length");
 
-			s.cursor = 0;
-			s.maxlen = -1;
-			s.data = (char *) data;
-			s.len = len;
+			initReadOnlyStringInfo(&s, data, len);
 
 			/*
 			 * The first byte of messages sent from leader apply worker to
diff --git a/src/backend/replication/logical/proto.c b/src/backend/replication/logical/proto.c
index d52c8963eb..4eaeb14148 100644
--- a/src/backend/replication/logical/proto.c
+++ b/src/backend/replication/logical/proto.c
@@ -879,6 +879,7 @@ logicalrep_read_tuple(StringInfo in, LogicalRepTupleData *tuple)
 	/* Read the data */
 	for (i = 0; i < natts; i++)
 	{
+		char	   *buff;
 		char		kind;
 		int			len;
 		StringInfo	value = &tuple->colvalues[i];
@@ -899,19 +900,18 @@ logicalrep_read_tuple(StringInfo in, LogicalRepTupleData *tuple)
 				len = pq_getmsgint(in, 4);	/* read length */
 
 				/* and data */
-				value->data = palloc(len + 1);
-				pq_copymsgbytes(in, value->data, len);
+				buff = palloc(len + 1);
+				pq_copymsgbytes(in, buff, len);
 
 				/*
-				 * Not strictly necessary for LOGICALREP_COLUMN_BINARY, but
-				 * per StringInfo practice.
+				 * NUL termination is required for LOGICALREP_COLUMN_TEXT mode
+				 * as input function require that.  For
+				 * LOGICALREP_COLUMN_BINARY it's not technically required, but
+				 * it's harmless.
 				 */
-				value->data[len] = '\0';
+				buff[len] = '\0';
 
-				/* make StringInfo fully valid */
-				value->len = len;
-				value->cursor = 0;
-				value->maxlen = len;
+				initStringInfoFromString(value, buff, len);
 				break;
 			default:
 				elog(ERROR, "unrecognized data representation type '%c'", kind);
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 54c14495be..567485c4a4 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -3582,10 +3582,7 @@ LogicalRepApplyLoop(XLogRecPtr last_received)
 					/* Ensure we are reading the data into our memory context. */
 					MemoryContextSwitchTo(ApplyMessageContext);
 
-					s.data = buf;
-					s.len = len;
-					s.cursor = 0;
-					s.maxlen = -1;
+					initReadOnlyStringInfo(&s, buf, len);
 
 					c = pq_getmsgbyte(&s);
 
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index c900427ecf..cb68b8efaf 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1817,23 +1817,18 @@ exec_bind_message(StringInfo input_message)
 
 			if (!isNull)
 			{
-				const char *pvalue = pq_getmsgbytes(input_message, plength);
+				char	   *pvalue;
 
 				/*
-				 * Rather than copying data around, we just set up a phony
+				 * Rather than copying data around, we just initialize a
 				 * StringInfo pointing to the correct portion of the message
-				 * buffer.  We assume we can scribble on the message buffer so
-				 * as to maintain the convention that StringInfos have a
-				 * trailing null.  This is grotty but is a big win when
-				 * dealing with very large parameter strings.
+				 * buffer.  We must NUL terminate the string for the sake of
+				 * the input function call below.
 				 */
-				pbuf.data = unconstify(char *, pvalue);
-				pbuf.maxlen = plength + 1;
-				pbuf.len = plength;
-				pbuf.cursor = 0;
-
-				csave = pbuf.data[plength];
-				pbuf.data[plength] = '\0';
+				pvalue = unconstify(char *, pq_getmsgbytes(input_message, plength));
+				csave = pvalue[plength];
+				pvalue[plength] = '\0';
+				initReadOnlyStringInfo(&pbuf, pvalue, plength);
 			}
 			else
 			{
diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/adt/array_userfuncs.c
index 5c4fdcfba4..c831a9395c 100644
--- a/src/backend/utils/adt/array_userfuncs.c
+++ b/src/backend/utils/adt/array_userfuncs.c
@@ -784,7 +784,6 @@ array_agg_deserialize(PG_FUNCTION_ARGS)
 		{
 			int			itemlen;
 			StringInfoData elem_buf;
-			char		csave;
 
 			if (result->dnulls[i])
 			{
@@ -799,28 +798,19 @@ array_agg_deserialize(PG_FUNCTION_ARGS)
 						 errmsg("insufficient data left in message")));
 
 			/*
-			 * Rather than copying data around, we just set up a phony
-			 * StringInfo pointing to the correct portion of the input buffer.
-			 * We assume we can scribble on the input buffer so as to maintain
-			 * the convention that StringInfos have a trailing null.
+			 * Rather than copying data around, we just initialize a
+			 * StringInfo pointing to the correct portion of the message
+			 * buffer.
 			 */
-			elem_buf.data = &buf.data[buf.cursor];
-			elem_buf.maxlen = itemlen + 1;
-			elem_buf.len = itemlen;
-			elem_buf.cursor = 0;
+			initReadOnlyStringInfo(&elem_buf, &buf.data[buf.cursor], itemlen);
 
 			buf.cursor += itemlen;
 
-			csave = buf.data[buf.cursor];
-			buf.data[buf.cursor] = '\0';
-
 			/* Now call the element's receiveproc */
 			result->dvalues[i] = ReceiveFunctionCall(&iodata->typreceive,
 													 &elem_buf,
 													 iodata->typioparam,
 													 -1);
-
-			buf.data[buf.cursor] = csave;
 		}
 	}
 
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 7828a6264b..6a920a02b7 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -1475,7 +1475,6 @@ ReadArrayBinary(StringInfo buf,
 	{
 		int			itemlen;
 		StringInfoData elem_buf;
-		char		csave;
 
 		/* Get and check the item length */
 		itemlen = pq_getmsgint(buf, 4);
@@ -1494,21 +1493,13 @@ ReadArrayBinary(StringInfo buf,
 		}
 
 		/*
-		 * Rather than copying data around, we just set up a phony StringInfo
-		 * pointing to the correct portion of the input buffer. We assume we
-		 * can scribble on the input buffer so as to maintain the convention
-		 * that StringInfos have a trailing null.
+		 * Rather than copying data around, we just initialize a StringInfo
+		 * pointing to the correct portion of the message buffer.
 		 */
-		elem_buf.data = &buf->data[buf->cursor];
-		elem_buf.maxlen = itemlen + 1;
-		elem_buf.len = itemlen;
-		elem_buf.cursor = 0;
+		initReadOnlyStringInfo(&elem_buf, &buf->data[buf->cursor], itemlen);
 
 		buf->cursor += itemlen;
 
-		csave = buf->data[buf->cursor];
-		buf->data[buf->cursor] = '\0';
-
 		/* Now call the element's receiveproc */
 		values[i] = ReceiveFunctionCall(receiveproc, &elem_buf,
 										typioparam, typmod);
@@ -1520,8 +1511,6 @@ ReadArrayBinary(StringInfo buf,
 					(errcode(ERRCODE_INVALID_BINARY_REPRESENTATION),
 					 errmsg("improper binary format in array element %d",
 							i + 1)));
-
-		buf->data[buf->cursor] = csave;
 	}
 
 	/*
diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c
index ad176651d8..eb8fe95933 100644
--- a/src/backend/utils/adt/rowtypes.c
+++ b/src/backend/utils/adt/rowtypes.c
@@ -569,7 +569,6 @@ record_recv(PG_FUNCTION_ARGS)
 		int			itemlen;
 		StringInfoData item_buf;
 		StringInfo	bufptr;
-		char		csave;
 
 		/* Ignore dropped columns in datatype, but fill with nulls */
 		if (att->attisdropped)
@@ -619,25 +618,19 @@ record_recv(PG_FUNCTION_ARGS)
 			/* -1 length means NULL */
 			bufptr = NULL;
 			nulls[i] = true;
-			csave = 0;			/* keep compiler quiet */
 		}
 		else
 		{
+			char	   *strbuff;
+
 			/*
-			 * Rather than copying data around, we just set up a phony
-			 * StringInfo pointing to the correct portion of the input buffer.
-			 * We assume we can scribble on the input buffer so as to maintain
-			 * the convention that StringInfos have a trailing null.
+			 * Rather than copying data around, we just initialize a
+			 * StringInfo pointing to the correct portion of the message
+			 * buffer.
 			 */
-			item_buf.data = &buf->data[buf->cursor];
-			item_buf.maxlen = itemlen + 1;
-			item_buf.len = itemlen;
-			item_buf.cursor = 0;
-
+			strbuff = &buf->data[buf->cursor];
 			buf->cursor += itemlen;
-
-			csave = buf->data[buf->cursor];
-			buf->data[buf->cursor] = '\0';
+			initReadOnlyStringInfo(&item_buf, strbuff, itemlen);
 
 			bufptr = &item_buf;
 			nulls[i] = false;
@@ -667,8 +660,6 @@ record_recv(PG_FUNCTION_ARGS)
 						(errcode(ERRCODE_INVALID_BINARY_REPRESENTATION),
 						 errmsg("improper binary format in record column %d",
 								i + 1)));
-
-			buf->data[buf->cursor] = csave;
 		}
 	}
 
diff --git a/src/common/stringinfo.c b/src/common/stringinfo.c
index 05b22b5c53..dc97754a68 100644
--- a/src/common/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -70,10 +70,16 @@ initStringInfo(StringInfo str)
  *
  * Reset the StringInfo: the data buffer remains valid, but its
  * previous content, if any, is cleared.
+ *
+ * Read-only StringInfos as initialized by initReadOnlyStringInfo cannot be
+ * reset.
  */
 void
 resetStringInfo(StringInfo str)
 {
+	/* don't allow resets of read-only StringInfos */
+	Assert(str->maxlen != 0);
+
 	str->data[0] = '\0';
 	str->len = 0;
 	str->cursor = 0;
@@ -284,6 +290,9 @@ enlargeStringInfo(StringInfo str, int needed)
 {
 	int			newlen;
 
+	/* validate this is not a read-only StringInfo */
+	Assert(str->maxlen != 0);
+
 	/*
 	 * Guard against out-of-range "needed" values.  Without this, we can get
 	 * an overflow or infinite loop in the following.
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index 36a416f8e0..019129a6d3 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -20,17 +20,25 @@
 
 /*-------------------------
  * StringInfoData holds information about an extensible string.
- *		data	is the current buffer for the string (allocated with palloc).
- *		len		is the current string length.  There is guaranteed to be
- *				a terminating '\0' at data[len], although this is not very
- *				useful when the string holds binary data rather than text.
+ *		data	is the current buffer for the string.
+ *		len		is the current string length.  Except in the case of read-only
+ *				strings described below, there is guaranteed to be a
+ *				terminating '\0' at data[len].
  *		maxlen	is the allocated size in bytes of 'data', i.e. the maximum
  *				string size (including the terminating '\0' char) that we can
  *				currently store in 'data' without having to reallocate
- *				more space.  We must always have maxlen > len.
- *		cursor	is initialized to zero by makeStringInfo or initStringInfo,
- *				but is not otherwise touched by the stringinfo.c routines.
- *				Some routines use it to scan through a StringInfo.
+ *				more space.  We must always have maxlen > len, except
+ *				in the read-only case described below.
+ *		cursor	is initialized to zero by makeStringInfo, initStringInfo,
+ *				initReadOnlyStringInfo and initStringInfoFromString but is not
+ *				otherwise touched by the stringinfo.c routines.  Some routines
+ *				use it to scan through a StringInfo.
+ *
+ * As a special case, a StringInfoData can be initialized with a read-only
+ * string buffer.  In this case "data" does not necessarily point at a
+ * palloc'd chunk, and management of the buffer storage is the caller's
+ * responsibility.  maxlen is set to zero to indicate that this is the case.
+ * Read-only StringInfoDatas cannot be appended to or reset.
  *-------------------------
  */
 typedef struct StringInfoData
@@ -45,7 +53,7 @@ typedef StringInfoData *StringInfo;
 
 
 /*------------------------
- * There are two ways to create a StringInfo object initially:
+ * There are four ways to create a StringInfo object initially:
  *
  * StringInfo stringptr = makeStringInfo();
  *		Both the StringInfoData and the data buffer are palloc'd.
@@ -56,8 +64,31 @@ typedef StringInfoData *StringInfo;
  *		This is the easiest approach for a StringInfo object that will
  *		only live as long as the current routine.
  *
+ * StringInfoData string;
+ * initReadOnlyStringInfo(&string, existingbuf, len);
+ *		The StringInfoData's data field is set to point directly to the
+ *		existing buffer and the StringInfoData's len is set to the given len.
+ *		The given buffer can point to memory that's not managed by palloc or
+ *		is pointing partway through a palloc'd chunk.  The maxlen field is set
+ *		to 0.  A read-only StringInfo cannot be appended to using any of the
+ *		appendStringInfo functions or reset with resetStringInfo().  The given
+ *		buffer can optionally omit the NUL termination character.
+ *
+ * StringInfoData string;
+ * initStringInfoFromString(&string, palloced_buf, len);
+ *		The StringInfoData's data field is set to point directly to the given
+ *		buffer and the StringInfoData's len is set to the given len.  This
+ *		method of initialization is useful when the buffer already exists.
+ *		StringInfos initialized this way can be appended to using the
+ *		appendStringInfo functions and reset with resetStringInfo().  The
+ *		given buffer must be NUL-terminated.  The palloc'd buffer is assumed
+ *		to be len + 1 in size.
+ *
  * To destroy a StringInfo, pfree() the data buffer, and then pfree() the
  * StringInfoData if it was palloc'd.  There's no special support for this.
+ * However, if the StringInfo was initialized using initReadOnlyStringInfo()
+ * then the caller will need to consider if it is safe to pfree the data
+ * buffer.
  *
  * NOTE: some routines build up a string using StringInfo, and then
  * release the StringInfoData but return the data string itself to their
@@ -79,6 +110,48 @@ extern StringInfo makeStringInfo(void);
  */
 extern void initStringInfo(StringInfo str);
 
+/*------------------------
+ * initReadOnlyStringInfo
+ * Initialize a StringInfoData struct from an existing string without copying
+ * the string.  The caller is responsible for ensuring the given string
+ * remains valid as long as the StringInfoData does.  Calls to this are used
+ * in performance critical locations where allocating a new buffer and copying
+ * would be too costly.  Read-only StringInfoData's may not be appended to
+ * using any of the appendStringInfo functions or reset with
+ * resetStringInfo().
+ *
+ * 'data' does not need to point directly to a palloc'd chunk of memory and may
+ * omit the NUL termination character at data[len].
+ */
+static inline void
+initReadOnlyStringInfo(StringInfo str, char *data, int len)
+{
+	str->data = data;
+	str->len = len;
+	str->maxlen = 0;			/* read-only */
+	str->cursor = 0;
+}
+
+/*------------------------
+ * initStringInfoFromString
+ * Initialize a StringInfoData struct from an existing string without copying
+ * the string.  'data' must be a valid palloc'd chunk of memory that can have
+ * repalloc() called should more space be required during a call to any of the
+ * appendStringInfo functions.
+ *
+ * 'data' must be NUL terminated at 'len' bytes.
+ */
+static inline void
+initStringInfoFromString(StringInfo str, char *data, int len)
+{
+	Assert(data[len] == '\0');
+
+	str->data = data;
+	str->len = len;
+	str->maxlen = len + 1;
+	str->cursor = 0;
+}
+
 /*------------------------
  * resetStringInfo
  * Clears the current content of the StringInfo, if any. The