Make serialization of Nodes' scalar-array fields more robust.

When the ability to print variable-length-array fields was first
added to outfuncs.c, there was no corresponding read capability,
as it was used only for debug dumps of planner-internal Nodes.
Not a lot of thought seems to have been put into the output format:
it's just the space-separated array elements and nothing else.
Later such fields appeared in Plan nodes, and still later we grew
read support so that Plans could be transferred to parallel workers,
but the original text format wasn't rethought.  It seems inadequate
to me because (a) no cross-check is possible that we got the right
number of array entries, (b) we can't tell the difference between
a NULL pointer and a zero-length array, and (c) except for
WRITE_INDEX_ARRAY, we'd crash if a non-zero length is specified
when the pointer is NULL, a situation that can arise in some fields
that we currently conveniently avoid printing.

Since we're currently in a campaign to make the Node infrastructure
generally more it-just-works-without-thinking-about-it, now seems
like a good time to improve this.

Let's adopt a format similar to that used for Lists, that is "<>"
for a NULL pointer or "(item item item)" otherwise.  Also retool
the code to not have so many copies of the identical logic.

I bumped catversion out of an abundance of caution, although I think
that we don't use any such array fields in Nodes that can get into
the catalogs.

Discussion: https://postgr.es/m/1528424.1658272135@sss.pgh.pa.us

diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 2b85f97f3982a851e3d4d224a88269a3f12416ee..4077fc4498d268fd2737cb0d70dfa40a1af993dd 100644 (file)
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -16,6 +16,7 @@
 
 #include <ctype.h>
 
+#include "access/attnum.h"
 #include "lib/stringinfo.h"
 #include "miscadmin.h"
 #include "nodes/bitmapset.h"
@@ -96,48 +97,30 @@ static void outChar(StringInfo str, char c);
    (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
     outBitmapset(str, node->fldname))
 
+/* Write a variable-length array of AttrNumber */
 #define WRITE_ATTRNUMBER_ARRAY(fldname, len) \
-   do { \
-       appendStringInfoString(str, " :" CppAsString(fldname) " "); \
-       for (int i = 0; i < len; i++) \
-           appendStringInfo(str, " %d", node->fldname[i]); \
-   } while(0)
+   (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+    writeAttrNumberCols(str, node->fldname, len))
 
+/* Write a variable-length array of Oid */
 #define WRITE_OID_ARRAY(fldname, len) \
-   do { \
-       appendStringInfoString(str, " :" CppAsString(fldname) " "); \
-       for (int i = 0; i < len; i++) \
-           appendStringInfo(str, " %u", node->fldname[i]); \
-   } while(0)
+   (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+    writeOidCols(str, node->fldname, len))
 
-/*
- * This macro supports the case that the field is NULL.  For the other array
- * macros, that is currently not needed.
- */
+/* Write a variable-length array of Index */
 #define WRITE_INDEX_ARRAY(fldname, len) \
-   do { \
-       appendStringInfoString(str, " :" CppAsString(fldname) " "); \
-       if (node->fldname) \
-           for (int i = 0; i < len; i++) \
-               appendStringInfo(str, " %u", node->fldname[i]); \
-       else \
-           appendStringInfoString(str, "<>"); \
-   } while(0)
+   (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+    writeIndexCols(str, node->fldname, len))
 
+/* Write a variable-length array of int */
 #define WRITE_INT_ARRAY(fldname, len) \
-   do { \
-       appendStringInfoString(str, " :" CppAsString(fldname) " "); \
-       for (int i = 0; i < len; i++) \
-           appendStringInfo(str, " %d", node->fldname[i]); \
-   } while(0)
+   (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+    writeIntCols(str, node->fldname, len))
 
+/* Write a variable-length array of bool */
 #define WRITE_BOOL_ARRAY(fldname, len) \
-   do { \
-       appendStringInfoString(str, " :" CppAsString(fldname) " "); \
-       for (int i = 0; i < len; i++) \
-           appendStringInfo(str, " %s", booltostr(node->fldname[i])); \
-   } while(0)
-
+   (appendStringInfoString(str, " :" CppAsString(fldname) " "), \
+    writeBoolCols(str, node->fldname, len))
 
 #define booltostr(x)  ((x) ? "true" : "false")
 
@@ -196,6 +179,38 @@ outChar(StringInfo str, char c)
    outToken(str, in);
 }
 
