Introduce the concept of read-only StringInfos
authorDavid Rowley <[email protected]>
Thu, 26 Oct 2023 03:31:48 +0000 (16:31 +1300)
committerDavid Rowley <[email protected]>
Thu, 26 Oct 2023 03:31:48 +0000 (16:31 +1300)
There were various places in our codebase which conjured up a StringInfo
by manually assigning the StringInfo fields and setting the data field
to point to some existing buffer.  There wasn't much consistency here as
to what fields like maxlen got set to and in one location we didn't
correctly ensure that the buffer was correctly NUL terminated at len
bytes, as per what was documented as required in stringinfo.h

Here we introduce 2 new functions to initialize StringInfos.  One allows
callers to initialize a StringInfo passing along a buffer that is
already allocated by palloc.  Here the StringInfo code uses this buffer
directly rather than doing any memcpying into a new allocation.  Having
this as a function allows us to verify the buffer is correctly NUL
terminated.  StringInfos initialized this way can be appended to and
reset just like any other normal StringInfo.

The other new initialization function also accepts an existing buffer,
but the given buffer does not need to be a pointer to a palloc'd chunk.
This buffer could be a pointer pointing partway into some palloc'd chunk
or may not even be palloc'd at all.  StringInfos initialized this way
are deemed as "read-only".  This means that it's not possible to
append to them or reset them.

For the latter of the two new initialization functions mentioned above,
we relax the requirement that the data buffer must be NUL terminated.
Relaxing this requirement is convenient in a few places as it can save
us from having to allocate an entire new buffer just to add the NUL
terminator or save us from having to temporarily add a NUL only to have to
put the original char back again later.

Incompatibility note:

Here we also forego adding the NUL in a few places where it does not
seem to be required.  These locations are passing the given StringInfo
into a type's receive function.  It does not seem like any of our
built-in receive functions require this, but perhaps there's some UDT
out there in the wild which does require this.  It is likely worthy of
a mention in the release notes that a UDT's receive function mustn't rely
on the input StringInfo being NUL terminated.

Author: David Rowley
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/CAApHDvorfO3iBZ%3DxpiZvp3uHtJVLyFaPBSvcAhAq2HPLnaNSwQ%40mail.gmail.com

src/backend/replication/logical/applyparallelworker.c
src/backend/replication/logical/proto.c
src/backend/replication/logical/worker.c
src/backend/tcop/postgres.c
src/backend/utils/adt/array_userfuncs.c
src/backend/utils/adt/arrayfuncs.c
src/backend/utils/adt/rowtypes.c
src/common/stringinfo.c
src/include/lib/stringinfo.h

index 82f48a488e9fa6bc536488e49b684200dd5d29c5..9b37736f8e5e536ba1eb22d28eb865a224de064d 100644 (file)
@@ -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
index d52c8963eb67e101d2c8a32d24e7726248cb6830..aa471dccdf305cd5f857a152f9cebdb39196450e 100644 (file)
@@ -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 functions 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);
index 54c14495bea5b9367c229241f3a8f4949a957451..567485c4a4d4e4036e2033e0ac910776afae135c 100644 (file)
@@ -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);
 
index c900427ecf957db1612028deff49887762a10b9b..6a070b5d8cb468f25910f3139dda75a01ea5c941 100644 (file)
@@ -1817,23 +1817,19 @@ 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 assume we can scribble on the message buffer to
+                                * add a trailing NUL which is required for the input function
+                                * call.
                                 */
-                               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
                        {
index 5c4fdcfba4693dcf6d83047806bd62cbe3adfd1d..c831a9395c696fc7e2a3a6afb558fa0640ccbf94 100644 (file)
@@ -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;
                }
        }
 
index 7828a6264b5822c0d69d50fc8b1b096ff47e6495..6a920a02b7266ad29db25ec100feea2b97cec5f6 100644 (file)
@@ -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;
        }
 
        /*
index ad176651d85843ce9a2972300a931fa3cfa80027..eb8fe95933039b85a7da7bc8c97e138f4f0ceb6e 100644 (file)
@@ -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;
                }
        }
 
index 05b22b5c53c6a4137f022dc74035ed71c8282e68..dc97754a685d3c15d3af05b4fb6829537fca61a5 100644 (file)
@@ -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.
index 36a416f8e0a0e3f151186b6c2b41ec96a40c3f53..598ed093dc8757fd08f089b0a1c3cb15ec0c37d0 100644 (file)
 
 /*-------------------------
  * 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.
+ * Also, it is caller's option whether a read-only string buffer has a
+ * terminating '\0' or not.  This depends on the intended usage.
  *-------------------------
  */
 typedef struct StringInfoData
@@ -45,7 +55,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 +66,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 trailing NUL.
+ *
+ * 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 +112,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