Re: Convert planner's AggInfo and AggTransInfo to Nodes

I wrote:
> * WRITE_OID_ARRAY and WRITE_BOOL_ARRAY needed extension to handle a null
> array pointer.  I think we should make all the WRITE_FOO_ARRAY macros
> work alike, so I added that to all of them.  I first tried to make the
> rest work like WRITE_INDEX_ARRAY, but that failed because readfuncs.c
> isn't expecting "<>" for an empty array; it's expecting nothing at
> all.  (Note there is no readfuncs equivalent to WRITE_INDEX_ARRAY.)
> What I've done here is to change WRITE_INDEX_ARRAY to work like the
> others and print nothing for an empty array, but I wonder if now
> wouldn't be a good time to redefine the serialized representation
> to be more robust.  I'm imagining "<>" for a NULL array pointer and
> "(item item item)" otherwise, allowing a cross-check that we're
> getting the right number of items.

Concretely, about like this.

            regards, tom lane

diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 2b85f97f39..fe97d559b6 100644
--- 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,37 @@ 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.
+ * 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 a2391280be..c77c3984ca 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -502,97 +502,45 @@ 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) \
+            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)
+READ_SCALAR_ARRAY(readIntCols, int, atoi)
+READ_SCALAR_ARRAY(readBoolCols, bool, strtobool)