+/*
+ * common implementation for scalar-array-writing functions
+ *
+ * The data format is either "<>" for a NULL pointer or "(item item item)".
+ * fmtstr must include a leading space, and the rest of it must produce
+ * something that will be seen as a single simple token by pg_strtok().
+ * convfunc can be empty, or the name of a conversion macro or function.
+ */
+#define WRITE_SCALAR_ARRAY(fnname, datatype, fmtstr, convfunc) \
+static void \
+fnname(StringInfo str, const datatype *arr, int len) \
+{ \
+   if (arr != NULL) \
+   { \
+       appendStringInfoChar(str, '('); \
+       for (int i = 0; i < len; i++) \
+           appendStringInfo(str, fmtstr, convfunc(arr[i])); \
+       appendStringInfoChar(str, ')'); \
+   } \
+   else \
+       appendStringInfoString(str, "<>"); \
+}
+
+WRITE_SCALAR_ARRAY(writeAttrNumberCols, AttrNumber, " %d",)
+WRITE_SCALAR_ARRAY(writeOidCols, Oid, " %u",)
+WRITE_SCALAR_ARRAY(writeIndexCols, Index, " %u",)
+WRITE_SCALAR_ARRAY(writeIntCols, int, " %d",)
+WRITE_SCALAR_ARRAY(writeBoolCols, bool, " %s", booltostr)
+
+/*
+ * Print a List.
+ */
 static void
 _outList(StringInfo str, const List *node)
 {

diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index a2391280bea551a048cfee9fbca9407a2161209d..bee62fc15cd58cba6f705d565183b9517c94abfc 100644 (file)
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -502,97 +502,47 @@ readDatum(bool typbyval)
 }
 
 /*
- * readAttrNumberCols
- */
-AttrNumber *
-readAttrNumberCols(int numCols)
-{
-   int         tokenLength,
-               i;
-   const char *token;
-   AttrNumber *attr_vals;
-
-   if (numCols <= 0)
-       return NULL;
-
-   attr_vals = (AttrNumber *) palloc(numCols * sizeof(AttrNumber));
-   for (i = 0; i < numCols; i++)
-   {
-       token = pg_strtok(&tokenLength);
-       attr_vals[i] = atoi(token);
-   }
-
-   return attr_vals;
-}
-
-/*
- * readOidCols
- */
-Oid *
-readOidCols(int numCols)
-{
-   int         tokenLength,
-               i;
-   const char *token;
-   Oid        *oid_vals;
-
-   if (numCols <= 0)
-       return NULL;
-
-   oid_vals = (Oid *) palloc(numCols * sizeof(Oid));
-   for (i = 0; i < numCols; i++)
-   {
-       token = pg_strtok(&tokenLength);
-       oid_vals[i] = atooid(token);
-   }
-
-   return oid_vals;
-}
-
-/*
- * readIntCols
+ * common implementation for scalar-array-reading functions
+ *
+ * The data format is either "<>" for a NULL pointer (in which case numCols
+ * is ignored) or "(item item item)" where the number of items must equal
+ * numCols.  The convfunc must be okay with stopping at whitespace or a
+ * right parenthesis, since pg_strtok won't null-terminate the token.
  */
-int *
-readIntCols(int numCols)
-{
-   int         tokenLength,
-               i;
-   const char *token;
-   int        *int_vals;
-
-   if (numCols <= 0)
-       return NULL;
-
-   int_vals = (int *) palloc(numCols * sizeof(int));
-   for (i = 0; i < numCols; i++)
-   {
-       token = pg_strtok(&tokenLength);
-       int_vals[i] = atoi(token);
-   }
-
-   return int_vals;
+#define READ_SCALAR_ARRAY(fnname, datatype, convfunc) \
+datatype * \
+fnname(int numCols) \
+{ \
+   datatype   *vals; \
+   READ_TEMP_LOCALS(); \
+   token = pg_strtok(&length); \
+   if (token == NULL) \
+       elog(ERROR, "incomplete scalar array"); \
+   if (length == 0) \
+       return NULL;            /* it was "<>", so return NULL pointer */ \
+   if (length != 1 || token[0] != '(') \
+       elog(ERROR, "unrecognized token: \"%.*s\"", length, token); \
+   vals = (datatype *) palloc(numCols * sizeof(datatype)); \
+   for (int i = 0; i < numCols; i++) \
+   { \
+       token = pg_strtok(&length); \
+       if (token == NULL || token[0] == ')') \
+           elog(ERROR, "incomplete scalar array"); \
+       vals[i] = convfunc(token); \
+   } \
+   token = pg_strtok(&length); \
+   if (token == NULL || length != 1 || token[0] != ')') \
+       elog(ERROR, "incomplete scalar array"); \
+   return vals; \
 }
 
 /*
- * readBoolCols
+ * Note: these functions are exported in nodes.h for possible use by
+ * extensions, so don't mess too much with their names or API.
  */
-bool *
-readBoolCols(int numCols)
-{
-   int         tokenLength,
-               i;
-   const char *token;
-   bool       *bool_vals;
-
-   if (numCols <= 0)
-       return NULL;
-
-   bool_vals = (bool *) palloc(numCols * sizeof(bool));
-   for (i = 0; i < numCols; i++)
-   {
-       token = pg_strtok(&tokenLength);
-       bool_vals[i] = strtobool(token);
-   }
-
-   return bool_vals;
-}
+READ_SCALAR_ARRAY(readAttrNumberCols, int16, atoi)
+READ_SCALAR_ARRAY(readOidCols, Oid, atooid)
+/* outfuncs.c has writeIndexCols, but we don't yet need that here */
+/* READ_SCALAR_ARRAY(readIndexCols, Index, atoui) */
+READ_SCALAR_ARRAY(readIntCols, int, atoi)
+READ_SCALAR_ARRAY(readBoolCols, bool, strtobool)

diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 02ca4aec8c96dc1367bf27d67c33b4fa22c24187..c27fe0fcd8172cbcb2b2379f1a403428bd30543e 100644 (file)
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -57,6 +57,6 @@
  */
 
 /*                         yyyymmddN */
-#define CATALOG_VERSION_NO 202207131
+#define CATALOG_VERSION_NO 202207201
 
 #endif